8293562: KeepAliveCache Blocks Threads while Closing Connections #409

dhanalla · 2023-12-18T19:11:29Z

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

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

Test performed:

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

Progress

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

Issue

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

Reviewers

Severin Gehwolf (@jerboaa - Reviewer)
Paul Hohensee (@phohensee - Reviewer)
Andrew John Hughes (@gnu-andrew - 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 · 2023-12-18T19:12:27Z

👋 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 · 2023-12-18T19:13:46Z

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

mlbridge · 2023-12-18T19:16:56Z

Webrevs

bridgekeeper · 2024-01-16T01:42:02Z

@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 · 2024-01-16T17:13:34Z

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

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.

jerboaa · 2024-01-30T13:06:46Z

jdk/src/share/classes/sun/net/www/http/KeepAliveCache.java

        long currentTime = System.currentTimeMillis();
-        do {
-            KeepAliveEntry e = pop();
            if ((currentTime - e.idleStartTime) > nap) {


Indenting is off.

jerboaa · 2024-01-30T13:07:13Z

jdk/src/share/classes/sun/net/www/http/KeepAliveCache.java

+                 for (HttpClient hc : closeList) {
+                     hc.closeServer();
+               }
+          }


There are some whitespace issues here. Please clean this up.

jerboaa · 2024-01-30T13:08:22Z

jdk/src/share/classes/sun/net/www/http/KeepAliveCache.java

+        }
+    }
+        // close after releasing locks
+        if (oldClient != null) {
+            oldClient.closeServer();


Whitespace and aligned-ness of braces are off here.

dhanalla · 2024-02-02T01:37:15Z

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.

dhanalla · 2024-02-05T01:41:21Z

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.

jerboaa · 2024-02-05T10:13:01Z

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 · 2024-02-08T10:25:31Z

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 · 2024-02-08T17:05:24Z

@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 · 2024-02-09T09:59:43Z

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 · 2024-02-14T22:37:47Z

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.

openjdk · 2024-04-08T15:54:46Z

⚠️ @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).

jerboaa

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

dhanalla · 2024-04-09T17:11:05Z

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

jerboaa

This looks OK to me, but I'd like for somebody else to give the go-ahead as well.

openjdk · 2024-04-22T17:35:25Z

⚠️ @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.

phohensee

Lgtm. The only real difference from the 11u backport looks to be the keepAliveTimer thread creation code.

dhanalla · 2024-05-02T16:32:54Z

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 · 2024-05-14T00:22:38Z

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

Backport looks good.

gnu-andrew · 2024-05-14T00:23:09Z

/approve yes

openjdk · 2024-05-14T00:24:48Z

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

dhanalla · 2024-05-14T00:30:55Z

/integrate

dhanalla · 2024-05-14T00:31:36Z

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

Thanks @gnu-andrew for reviewing this PR.

openjdk · 2024-05-14T00:31:42Z

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

gnu-andrew · 2024-05-14T00:53:00Z

/sponsor

openjdk · 2024-05-14T00:53:53Z

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

89def4d: 8331980: [8u] Problem list CAInterop.java#certignarootca test
6b53212: 8258855: Two tests sun/security/krb5/auto/ReplayCacheTestProc.java and ReplayCacheTestProcWithMD5.java failed on OL8.3
6a09637: 8159454: [TEST_BUG] javax/swing/ToolTipManager/7123767/bug7123767.java: number of checked graphics configurations should be limited
68ab0f3: 8318410: jdk/java/lang/instrument/BootClassPath/BootClassPathTest.sh fails on Japanese Windows
0e0b018: Merge
43cb875: 8317507: C2 compilation fails with "Exceeded _node_regs array"
97be547: 8322122: Enhance generation of addresses
2ffb900: 8322114: Improve Pack 200 handling
ce255d6: 8319851: Improve exception logging
54419d4: 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 · 2024-05-14T00:54:03Z

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

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

dhanalla added 2 commits December 15, 2023 18:21

Backport of 2e31cc7ee1b875af7c7b3a5367ac8056fbc60650

0ef34e3

Backport 770c1f65c588f3156f9b70097df752d8059c1038

0d51b25

openjdk bot changed the title ~~Backport 770c1f65c588f3156f9b70097df752d8059c1038~~ 8293562: KeepAliveCache Blocks Threads while Closing Connections Dec 18, 2023

openjdk bot added backport rfr Pull request is ready for review labels Dec 18, 2023

dhanalla mentioned this pull request Dec 18, 2023

8293562: KeepAliveCache Blocks Threads while Closing Connections #402

Closed

4 tasks

dhanalla mentioned this pull request Jan 29, 2024

8155590: Dubious collection management in sun.net.www.http.KeepAliveCache #407

Closed

4 tasks

jerboaa reviewed Jan 30, 2024

View reviewed changes

Fix indentation

7d73c52

dhanalla added 2 commits February 7, 2024 16:51

Backport d8f44aa39e921594505864e6270f42b745265293

80387af

Fix indentation

22fb611

openjdk-notifier bot changed the base branch from pr/407 to master February 8, 2024 10:25

dhanalla changed the base branch from master to pr/437 February 8, 2024 17:04

openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Feb 8, 2024

Merge pr/437

210bca0

openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Feb 8, 2024

dhanalla mentioned this pull request Feb 20, 2024

8278067: Make HttpURLConnection default keep alive timeout configurable #437

Closed

5 tasks

Remove SystemProperty tag as it is not supported in JDK8

f44e27b

openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Apr 8, 2024

Merge master

36cb912

openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Apr 8, 2024

jerboaa reviewed Apr 9, 2024

View reviewed changes

Fix white spaces

0476345

jerboaa approved these changes Apr 22, 2024

View reviewed changes

phohensee approved these changes Apr 22, 2024

View reviewed changes

openjdk bot added the approval label Apr 22, 2024

gnu-andrew approved these changes May 14, 2024

View reviewed changes

openjdk bot added ready Pull request is ready to be integrated and removed approval labels May 14, 2024

openjdk bot added the sponsor Pull request is ready to be sponsored label May 14, 2024

openjdk bot added the integrated Pull request has been integrated label May 14, 2024

openjdk bot closed this May 14, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels May 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8293562: KeepAliveCache Blocks Threads while Closing Connections #409

8293562: KeepAliveCache Blocks Threads while Closing Connections #409

dhanalla commented Dec 18, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Dec 18, 2023

openjdk bot commented Dec 18, 2023

mlbridge bot commented Dec 18, 2023 •

edited

Loading

bridgekeeper bot commented Jan 16, 2024

dhanalla commented Jan 16, 2024

jerboaa left a comment

jerboaa Jan 30, 2024

jerboaa Jan 30, 2024

jerboaa Jan 30, 2024

dhanalla commented Feb 2, 2024

dhanalla commented Feb 5, 2024

jerboaa commented Feb 5, 2024

openjdk-notifier bot commented Feb 8, 2024

openjdk bot commented Feb 8, 2024

theRealAph commented Feb 9, 2024

dhanalla commented Feb 14, 2024

openjdk bot commented Apr 8, 2024

jerboaa left a comment

dhanalla commented Apr 9, 2024

jerboaa left a comment

openjdk bot commented Apr 22, 2024

phohensee left a comment

dhanalla commented May 2, 2024

gnu-andrew commented May 14, 2024

gnu-andrew left a comment

gnu-andrew commented May 14, 2024

openjdk bot commented May 14, 2024

dhanalla commented May 14, 2024

dhanalla commented May 14, 2024

openjdk bot commented May 14, 2024

gnu-andrew commented May 14, 2024

openjdk bot commented May 14, 2024

openjdk bot commented May 14, 2024

8293562: KeepAliveCache Blocks Threads while Closing Connections #409

8293562: KeepAliveCache Blocks Threads while Closing Connections #409

Conversation

dhanalla commented Dec 18, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Dec 18, 2023

openjdk bot commented Dec 18, 2023

mlbridge bot commented Dec 18, 2023 • edited Loading

Webrevs

bridgekeeper bot commented Jan 16, 2024

dhanalla commented Jan 16, 2024

jerboaa left a comment

Choose a reason for hiding this comment

jerboaa Jan 30, 2024

Choose a reason for hiding this comment

jerboaa Jan 30, 2024

Choose a reason for hiding this comment

jerboaa Jan 30, 2024

Choose a reason for hiding this comment

dhanalla commented Feb 2, 2024

dhanalla commented Feb 5, 2024

jerboaa commented Feb 5, 2024

openjdk-notifier bot commented Feb 8, 2024

openjdk bot commented Feb 8, 2024

theRealAph commented Feb 9, 2024

dhanalla commented Feb 14, 2024

openjdk bot commented Apr 8, 2024

jerboaa left a comment

Choose a reason for hiding this comment

dhanalla commented Apr 9, 2024

jerboaa left a comment

Choose a reason for hiding this comment

openjdk bot commented Apr 22, 2024

phohensee left a comment

Choose a reason for hiding this comment

dhanalla commented May 2, 2024

gnu-andrew commented May 14, 2024

gnu-andrew left a comment

Choose a reason for hiding this comment

gnu-andrew commented May 14, 2024

openjdk bot commented May 14, 2024

dhanalla commented May 14, 2024

dhanalla commented May 14, 2024

openjdk bot commented May 14, 2024

gnu-andrew commented May 14, 2024

openjdk bot commented May 14, 2024

openjdk bot commented May 14, 2024

dhanalla commented Dec 18, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Dec 18, 2023 •

edited

Loading