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

[Daniel Fuchs]

On Thu, 27 Nov 2025 11:46:35 GMT, Volkan Yazici <vyazici at openjdk.org> wrote:

>> Daniel Fuchs has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   test update: remove left over from debug session
>
> 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. 😜

For some reason those stack traces did not show up. Now I believe it was due to not calling `client.shutDownNow()` (a bug in a previous version of the test) in the loop that tries to acquire the permits, when an `AssertionError` is thrown there. In that case since the client was not shutdown the try-with-resource would wait on request to complete which would not happen since the semaphore the server was waiting on was not released, and the requests whose response was waiting on this semaphore where not cancelled. Another reason why I doubted everything ;-). Printing those exceptions to system.out avoid having to look for them in System.err where they could be lost in the noise of server side exceptions throwns when requests are cancelled by `client.shutdownNow`

> 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?

See reply above

> 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?

close while return to pool after succeful 200

> 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?

No - we  don't want to fail here when shutdown==true, we want to check all the cfs first, we will fail later while checking them if necessary.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/28430#discussion_r2568526083
PR Review Comment: https://git.openjdk.org/jdk/pull/28430#discussion_r2568527599
PR Review Comment: https://git.openjdk.org/jdk/pull/28430#discussion_r2568540656
PR Review Comment: https://git.openjdk.org/jdk/pull/28430#discussion_r2568535133