On Thu, 27 Nov 2025 11:46:35 GMT, Volkan Yazici <[email protected]> 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
