On Mon, 24 Nov 2025 19:53:39 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 with a new target base due to a > merge or a rebase. The pull request now contains eight commits: > > - Merge branch 'master' into ConnectionCloseLock-8372198 > - Merge master > - 8372198: Avoid closing PlainHttpConnection while holding a lock > - Merge branch 'master' into SelectorManagerVT-8372159 > - Copyright years > - Review feedback on test > - Revert changes to SelectorManager::shutdown > - 8372159: HttpClient SelectorManager thread could be a VirtualThread src/java.net.http/share/classes/jdk/internal/net/http/ConnectionPool.java line 213: > 211: if (cleanup.isDone()) { > 212: return; > 213: } else if (stopping = stopped) { This is tricky to read. Should we change this to: else if (stopped) { stopping = true; .... } src/java.net.http/share/classes/jdk/internal/net/http/HttpClientImpl.java line 1356: > 1354: } finally { > 1355: // cleanup anything that might have been left behind > 1356: owner.stop(); Hello Daniel, since we already call `owner.stop()` unconditionally before the `try` block, is this second invocation of that method required? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/28430#discussion_r2558717975 PR Review Comment: https://git.openjdk.org/jdk/pull/28430#discussion_r2558712890
