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

Thu Nov 27 12:23:56 UTC 2025

On Wed, 26 Nov 2025 17:46:40 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 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