RFR: 8372198: Avoid closing PlainHttpConnection while holding a lock [v4]

[Jaikiran Pai]

On Mon, 24 Nov 2025 19:53:39 GMT, Daniel Fuchs <dfuchs at openjdk.org> 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