8302732: sun/net/www/http/HttpClient/MultiThreadTest.java still failing intermittently #12676

djelinski · 2023-02-20T19:38:35Z

Please review this fix for a race in KeepAliveCache.

The sweeper thread (KeepAliveCache.run()) terminates when the cache is empty. The check for empty cache was performed without holding the cache lock. As a result, there was a small window of time where a new connection could be added to the cache while the sweeper thread was stopping. The added connection would then be removed from cache without closing when a new sweeper thread was started.

As an additional observation, the check for empty cache was performed after sweeping. The cache could be empty because all connections were closed, or because all connections were busy. In the latter case, a new thread was created soon after the old one terminated.

This patch addresses both these issues. The sweeper thread makes the decision to terminate while holding the lock, and other threads are aware of that when they acquire the lock. Also, the sweeper thread only terminates if the cache is empty AND there was no cache activity in the last LIFETIME (5 seconds).

Tier1-3 clean. No new tests, the issue was very hard to reproduce without adding delays in production code..

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-8302732: sun/net/www/http/HttpClient/MultiThreadTest.java still failing intermittently

Reviewers

Daniel Fuchs (@dfuch - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 12676

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

Using diff file

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

bridgekeeper · 2023-02-20T19:39:26Z

👋 Welcome back djelinski! 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 · 2023-02-20T19:40:56Z

@djelinski 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 · 2023-02-21T11:44:12Z

Webrevs

dfuch

Looks reasonable to me. Good analysis. But now that the cacheLock is used everywhere, do we still need the other lock in the ClientVector? It seems that all accesses to the client vector are already protected by the coarser cacheLock.

openjdk · 2023-02-21T14:53:20Z

@djelinski 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:

8302732: sun/net/www/http/HttpClient/MultiThreadTest.java still failing intermittently

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

58ca711: 8303078: Reduce allocations when pretty printing JCTree during compilation
2cf8b86: 8303071: Memory leaks in libjdwp
f113b04: 8302927: Unproblemlist Fuzz.java from ProblemList-zgc.txt again
1bab93b: 8303073: (bf) Temporarily reinstate private DirectByteBuffer(long, int) constructor
221f346: 8303047: avoid NULL after 8301661
23e9d9d: 8302811: NMT.random_reallocs_vm fails if NMT is off
fcaf871: 8302028: Port fdlibm atan2 to Java
07e976a: 8303081: Serial: Remove unused VM_MarkSweep
8de841d: 8302667: Improve message format when failing to load symbols or libraries
f893d23: 8303024: (fs) WindowsFileSystem.supportedFileAttributeViews can use Set.of
... and 55 more: https://git.openjdk.org/jdk/compare/9a797228f3576720196d5e3bf4b204a5e3f87376...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.

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

djelinski · 2023-02-23T08:29:23Z

You're right, all accesses to ClientVector use the cacheLock. I removed the vector lock.

dfuch · 2023-02-23T12:35:39Z

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

-                    KeepAliveCache.logger.finest(msg);
-                }
-                return e.hc;
+        // check the most recent connection, use if still valid


It might be prudent to assert that the global cacheLock is held by the current thread in get & put (if that's feasible without too much hackery). Just worrying about future maintainers possibly modifying this code and using ClientVector outside of the lock. The assert would make sure they ponder the consequences.

Asserts added; let me know if the amount of hackery involved looks acceptable.

Looks reasonable. I personally dislike having several top-level classes in the same file so I'd say having ClientVector as an inner class is a plus. Having ClientVector carry a (hidden) pointer to its parent instance shouldn't be an issue since ClientVector shouldn't escape. I'd say this is good, provided that the modified test still passes. Otherwise, we can replace the assert with a simple comment, to warn that the class is not thread-safe and should not be used outside of blocks protected by the cacheLock.

djelinski · 2023-02-27T07:50:12Z

/integrate

openjdk · 2023-02-27T07:52:10Z

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

db217c9: 8292583: Comment for ciArrayKlass::make is wrong
2613b94: 8302149: Speed up compiler/jsr292/methodHandleExceptions/TestAMEnotNPE.java
d2660a6: 8303045: Remove RegionNode::LoopStatus::NeverIrreducibleEntry assert with wrong assumption
1794f49: 8302681: [IR Framework] Only allow cpuFeatures from a verified list
a43931b: 8303102: jcmd: ManagementAgent.status truncates the text longer than O_BUFLEN
2fb1e3b: 8302268: Prefer ArrayList to LinkedList in XEmbeddedFramePeer
3a9f491: 8301964: Expensive fillInStackTrace operation in HttpURLConnection.getLastModified when no last-modified in response
1dbd18a: 8302120: Prefer ArrayList to LinkedList in AggregatePainter
2e78d83: 8302899: Executors.newSingleThreadExecutor can use Cleaner to shutdown executor
17e3769: 8302871: Speed up StringLatin1.regionMatchesCI
... and 79 more: https://git.openjdk.org/jdk/compare/9a797228f3576720196d5e3bf4b204a5e3f87376...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-02-27T07:52:32Z

@djelinski Pushed as commit a2c5a4a.

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

Fix possible race

3883285

openjdk bot changed the title ~~8302732~~ 8302732: sun/net/www/http/HttpClient/MultiThreadTest.java still failing intermittently Feb 20, 2023

openjdk bot added the net label Feb 20, 2023

djelinski added 2 commits February 21, 2023 09:54

Delay closing the sweeper thread

f6edc7b

Reword

de6f329

djelinski marked this pull request as ready for review February 21, 2023 11:38

openjdk bot added the rfr label Feb 21, 2023

dfuch approved these changes Feb 21, 2023

View reviewed changes

openjdk bot added the ready label Feb 21, 2023

Remove unnecessary lock

bb5596c

dfuch reviewed Feb 23, 2023

View reviewed changes

Add asserts

0337c28

dfuch approved these changes Feb 23, 2023

View reviewed changes

openjdk bot added the integrated label Feb 27, 2023

openjdk bot closed this Feb 27, 2023

openjdk bot removed ready rfr labels Feb 27, 2023

djelinski deleted the keepalive-race branch March 8, 2023 10:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8302732: sun/net/www/http/HttpClient/MultiThreadTest.java still failing intermittently #12676

8302732: sun/net/www/http/HttpClient/MultiThreadTest.java still failing intermittently #12676

djelinski commented Feb 20, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Feb 20, 2023

openjdk bot commented Feb 20, 2023

mlbridge bot commented Feb 21, 2023 •

edited

Loading

dfuch left a comment

openjdk bot commented Feb 21, 2023 •

edited

Loading

djelinski commented Feb 23, 2023

dfuch Feb 23, 2023

djelinski Feb 23, 2023

dfuch Feb 23, 2023

djelinski commented Feb 27, 2023

openjdk bot commented Feb 27, 2023

openjdk bot commented Feb 27, 2023

8302732: sun/net/www/http/HttpClient/MultiThreadTest.java still failing intermittently #12676

8302732: sun/net/www/http/HttpClient/MultiThreadTest.java still failing intermittently #12676

Conversation

djelinski commented Feb 20, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Feb 20, 2023

openjdk bot commented Feb 20, 2023

mlbridge bot commented Feb 21, 2023 • edited Loading

Webrevs

dfuch left a comment

Choose a reason for hiding this comment

openjdk bot commented Feb 21, 2023 • edited Loading

djelinski commented Feb 23, 2023

dfuch Feb 23, 2023

Choose a reason for hiding this comment

djelinski Feb 23, 2023

Choose a reason for hiding this comment

dfuch Feb 23, 2023

Choose a reason for hiding this comment

djelinski commented Feb 27, 2023

openjdk bot commented Feb 27, 2023

openjdk bot commented Feb 27, 2023

djelinski commented Feb 20, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Feb 21, 2023 •

edited

Loading

openjdk bot commented Feb 21, 2023 •

edited

Loading