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 I see the `noreg-hard` label. An approach that you might consider exploring: 1. Configure `jdk.httpclient.connectionPoolSize` to something really low (e.g., 1) to force `returnToPool()` to close excessive connections 2. Configure an `HttpTestServer` that blocks all `HttpTestExchange::sendResponseHeaders` behind a latch 3. Fire 100 `sendAsync().thenCompose(responseFutures::add)`, where `responseFutures` is an unbounded `BlockingQueue<HttpResponse>` 4. Release the `sendResponseHeaders` latch 5. Verify the success of the very first response, i.e., `responseFutures.enqueue()` 6. Shutdown the client without waiting for the rest of the responses to arrive 7. Wrap the test method in a `@RepeatedTest(50)` ------------- PR Comment: https://git.openjdk.org/jdk/pull/28430#issuecomment-3574843187
