8293562: KeepAliveCache Blocks Threads while Closing Connections

[dhanalla]

trafficstars

Issue: JDK-8293562: KeepAliveCache Blocks Threads while Closing Connections Backport of PR https://github.com/openjdk/jdk11u-dev/pull/1890 fromJDK11 to JDK8.

The JDK11 patch doesn't apply cleanly 3/11 hunks need to be ported manually in KeepAliveCache.java.

Test performed:

1. JTreg jdk_tier1
2. JTreg jdk_tier2
3. New test case added to this PR passed with the fix and failed without it.
4. The customer failure scenario has been validated.

---

Progress

• [x] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• [ ] JDK-8293562 needs maintainer approval
• [x] Change must not contain extraneous whitespace
• [x] Commit message must refer to an issue

Issue

• JDK-8293562: KeepAliveCache Blocks Threads while Closing Connections (Bug - P4 - Requested)

Reviewers

• Severin Gehwolf (@jerboaa - Reviewer)
• Paul Hohensee (@phohensee - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk8u-dev.git pull/409/head:pull/409
$ git checkout pull/409

Update a local copy of the PR:
$ git checkout pull/409
$ git pull https://git.openjdk.org/jdk8u-dev.git pull/409/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 409

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk8u-dev/pull/409.diff

Webrev

Link to Webrev Comment

[bridgekeeper[bot]]

:wave: Welcome back dhanalla! A progress list of the required criteria for merging this PR into pr/407 will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk[bot]]

This backport pull request has now been updated with issue from the original commit.

[mlbridge[bot]]

Webrevs

• 04: Full - Incremental (0476345c)
• 03: Full (36cb9124)
• 02: Full (210bca0f)
• 01: Full - Incremental (7d73c521)
• 00: Full (0d51b250)

[bridgekeeper[bot]]

@dhanalla 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!

[dhanalla]

@dhanalla 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!

adding a comment to keep the PR active.

[dhanalla]

This PR seems OK. It would have been easier to review if the whitespace wasn't all different. Consider adding jerboaa@054efe5 to make this more aligned with JDK 11u. Finally, I wonder if we should bring in https://bugs.openjdk.org/browse/JDK-8278067 first so as to make the patch cleaner (as compared to JDK 11). That one is in 8u361 oracle as well, so would probably good to have and seems low risk. Meanwhile, please apply for approval for #407 so this can get in.

Thanks @jerboaa for reviewing this PR. I have submitted a request for approval for PR https://github.com/openjdk/jdk8u-dev/pull/407 and have addressed the indentation and spacing issues.

[dhanalla]

This PR seems OK. It would have been easier to review if the whitespace wasn't all different. Consider adding jerboaa@054efe5 to make this more aligned with JDK 11u. Finally, I wonder if we should bring in https://bugs.openjdk.org/browse/JDK-8278067 first so as to make the patch cleaner (as compared to JDK 11). That one is in 8u361 oracle as well, so would probably good to have and seems low risk. Meanwhile, please apply for approval for #407 so this can get in.

Thanks @jerboaa for reviewing this PR. I have submitted a request for approval for PR #407 and have addressed the indentation and spacing issues.

I am happy to backport the bug https://bugs.openjdk.org/browse/JDK-8278067, but my concern is that the backport of the other files in the PR (https://github.com/openjdk/jdk11u-dev/pull/1193) may not be clean and could result in bringing in other changes.

[jerboaa]

This PR seems OK. It would have been easier to review if the whitespace wasn't all different. Consider adding jerboaa@054efe5 to make this more aligned with JDK 11u. Finally, I wonder if we should bring in https://bugs.openjdk.org/browse/JDK-8278067 first so as to make the patch cleaner (as compared to JDK 11). That one is in 8u361 oracle as well, so would probably good to have and seems low risk. Meanwhile, please apply for approval for #407 so this can get in.

Thanks @jerboaa for reviewing this PR. I have submitted a request for approval for PR #407 and have addressed the indentation and spacing issues.

I am happy to backport the bug https://bugs.openjdk.org/browse/JDK-8278067, but my concern is that the backport of the other files in the PR (openjdk/jdk11u-dev#1193) may not be clean and could result in bringing in other changes.

Yes, I'd prefer that. Get #407 integrated, then work on JDK-8278067 (it shouldn't require any other changes; at least that's my understanding when looking at the patch). Then rebase this one on top of JDK-8278067. JDK-8278067 might not be clean but it's a fairly contained change.

[openjdk-notifier[bot]]

The parent pull request that this pull request depends on has now been integrated and the target branch of this pull request has been updated. This means that changes from the dependent pull request can start to show up as belonging to this pull request, which may be confusing for reviewers. To remedy this situation, simply merge the latest changes from the new target branch into this pull request by running commands similar to these in the local repository for your personal fork:

git checkout Backport-JDK-8293562
git fetch https://git.openjdk.org/jdk8u-dev.git master
git merge FETCH_HEAD
# if there are conflicts, follow the instructions given by git merge
git commit -m "Merge master"
git push

[openjdk[bot]]

@dhanalla this pull request can not be integrated into pr/437 due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout Backport-JDK-8293562
git fetch https://git.openjdk.org/jdk8u-dev.git pr/437
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge pr/437"
git push

[theRealAph]

I'm surprised you think this is suitable for 8u. It's not low risk, and it's more of a performance fix than a bug, and it's P4. JDK 8u is done. It's a long-term maintenance release.

[dhanalla]

I'm surprised you think this is suitable for 8u. It's not low risk, and it's more of a performance fix than a bug, and it's P4. JDK 8u is done. It's a long-term maintenance release.

Thanks for reviewing this PR @theRealAph ,

I understand that this is not a Low-risk. We've observed that two of our valued customers' applications, which heavily rely on HttpClient and handle thousands of requests per second, encounter significant challenges with TLS 1.3. Specifically, these applications become almost unresponsive due to a huge number of blocked threads (The same issue is explained here https://techcommunity.microsoft.com/t5/azure-storage-blog/prepare-for-upcoming-tls-1-3-support-for-azure-storage/ba-p/4034014) Unfortunately, due to various constraints, these customers are unable to upgrade to the later JDK version or revert to TLS 1.2. Both of these customers have explicitly conveyed that implementing this fix would greatly alleviate their current roadblocks.

The attached test simulates a scenario in which the second HTTP client read request fails with a timeout as it is blocked by the first socket close connection. This blocking occurs due to synchronization locks in the KeepAliveCache's put and Keep-Alive-Timer thread methods, with the Keep-Alive-Timer thread handling socket closure while holding a lock. This means that while it's closing connections, no other connections can be established, retrieved from the cache, or added to the cache. And the test passes with the fix.

[jerboaa]

If we are going to backport this, then I think we should use the following approach in order to keep the risk manageable (and to keep in sync with what had been done in JDK 11):

1. Get JDK-8278067: Make HttpURLConnection default keep alive timeout configurable into JDK 8u. That's #437
2. Get JDK-8291226: Create Test Cases to cover scenarios for JDK-8278067 backported as well as it includes correctness tests for JDK-8293562 as well.
3. Get JDK-8291638: Keep-Alive timeout of 0 should close connection immediately backported.

[dhanalla]

If we are going to backport this, then I think we should use the following approach in order to keep the risk manageable (and to keep in sync with what had been done in JDK 11):

1. Get JDK-8278067: Make HttpURLConnection default keep alive timeout configurable into JDK 8u. That's 8278067: Make HttpURLConnection default keep alive timeout configurable #437
2. Get JDK-8291226: Create Test Cases to cover scenarios for JDK-8278067 backported as well as it includes correctness tests for JDK-8293562 as well.
3. Get JDK-8291638: Keep-Alive timeout of 0 should close connection immediately backported.

Thanks @jerboaa , created backport PRs for both 2 & 3. JDK-8291226: https://github.com/openjdk/jdk8u-dev/pull/453 JDK-8291638: https://github.com/openjdk/jdk8u-dev/pull/454

[ltning]

I'd like to pitch in that this feels (to us) like a serious bug that warrants fixing in 8. I don't know what the criteria for calling something a "performance issue" are, but my gut feeling tells me it falls far short of being classified as "just" that. Our application ground to a complete halt (not just a slowdown) when the client switched on TLS 1.3, and it took us the better part of a day to figure out the root cause and restore service.

My few cents, and a huge thanks to @dhanalla and anyone else working on backporting this fix.

[openjdk[bot]]

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

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

8293562: KeepAliveCache Blocks Threads while Closing Connections

Reviewed-by: sgehwolf, phh, andrew

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 14 new commits pushed to the master branch:

• 89def4d2c94f5e174f704318f31fb85a1fee4561: 8331980: [8u] Problem list CAInterop.java#certignarootca test
• 6b53212ef78ad50f9eede829c5ff87cadcdb434b: 8258855: Two tests sun/security/krb5/auto/ReplayCacheTestProc.java and ReplayCacheTestProcWithMD5.java failed on OL8.3
• 6a096374a52eddf28fb87c2cfe1778ef6d3be8e9: 8159454: [TEST_BUG] javax/swing/ToolTipManager/7123767/bug7123767.java: number of checked graphics configurations should be limited
• 68ab0f3b243c7b7934b47ccf9513f052aaf6536f: 8318410: jdk/java/lang/instrument/BootClassPath/BootClassPathTest.sh fails on Japanese Windows
• 0e0b018acba9a869f8795a18b03f94e85cfaf86f: Merge
• 43cb87550865a93c559c9e8eaa59fcb071301bd3: 8317507: C2 compilation fails with "Exceeded _node_regs array"
• 97be54783b8e5464dcb9796c1714e8b69c106409: 8322122: Enhance generation of addresses
• 2ffb900d7eca6472fd21d362d00e229babb7bd3f: 8322114: Improve Pack 200 handling
• ce255d6565fb6819596e218161538bde327daf91: 8319851: Improve exception logging
• 54419d4dcd230a1c7bd9fc67a169c6ad6cb71385: 8318340: Improve RSA key implementations
• ... and 4 more: https://git.openjdk.org/jdk8u-dev/compare/d0d9a152d14fb3e056d5534ab7e6b0bde3870c14...master

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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@jerboaa, @phohensee, @gnu-andrew) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[openjdk-notifier[bot]]

The parent pull request that this pull request depends on has now been integrated and the target branch of this pull request has been updated. This means that changes from the dependent pull request can start to show up as belonging to this pull request, which may be confusing for reviewers. To remedy this situation, simply merge the latest changes from the new target branch into this pull request by running commands similar to these in the local repository for your personal fork:

git checkout Backport-JDK-8293562
git fetch https://git.openjdk.org/jdk8u-dev.git master
git merge FETCH_HEAD
# if there are conflicts, follow the instructions given by git merge
git commit -m "Merge master"
git push

[openjdk[bot]]

⚠️ @dhanalla This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

[dhanalla]

This seems OK now modulo some whitespace changes. The best course of action would be to compare the KeepAliveCache.java file to the version from jdk11u and fix the whitespace changes in the current version. The only difference then should be in the AccessController.doPrivileged( block between JDK 8 and 11 and nothing else.

http tests pass for me:

Passed: sun/net/www/http/ChunkedInputStream/ChunkedEncodingTest.java
Passed: sun/net/www/http/ChunkedInputStream/ChunkedEncodingWithProgressMonitorTest.java
Passed: sun/net/www/http/ChunkedInputStream/TestAvailable.java
Passed: sun/net/www/http/ChunkedOutputStream/checkError.java
Passed: sun/net/www/http/ChunkedOutputStream/Test.java
Passed: sun/net/www/http/HttpClient/B6726695.java
Passed: sun/net/www/http/HttpClient/B7025238.java
Passed: sun/net/www/http/HttpClient/B8025710.java
Passed: sun/net/www/http/HttpClient/B8209178.java
Passed: sun/net/www/http/HttpClient/CookieHttpClientTest.java
Passed: sun/net/www/http/HttpClient/GetProxyPort.java
Passed: sun/net/www/http/HttpClient/ImplicitFileName.java
Passed: sun/net/www/http/HttpClient/IsAvailable.java
Passed: sun/net/www/http/HttpClient/IsKeepingAlive.java
Passed: sun/net/www/http/HttpClient/KeepAliveTest.java
Passed: sun/net/www/http/HttpClient/MultiThreadTest.java
Passed: sun/net/www/http/HttpClient/OpenServer.java
Passed: sun/net/www/http/HttpClient/ProxyFromCache.java
Passed: sun/net/www/http/HttpClient/ProxyTest.java
Passed: sun/net/www/http/HttpClient/RequestURI.java
Passed: sun/net/www/http/HttpClient/RetryPost.sh
Passed: sun/net/www/http/HttpClient/StreamingRetry.java
Passed: sun/net/www/http/HttpURLConnection/PostOnDelete.java
Passed: sun/net/www/http/KeepAliveCache/B5045306.java
Passed: sun/net/www/http/KeepAliveCache/B8291637.java
Passed: sun/net/www/http/KeepAliveCache/B8293562.java
Passed: sun/net/www/http/KeepAliveCache/KeepAliveProperty.java
Passed: sun/net/www/http/KeepAliveCache/KeepAliveTimerThread.java
Passed: sun/net/www/http/KeepAliveStream/InfiniteLoop.java
Passed: sun/net/www/http/KeepAliveStream/KeepAliveStreamClose.java
Passed: sun/net/www/http/KeepAliveStream/KeepAliveStreamCloseWithWrongContentLength.java
Passed: sun/net/www/http/RequestMethodCheck/RequestMethodEquality.java
Test results: passed: 32

Done

[openjdk[bot]]

⚠️ @dhanalla This change is now ready for you to apply for maintainer approval. This can be done directly in each associated issue or by using the /approval command.

[dhanalla]

Thanks @jerboaa & @phohensee for reviewing this PR. I added the JDK8u fix request tag to the bug as the next step. Please let me know if there is anything else that needs to be addressed.

[gnu-andrew]

This seems OK now modulo some whitespace changes. The best course of action would be to compare the KeepAliveCache.java file to the version from jdk11u and fix the whitespace changes in the current version. The only difference then should be in the AccessController.doPrivileged( block between JDK 8 and 11 and nothing else.

This now looks correct after applying the patch, though the 8u & 11u patches look different to achieve the same end result. There's an additional whitespace change which I'm not sure if it was missed in an earlier backport or 8u only to begin with, but the file looks good after this patch, which is the main thing.

[gnu-andrew]

/approve yes

[openjdk[bot]]

@gnu-andrew 8293562: The approval request has been approved.

[dhanalla]

/integrate

[dhanalla]

@gnu-andrew 8293562: The approval request has been approved.

Thanks @gnu-andrew for reviewing this PR.

[openjdk[bot]]

@dhanalla Your change (at version 0476345ce007d26a9d3dffe785e97e5d123345f1) is now ready to be sponsored by a Committer.

[gnu-andrew]

/sponsor

[openjdk[bot]]

Going to push as commit 853a87a8e4c4a075e19ddcd6d3d4a7a924967f32. Since your change was applied there have been 14 commits pushed to the master branch:

• 89def4d2c94f5e174f704318f31fb85a1fee4561: 8331980: [8u] Problem list CAInterop.java#certignarootca test
• 6b53212ef78ad50f9eede829c5ff87cadcdb434b: 8258855: Two tests sun/security/krb5/auto/ReplayCacheTestProc.java and ReplayCacheTestProcWithMD5.java failed on OL8.3
• 6a096374a52eddf28fb87c2cfe1778ef6d3be8e9: 8159454: [TEST_BUG] javax/swing/ToolTipManager/7123767/bug7123767.java: number of checked graphics configurations should be limited
• 68ab0f3b243c7b7934b47ccf9513f052aaf6536f: 8318410: jdk/java/lang/instrument/BootClassPath/BootClassPathTest.sh fails on Japanese Windows
• 0e0b018acba9a869f8795a18b03f94e85cfaf86f: Merge
• 43cb87550865a93c559c9e8eaa59fcb071301bd3: 8317507: C2 compilation fails with "Exceeded _node_regs array"
• 97be54783b8e5464dcb9796c1714e8b69c106409: 8322122: Enhance generation of addresses
• 2ffb900d7eca6472fd21d362d00e229babb7bd3f: 8322114: Improve Pack 200 handling
• ce255d6565fb6819596e218161538bde327daf91: 8319851: Improve exception logging
• 54419d4dcd230a1c7bd9fc67a169c6ad6cb71385: 8318340: Improve RSA key implementations
• ... and 4 more: https://git.openjdk.org/jdk8u-dev/compare/d0d9a152d14fb3e056d5534ab7e6b0bde3870c14...master

Your commit was automatically rebased without conflicts.

[openjdk[bot]]

@gnu-andrew @dhanalla Pushed as commit 853a87a8e4c4a075e19ddcd6d3d4a7a924967f32.

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