8361060: Keep track of the origin server against which a jdk.internal.net.http.HttpConnection was constructed #26041

jaikiran · 2025-06-30T11:07:55Z

Can I please get a review of this change which updates the jdk.internal.net.http.HttpConnection to keep track of the origin server for which the HttpConnection was constructed? This addresses https://bugs.openjdk.org/browse/JDK-8361060.

This is an internal implementation change which will allow other parts of the JDK's HttpClient implementation to use the origin server information. An example of such usage is the alternate services that are going to be supported in the JDK's HttpClient upcoming implementation for HTTP/3.

No new tests have been introduced and existing tests in tier1, tier2 and tier3 continue to pass.

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

Issue

JDK-8361060: Keep track of the origin server against which a jdk.internal.net.http.HttpConnection was constructed (Enhancement - P4)

Reviewers

Daniel Fuchs (@dfuch - Reviewer) 🔄 Re-review required (review applies to b2ba49ce)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 26041

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

Using diff file

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

Using Webrev

Link to Webrev Comment

….net.http.HttpConnection was constructed

bridgekeeper · 2025-06-30T11:08:56Z

👋 Welcome back jpai! 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-06-30T11:09:33Z

@jaikiran This change is no longer ready for integration - check the PR body for details.

openjdk · 2025-06-30T11:10:01Z

@jaikiran 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-06-30T11:12:55Z

Webrevs

dfuch · 2025-06-30T11:18:52Z

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

                       HttpClientImpl client,
                       String[] alpn,
                       String label) {
-        super(addr, client, Utils.getServerName(addr), addr.getPort(), alpn, label);
-        plainConnection = new PlainHttpConnection(addr, client, label);
+        super(originServer, addr, client, Utils.getServerName(addr), addr.getPort(), alpn, label);


Do we still need Utils.getServerName(addr), addr.getPort(), now that we have originServer? Or in other words - should we base these calls on the addr parameter or on the originServer parameters?

I had a look at it and you are right - with the introduction of this Origin construct, it should now be possible (and in fact prefered) to use it for constructing the SNI names. I updated this code to use the Origin instance.

While doing that, the change caused a test failure which exposed one detail that I hadn't considered. java.net.URI.getHost() is specified to return a IPv6 address enclosed in square brackets. The Origin class hadn't considered that previously. It's now been updated to strip the square brackets when constructing the host value. This will allow the Origin.host() to be used in places where it was/is being used as a host that might be returned from a InetAddress. Local testing with this change now passes all the tests. I'll now trigger tier testing in our CI.

dfuch · 2025-06-30T11:19:26Z

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

                             HttpClientImpl client,
                             String[] alpn,
                             InetSocketAddress proxy,
                             ProxyHeaders proxyHeaders,
                             String label)
    {
-        super(addr, client, Utils.getServerName(addr), addr.getPort(), alpn, label);
-        this.plainConnection = new PlainTunnelingConnection(addr, proxy, client, proxyHeaders, label);
+        super(originServer, addr, client, Utils.getServerName(addr), addr.getPort(), alpn, label);


Same question

Updated to use the Origin instance.

dfuch · 2025-06-30T11:21:32Z

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

+        }
+        return host + ":" + port;
+    }
+}


Missing newline.

dfuch · 2025-06-30T11:28:49Z

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

+public record Origin(String scheme, String host, int port) {
+    public Origin {
+        Objects.requireNonNull(scheme);
+        Objects.requireNonNull(host);
+        if (port <= 0) {
+            throw new IllegalArgumentException("Invalid port");
+        }
+    }


Should we enforce lower case for scheme and host in this constructor?
For instance - convert to lower case if needed in from(URI) and throw/assert here if not lower case?

We only support http and https. So I think it makes sense to convert it to lower case and expect the scheme to be either of these. I've updated the PR accordingly.

dfuch

Looks good. I wonder if we should lower case the host returned by getHost() too (when not a literal address).

jaikiran · 2025-07-02T07:40:37Z

I wonder if we should lower case the host returned by getHost() too (when not a literal address).

Done. The PR has been updated with this change. tier1, tier2 and tier3 test pass with this change. Given the kind of code we added in this Origin class, I'm now considering introducing a test for this class' behaviour. I'll update this PR shortly.

jaikiran · 2025-07-02T12:43:11Z

I've updated the PR to include a test for the Origin class. No other changes. The test passes with these changes.

8361060: Keep track of the origin server against which a jdk.internal…

7b34255

….net.http.HttpConnection was constructed

openjdk bot added the rfr Pull request is ready for review label Jun 30, 2025

openjdk bot added the net net-dev@openjdk.org label Jun 30, 2025

dfuch reviewed Jun 30, 2025

View reviewed changes

jaikiran added 4 commits June 30, 2025 18:21

add new line

9ef8828

support only lower case http and https literals for scheme in Origin

aa50883

strip the square brackets from URI's host when constructing an Origin

3a6b1ca

SNI server names can now be derived from the Origin instance

c8825e0

dfuch reviewed Jun 30, 2025

View reviewed changes

jaikiran added 2 commits July 2, 2025 10:37

merge latest from master branch

ba56523

Daniel's review - store Origin.host as lower case for DNS hostnames

b2ba49c

dfuch approved these changes Jul 2, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jul 2, 2025

add a test for jdk.internal.net.http.Origin

711840f

openjdk bot removed the ready Pull request is ready to be integrated label Jul 2, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

8361060: Keep track of the origin server against which a jdk.internal.net.http.HttpConnection was constructed #26041

8361060: Keep track of the origin server against which a jdk.internal.net.http.HttpConnection was constructed #26041

Uh oh!

jaikiran commented Jun 30, 2025 •

edited by openjdk bot

Loading

Uh oh!

bridgekeeper bot commented Jun 30, 2025

Uh oh!

openjdk bot commented Jun 30, 2025 •

edited

Loading

Uh oh!

openjdk bot commented Jun 30, 2025

Uh oh!

mlbridge bot commented Jun 30, 2025 •

edited

Loading

Uh oh!

dfuch Jun 30, 2025

Uh oh!

jaikiran Jun 30, 2025

Uh oh!

dfuch Jun 30, 2025

Uh oh!

jaikiran Jun 30, 2025

Uh oh!

dfuch Jun 30, 2025

Uh oh!

jaikiran Jun 30, 2025

Uh oh!

dfuch Jun 30, 2025

Uh oh!

jaikiran Jun 30, 2025

Uh oh!

dfuch left a comment

Uh oh!

jaikiran commented Jul 2, 2025

Uh oh!

jaikiran commented Jul 2, 2025

Uh oh!

Uh oh!

8361060: Keep track of the origin server against which a jdk.internal.net.http.HttpConnection was constructed #26041

Are you sure you want to change the base?

8361060: Keep track of the origin server against which a jdk.internal.net.http.HttpConnection was constructed #26041

Uh oh!

Conversation

jaikiran commented Jun 30, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Jun 30, 2025

Uh oh!

openjdk bot commented Jun 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Jun 30, 2025

Uh oh!

mlbridge bot commented Jun 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dfuch left a comment

Choose a reason for hiding this comment

Uh oh!

jaikiran commented Jul 2, 2025

Uh oh!

jaikiran commented Jul 2, 2025

Uh oh!

Uh oh!

jaikiran commented Jun 30, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Jun 30, 2025 •

edited

Loading

mlbridge bot commented Jun 30, 2025 •

edited

Loading