8350279: HttpClient: Add a new HttpResponse method to identify connections #24154

vy · 2025-03-21T13:45:24Z

Adds HttpResponse::connectionLabel method that provides an identifier for the connection.

Implementation note: The feature is facilitated by HttpConnection::label, which should not be confused with HttpConnection::id. This distinction is explained in the JavaDoc of both properties.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue
Change requires CSR request JDK-8352751 to be approved

Issues

JDK-8350279: HttpClient: Add a new HttpResponse method to identify connections (Enhancement - P4)
JDK-8352751: HttpClient: Add a new HttpResponse method to identify connections (CSR)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/24154/head:pull/24154
$ git checkout pull/24154

Update a local copy of the PR:
$ git checkout pull/24154
$ git pull https://git.openjdk.org/jdk.git pull/24154/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24154

View PR using the GUI difftool:
$ git pr show -t 24154

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/24154.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-03-21T13:46:31Z

👋 Welcome back vyazici! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-03-21T13:46:57Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

openjdk · 2025-03-21T13:47:36Z

@vy The following label will be automatically applied to this pull request:

net

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2025-03-21T13:51:31Z

Webrevs

test/jdk/java/net/httpclient/HttpResponseConnectionLabelTest.java

src/java.net.http/share/classes/jdk/internal/net/http/HttpResponseImpl.java

dfuch · 2025-03-21T16:51:32Z

/csr required

openjdk · 2025-03-21T16:52:18Z

@dfuch usage: /csr [needed|unneeded], requires that the issue the pull request refers to links to an approved CSR request.

liach · 2025-03-23T07:07:10Z

This new API returns an Optional.

Can users make any assumption about the connection identity when it is empty? Such as assuming such connections are all the same (as implied by your use of orElse(null) in tests) or all different?
If we cannot make any assumption about the empty return value, can users make any assumption that a HttpResponse obtained from the JDK built-in HttpClient always return non-empty for the label?

…nectionLabel` Co-authored-by: Daniel Fuchs <67001856+dfuch@users.noreply.github.com>

…nectionLabelTest`

vy · 2025-03-24T10:20:11Z

/csr needed

openjdk · 2025-03-24T10:20:29Z

@vy has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@vy please create a CSR request for issue JDK-8350279 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

dfuch · 2025-03-24T10:44:13Z

Can users make any assumption about the connection identity when it is empty? Such as assuming such connections are all the same (as implied by your use of orElse(null) in tests) or all different?

No. If empty is returned you can make no assumptions. In practice this should never be null with our impl (except possibly in whitebox tests)

If we cannot make any assumption about the empty return value, can users make any assumption that a HttpResponse obtained from the JDK built-in HttpClient always return non-empty for the label?

Yes.

liach · 2025-03-24T14:55:43Z

If we cannot make any assumption about the empty return value, can users make any assumption that a HttpResponse obtained from the JDK built-in HttpClient always return non-empty for the label?
Yes.

Should we specify this, like "The JDK built-in implementation of the {@code HttpClient} always assign a label to connections it create"?

dfuch · 2025-03-24T15:22:36Z

Should we specify this, like "The JDK built-in implementation of the {@code HttpClient} always assign a label to connections it create"?

Good question - that would be an @implNote - but I'm not sure this is required. The caller can determine whether a connection label is provided by testing whether the optional is empty.

src/java.net.http/share/classes/java/net/http/HttpResponse.java

jaikiran · 2025-03-27T12:45:07Z

src/java.net.http/share/classes/jdk/internal/net/http/HttpConnection.java

+     * {@return a label identifying the connection to facilitate
+     * {@link HttpResponse#connectionLabel() HttpResponse::connectionLabel}}
+     */
+    public String label() {


Perhaps make this method final?

Maybe not. We could want to extend it to give an indication that the connection is TCP or TLS - or Quic in the future...

jaikiran · 2025-03-27T12:47:22Z

src/java.net.http/share/classes/jdk/internal/net/http/HttpResponseImpl.java

@@ -44,6 +44,7 @@
 class HttpResponseImpl<T> implements HttpResponse<T>, RawChannel.Provider {


The copyright year on this file will need an update.

jaikiran · 2025-03-27T13:02:24Z

src/java.net.http/share/classes/jdk/internal/net/http/HttpConnection.java

+     * connection. For instance, an {@link AsyncSSLConnection} and the
+     * {@link PlainHttpConnection} it wraps will share the same label. As a
+     * result, compared to {@link #id}, this label does not give a total order
+     * among instances.


Hello Volkan, I think we shouldn't talk about any kind of ordering here. I see that the id field has been updated with a comment stating that it provides ordering among instances. Even there, I don't think that comment about ordering is needed. So far we haven't used the id to mean anything other than a logical identifier and these values have played no role in ordering of connections at any place. So adding the comments about ordering, I think, makes it confusing.

The only use of HttpConnection.id is for ordering - so that connections can be placed in a ConcurrentSkipListSet. So maybe here we should not speak of ordering (remove the last sentence in that paragraph) - but I think it helps to keep it for id.

The only use of HttpConnection.id is for ordering - so that connections can be placed in a ConcurrentSkipListSet.

I wasn't aware of that. Now that you mentioned it, I looked up the code which uses the Set to store these connections. And from what I can see, the order is only used during the closing of a HttpClient, to close these opened connections in that specific order. Did I miss any other usages of the order?

In any case, now that you corrected me about the usage of the id order, I agree that having the comment on the id field is fine and only remove it from the paragraph here.

When we close a connection, we take it out of the set. So it's not about ordering the connection but about quickly finding the connection in that set.

jaikiran · 2025-03-27T13:02:38Z

src/java.net.http/share/classes/jdk/internal/net/http/HttpConnection.java

+     * among instances.
+     * </p>
+     */
+    final String label;


I think this should be private.

jaikiran · 2025-03-27T13:07:44Z

src/java.net.http/share/classes/jdk/internal/net/http/HttpConnection.java

@@ -76,17 +78,48 @@ abstract class HttpConnection implements Closeable {
    public static final Comparator<HttpConnection> COMPARE_BY_ID
            = Comparator.comparing(HttpConnection::id);

+    private static final AtomicLong LABEL_COUNTER = new AtomicLong();


In the API documentation on HttpResponse.connectionLabel() we talk about the connection label being unique within a HttpClient scope. i.e. two different HttpClient instances could have the same connectionLabel for a connection. I think that's the right scoping.

So given that, having a static field on a HttpConnection which keeps track of a connection label, may not be the right place to keep track of that state. In this proposed form, no two connections in two different HttpClient instances will ever have the same connectionLabel. I think this counter should probably be present on the HttpClientImpl as an instance field.

I believe that's OK. We could go either way - but I'd rather keep the label unique regardless of the client because otherwise we would also need to include the client id in the debug traces (ATM we only have the label). If it's unique in the scope of the VM it's a fortiori unique in the scope of the client instance.

vy added 8 commits March 21, 2025 14:43

Initial HttpResponse::connectionLabel draft

c30d5bb

Add HttpConnection::connectionId and use that for ::connectionLabel

7cde035

Switch to old style JavaDoc in HttpResponse

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

8f63c22

Fix code typo in HttpConnection::connectionLabel

9765c08

Rename HttpConnection::connectionId to connectionLabel

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

4c5d559

Replace radix-62 usage with plain numbers

5c57d05

Revert HttpClientImpl changes

f8b0de5

Rename HttpConnection::connectionLabel to label

Loading
Loading status checks…

2857dde

openjdk bot added net rfr labels Mar 21, 2025

dfuch reviewed Mar 21, 2025

View reviewed changes

vy and others added 3 commits March 24, 2025 11:09

Switch from Optional<String> to String for `HttpResponseImpl::con…

Loading
Loading status checks…

028999a

…nectionLabel` Co-authored-by: Daniel Fuchs <67001856+dfuch@users.noreply.github.com>

Fix indentation

31a5647

Add import for java.net.http.HttpClient.Version in `HttpResponseCon…

Loading
Loading status checks…

a08c0cf

…nectionLabelTest`

openjdk bot added the csr label Mar 24, 2025

Michael-Mc-Mahon reviewed Mar 25, 2025

View reviewed changes

src/java.net.http/share/classes/java/net/http/HttpResponse.java Show resolved Hide resolved

Improve JavaDoc

Loading
Loading status checks…

59a11cc

jaikiran reviewed Mar 27, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8350279: HttpClient: Add a new HttpResponse method to identify connections #24154

8350279: HttpClient: Add a new HttpResponse method to identify connections #24154

vy commented Mar 21, 2025 •

edited by openjdk bot

Loading

bridgekeeper bot commented Mar 21, 2025

openjdk bot commented Mar 21, 2025

openjdk bot commented Mar 21, 2025

mlbridge bot commented Mar 21, 2025 •

edited

Loading

dfuch commented Mar 21, 2025

openjdk bot commented Mar 21, 2025

liach commented Mar 23, 2025

vy commented Mar 24, 2025

openjdk bot commented Mar 24, 2025

dfuch commented Mar 24, 2025

liach commented Mar 24, 2025

dfuch commented Mar 24, 2025

jaikiran Mar 27, 2025

dfuch Mar 27, 2025

jaikiran Mar 27, 2025

jaikiran Mar 27, 2025

dfuch Mar 27, 2025

jaikiran Mar 27, 2025

dfuch Mar 27, 2025

jaikiran Mar 27, 2025

jaikiran Mar 27, 2025

dfuch Mar 27, 2025 •

edited

Loading

		@@ -44,6 +44,7 @@
		class HttpResponseImpl<T> implements HttpResponse<T>, RawChannel.Provider {

8350279: HttpClient: Add a new HttpResponse method to identify connections #24154

Are you sure you want to change the base?

8350279: HttpClient: Add a new HttpResponse method to identify connections #24154

Conversation

vy commented Mar 21, 2025 • edited by openjdk bot Loading

Progress

Issues

Reviewing

bridgekeeper bot commented Mar 21, 2025

openjdk bot commented Mar 21, 2025

openjdk bot commented Mar 21, 2025

mlbridge bot commented Mar 21, 2025 • edited Loading

Webrevs

dfuch commented Mar 21, 2025

openjdk bot commented Mar 21, 2025

liach commented Mar 23, 2025

vy commented Mar 24, 2025

openjdk bot commented Mar 24, 2025

dfuch commented Mar 24, 2025

liach commented Mar 24, 2025

dfuch commented Mar 24, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfuch Mar 27, 2025 • edited Loading

Choose a reason for hiding this comment

vy commented Mar 21, 2025 •

edited by openjdk bot

Loading

mlbridge bot commented Mar 21, 2025 •

edited

Loading

dfuch Mar 27, 2025 •

edited

Loading