8305906: HttpClient may use incorrect key when finding pooled HTTP/2 connection for IPv6 address

[jaikiran]

Can I please get a review of this change which proposes to fix the issue reported in https://bugs.openjdk.org/browse/JDK-8305906?

As noted in that issue, when IPv6 hosts are involved, the HttpClient on certain occasions can end up caching the connection with a key which doesn't match with the key which is then used in a subsequent request against the same target host.

The commit in this PR now wraps the IPv6 address in a square bracket consistently so that the correct key is used both during storing the connection in the pool and when looking up.

A new jtreg test has been added which reproduces this issue without the fix and verifies the fix.

---

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-8305906: HttpClient may use incorrect key when finding pooled HTTP/2 connection for IPv6 address (Bug - "4")

Reviewers

• Daniel Jeliński (@djelinski - Reviewer)
• Daniel Fuchs (@dfuch - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13456

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 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]

@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]

Webrevs

• 04: Full - Incremental (13821d88)
• 03: Full - Incremental (6db0ea8c)
• 02: Full - Incremental (96b92659)
• 01: Full - Incremental (77547fef)
• 00: Full (cf641a69)

[djelinski]

This patch improves the situation, but is not sufficient to address all cases. For example, http://[::1] will still not use the cache, because the cache key will contain [0:0:0:0:0:0:0:1] instead of [::1].

[jaikiran]

Thank you for catching that, Daniel. You are indeed right, the fix missed this part. I updated the new ConnectionReuseTest to run a test which matches what you described and that too fails, with the fix I had in this PR.

I have now updated the PR to use HttpRequestImpl.getAddress() (which returns a InetSocketAddress) instead of HttpRequestImpl.getURI() while looking up the cache keys. What this does is that it gets rid of the usage of the URI.getHost() API completely, during connection cache lookups. Instead, this now relies solely on the InetAddress API which always returns a long form address string. This is similar to what we do in the HTTP1 connection caching internally.

With this change the ConnectionReuseTest passes, even with the newly added additional test. I will run tier tests to make sure this doesn't impact anything else.

[djelinski]

Thanks for the fix, that looks better.

Now that we always use InetSocketAddress to build the cache key, do we still need to wrap the hostnames in brackets?

[djelinski]

LGTM

[openjdk]

@jaikiran This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8305906: HttpClient may use incorrect key when finding pooled HTTP/2 connection for IPv6 address

Reviewed-by: djelinski, dfuchs

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 4 new commits pushed to the master branch:

• a25b7b8: 8295976: GetThreadListStackTraces returns wrong state for blocked VirtualThread
• fadcd65: 8309527: Improve test proxy performance
• 0ed4af7: 8309472: IGV: Add dump_igv(custom_name) for improved debugging
• f1c7afc: 8306647: Implementation of Structured Concurrency (Preview)

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[bridgekeeper]

@jaikiran This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[jaikiran]

(can't believe it's already 4 weeks)
Please keep this open, I'll revive this shortly.

[jaikiran]

Hello @dfuch, @djelinski, I've done some minor changes to this PR to have keyFor accept a HttpRequestImpl instance so that it's clear where the InetSocketAddresses get sourced from when constructing the cache key. CI tests with this latest change continue to pass. Requesting a fresh review of this change, please.

[jaikiran]

Thank you both for the reviews.

[jaikiran]

/integrate

[openjdk]

Going to push as commit 3ccb3c0.
Since your change was applied there have been 4 commits pushed to the master branch:

• a25b7b8: 8295976: GetThreadListStackTraces returns wrong state for blocked VirtualThread
• fadcd65: 8309527: Improve test proxy performance
• 0ed4af7: 8309472: IGV: Add dump_igv(custom_name) for improved debugging
• f1c7afc: 8306647: Implementation of Structured Concurrency (Preview)

Your commit was automatically rebased without conflicts.

[openjdk]

@jaikiran Pushed as commit 3ccb3c0.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

[backwaterred]

Thanks for your change @jaikiran! We've seen this issue in my organization occurring in jdk17, and jdk11. It's nice to see that it has been fixed.

Do you have any plans to backport it?

[jaikiran]

Hello Tyler,

Do you have any plans to backport it?

I haven't been involved in the 11/17 backports in recent times, so I don't have any specific plans to backport this. Having said that, I think this does look like a good candidate for backporting, so if you or anyone has done it before, please do go ahead.

[backwaterred]

Sure. I can give it a go.