8373409: java/net/httpclient/http3/H3ErrorHandlingTest.java failed due to deadlock by djelinski · Pull Request #28788 · openjdk/jdk

djelinski · 2025-12-12T12:31:07Z

This PR fixes a deadlock between the localConnectionIdManager and the connections map by closing the manager before calling connections.compute.

No new tests; the issue requires a complex setup to reproduce, and the new code is easy enough to reason about. Existing tests 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-8373409: java/net/httpclient/http3/H3ErrorHandlingTest.java failed due to deadlock (Bug - P3)

Reviewers

Daniel Fuchs (@dfuch - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 28788

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-12-12T12:32:10Z

👋 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 · 2025-12-12T12:32:35Z

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

8373409: java/net/httpclient/http3/H3ErrorHandlingTest.java failed due to deadlock

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

87d881f: 8368493: Disable most test JSSE debug output by default, and increase the test default maximum output log size
30be940: 8373625: CPUTimeCounters creates a total counter for unsupported GCs
2241218: 8373631: Improve classes in the "jdk.jpackage.internal.util.function" package
... and 82 more: https://git.openjdk.org/jdk/compare/b60ac710bebf195972436da324983e61b51484ef...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.

openjdk · 2025-12-12T12:33:30Z

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

djelinski · 2025-12-12T12:37:10Z

I checked all other uses of CHM.compute in the java.net.http module; most of them are trivially correct. The ones in AltServiceRegistry are not trivial, and might need to be replaced with something easier to reason about. I'll look into that.

EDIT - checked the AltServiceRegistry; compute is only used on a (non-concurrent) HashMap while holding a lock, and the lambdas only use functions internal to the registry. This should be fine as well.

mlbridge · 2025-12-12T12:42:01Z

Webrevs

01: Full - Incremental (84f5e430)
00: Full (d379634d)

dfuch · 2025-12-15T09:10:21Z

src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicEndpoint.java

+        DrainingConnection draining = new DrainingConnection(connection.connectionIds(), idleTimeout);
+        // we can ignore stateless reset in the draining state.
+        remapPeerIssuedResetToken(connection, draining);
+        draining.startTimer();


shouldn't we start the timer only if the connection has been added, and therefore call startTimer in compute?

I'd rather not call this from compute; every method called from the compute lambda increases the risk of reintroducing the deadlock, so I'd like to keep the lambda to a minimum.

Most of the time the connection will be added; the only case where it won't is when there are multiple threads attempting to close the connection in parallel. The timer task only removes the connection from the endpoint, so, worst case, we will remove the connection IDs and the reset tokens twice. The second removal will likely be a no-op, unless we somehow manage to reassign the IDs or the tokens to a different connection.

A look at startTimer lets me think that it should be safe, but OK. I suspect the double removal would not be an issue - IIRC we check with == on the removed connection, but we would be potentially adding an event to the timer queue which will wake up the timer queue for nothing. We could add a boolean field to ClosingConnection that we could set to true if the connection is added and check that after compute has been called, and start the timer then. I'll let you decide if it's worth it.

We don't check with ==; technically we could, but with multiple threads accessing the map, I'm not sure if we would guarantee that all connection IDs are unmapped when the connection is removed.

Multiple threads racing to update the connection map should be rare. Most of the time the compute calls will replace the connection. so the extra check is probably not worth the effort.

I'll move the startTimer call after the connection map updates; I observed occasional failures to update the map, because the timer fired before the map was updated.

dfuch · 2025-12-15T09:12:54Z

src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicEndpoint.java

+        connection.localConnectionIdManager().close();
+        var closingConnection = new ClosingConnection(connection.connectionIds(), idleTimeout, datagram);
+        remapPeerIssuedResetToken(connection, closingConnection);
+        closingConnection.startTimer();


Same question here.

dfuch · 2025-12-15T09:34:44Z

I checked all other uses of CHM.compute in the java.net.http module; most of them are trivially correct. The ones in AltServiceRegistry are not trivial, and might need to be replaced with something easier to reason about. I'll look into that.

EDIT - checked the AltServiceRegistry; compute is only used on a (non-concurrent) HashMap while holding a lock, and the lambdas only use functions internal to the registry. This should be fine as well.

Thanks for checking the other uses of CHM.compute @djelinski !

dfuch

LGTM

djelinski · 2025-12-17T07:48:12Z

Thanks @dfuch for the review!

/integrate

openjdk · 2025-12-17T07:50:02Z

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

94c51ce: 8372635: Lambdas do not copy over SYNTHETIC flag for local variables
e9b4696: 8373097: Save command should create missing parent directories
e635330: 8373069: RISC-V: implement GHASH intrinsic
... and 86 more: https://git.openjdk.org/jdk/compare/b60ac710bebf195972436da324983e61b51484ef...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-12-17T07:50:10Z

@djelinski Pushed as commit 386ad61.

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

djelinski added 3 commits December 12, 2025 11:49

Avoid calling methods in QUIC package from CHM.compute

7cfb3e5

Add bug ID

2cab3ff

Reorder QuicConnectionImpl check first

d379634

openjdk bot changed the title ~~8373409~~ 8373409: java/net/httpclient/http3/H3ErrorHandlingTest.java failed due to deadlock Dec 12, 2025

openjdk bot added the net net-dev@openjdk.org label Dec 12, 2025

djelinski marked this pull request as ready for review December 12, 2025 12:37

openjdk bot added the rfr Pull request is ready for review label Dec 12, 2025

dfuch reviewed Dec 15, 2025

View reviewed changes

Reorder startTimer after map updates

84f5e43

dfuch approved these changes Dec 16, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 16, 2025

openjdk bot added the integrated Pull request has been integrated label Dec 17, 2025

openjdk bot closed this Dec 17, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 17, 2025

djelinski deleted the h3-deadlock branch December 17, 2025 08:03

Conversation

djelinski commented Dec 12, 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 Dec 12, 2025

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

djelinski commented Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Webrevs

Uh oh!

dfuch Dec 15, 2025

Choose a reason for hiding this comment

Uh oh!

djelinski Dec 15, 2025

Choose a reason for hiding this comment

Uh oh!

dfuch Dec 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

djelinski Dec 16, 2025

Choose a reason for hiding this comment

Uh oh!

dfuch Dec 15, 2025

Choose a reason for hiding this comment

Uh oh!

dfuch commented Dec 15, 2025

Uh oh!

dfuch left a comment

Choose a reason for hiding this comment

Uh oh!

djelinski commented Dec 17, 2025

Uh oh!

openjdk bot commented Dec 17, 2025

Uh oh!

openjdk bot commented Dec 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

djelinski commented Dec 12, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Dec 12, 2025 •

edited

Loading

openjdk bot commented Dec 12, 2025 •

edited

Loading

djelinski commented Dec 12, 2025 •

edited

Loading

mlbridge bot commented Dec 12, 2025 •

edited

Loading

dfuch Dec 15, 2025 •

edited

Loading