On Wed, 26 Nov 2025 17:46:40 GMT, Daniel Fuchs <[email protected]> wrote:
>> An experimental change to SelectorManager::shutdown unveiled a potential >> deadlock between the SelectorManager thread trying to stop the >> HttpClientImpl, and an executor thread concurrently trying to return a >> connection to the pool. >> >> The issue seems to be caused by the ConnectionPool::returnToPool trying to >> close the returned connection when stopping, while holding the >> ConnectionPool state lock, and the SelectorManager thread trying to close a >> pooled connection, holding the connection stateLock and trying to close the >> channel, which caused the CleanupTrigger to fire and attempt to remove the >> connection from the pool. >> >> This problem was observed once with the >> java/net/httpclient/ThrowingSubscribersAsLimitingAsync.java test. >> >> To avoid the problem, the proposed fix is to wait until the >> ConnectionPool::stateLock has been released before actually closing the >> connection, and to wait until the PlainHttpConnection::stateLock has been >> released before actually closing the channel. Indeed, there should be no >> need to close those while holding the lock. >> >> This PR was recreated due to a bad merge pushed to >> https://github.com/openjdk/jdk/pull/28421 > > Daniel Fuchs has updated the pull request incrementally with one additional > commit since the last revision: > > test update: remove left over from debug session @dfuch, thanks so much for the test. It will certainly help with unintentionally introducing a similar unexpected behavior in the future. I've dropped some minor remarks and questions, but I see no blockers – LGTM. You can decide to address my comments as you wish. test/jdk/java/net/httpclient/PlainConnectionLockTest.java line 124: > 122: response.acquire(); > 123: return true; > 124: } catch (InterruptedException x) { _Nit:_ While it might not make much of difference in this particular case, I think it is always a good practice to not discard `InterruptedException` – this has recently been [discussed on core-libs-dev]. Suggestion: } catch (InterruptedException x) { Thread.currentThread().interrupt(); // Restore the interrupt [discussed on core-libs-dev]: https://mail.openjdk.org/pipermail/core-libs-dev/2025-November/154551.html test/jdk/java/net/httpclient/PlainConnectionLockTest.java line 131: > 129: > 130: @BeforeEach > 131: synchronized void beforeTest() throws Exception { Do we really need `synchronized` here? test/jdk/java/net/httpclient/PlainConnectionLockTest.java line 138: > 136: if (sslContext == null) { > 137: throw new AssertionError("Unexpected null sslContext"); > 138: } _Nit:_ `sslContext` can be a class (i.e., `static`) field, and reused. test/jdk/java/net/httpclient/PlainConnectionLockTest.java line 143: > 141: > 142: // On windows, sending 100 concurrent requests may > 143: // fail is the server's connection backlog is less than 100. Suggestion: // On Windows, sending 100 concurrent requests may // fail if the server's connection backlog is less than 100. test/jdk/java/net/httpclient/PlainConnectionLockTest.java line 146: > 144: // The default backlog is 50. Just make sure the backlog is > 145: // big enough. > 146: int backlog = MANY > 50 ? MANY : 50; _Nit:_ A simpler alternative: Suggestion: int backlog = Math.max(MANY, 50); test/jdk/java/net/httpclient/PlainConnectionLockTest.java line 159: > 157: exchange.sendResponseHeaders(500, 0); > 158: } > 159: }, "/PlainConnectionLockTest/"); Both servers have identical server handlers. You can consider extracting it into a variable, and reusing it both in `https1Server::addHandler` and `http1Server::addHandler`. test/jdk/java/net/httpclient/PlainConnectionLockTest.java line 165: > 163: > 164: // create a plain http server for HTTP/1.1 > 165: var wrappedHttp1Server = HttpServer.create(new > InetSocketAddress(InetAddress.getLoopbackAddress(), 0), backlog); Suggestion: var wrappedHttp1Server = HttpServer.create(new InetSocketAddress(loopback, 0), backlog); test/jdk/java/net/httpclient/PlainConnectionLockTest.java line 214: > 212: } catch (Throwable t) { > 213: t.printStackTrace(System.out); > 214: throw t; I see a longing for TestNG, where stack traces were dumped into stdout, and stderr is populated with more verbose logging. 😜 test/jdk/java/net/httpclient/PlainConnectionLockTest.java line 272: > 270: } > 271: > 272: private synchronized void sendManyRequests(final String requestURI, > final int many, boolean shutdown) throws Exception { Is `synchronized` necessary here? test/jdk/java/net/httpclient/PlainConnectionLockTest.java line 272: > 270: } > 271: > 272: private synchronized void sendManyRequests(final String requestURI, > final int many, boolean shutdown) throws Exception { When `shutdown = false`, we expect all request-response exchange to succeed with 200. In the context of the tested behaviour, which case does `shutdown = false` stress? test/jdk/java/net/httpclient/PlainConnectionLockTest.java line 350: > 348: // wait for all responses > 349: System.out.printf("%sWaiting for all %s responses to > complete%n", now(), many); > 350: CompletableFuture.allOf(futures.toArray(new > CompletableFuture[0])).exceptionally(t -> null).join(); Is `exceptionally(t -> null)` a refactoring leftover? ------------- Marked as reviewed by vyazici (Committer). PR Review: https://git.openjdk.org/jdk/pull/28430#pullrequestreview-3514427710 PR Review Comment: https://git.openjdk.org/jdk/pull/28430#discussion_r2567926507 PR Review Comment: https://git.openjdk.org/jdk/pull/28430#discussion_r2567914626 PR Review Comment: https://git.openjdk.org/jdk/pull/28430#discussion_r2568061462 PR Review Comment: https://git.openjdk.org/jdk/pull/28430#discussion_r2567928870 PR Review Comment: https://git.openjdk.org/jdk/pull/28430#discussion_r2568028255 PR Review Comment: https://git.openjdk.org/jdk/pull/28430#discussion_r2568051609 PR Review Comment: https://git.openjdk.org/jdk/pull/28430#discussion_r2568045225 PR Review Comment: https://git.openjdk.org/jdk/pull/28430#discussion_r2568268591 PR Review Comment: https://git.openjdk.org/jdk/pull/28430#discussion_r2568275803 PR Review Comment: https://git.openjdk.org/jdk/pull/28430#discussion_r2568394233 PR Review Comment: https://git.openjdk.org/jdk/pull/28430#discussion_r2568377890
