On Mon, 24 Nov 2025 20:14:02 GMT, Volkan Yazici <vyazici at openjdk.org> wrote:
>> Daniel Fuchs has updated the pull request incrementally with one additional commit since the last revision:
>>
>> Update test/jdk/java/net/httpclient/http3/H3QuicVTTest.java
>>
>> Co-authored-by: Daniel Jelinski <djelinski1 at gmail.com>
>
> I had started my review in the morning, polished it in the evening, submitted, and then noticed it was already integrated. 😅 None of my remarks were a blocker. @dfuch, feel free to interpret them as you wish.
Sorry @vy don't know what happened here: did you comment after I integrated the PR or was there a glitch with github/bots? I only saw your comments today.
> test/jdk/java/net/httpclient/http2/H2SelectorVTTest.java line 55:
>
>> 53: * jdk.httpclient.test.lib.common.HttpServerAdapters
>> 54: * @run junit/othervm
>> 55: * -Djdk.httpclient.HttpClient.log=requests,responses,headers,errors
>
> @dfuch, double-checking: do we really want to keep `-Djdk.httpclient.HttpClient.log=...` configuration? They generally generate excessive output and consequently get truncated. Judging from my experience, we tend to increase logging verbosity of tests if there are intermittent failures, otherwise, if they pass, they better pass quietly? In short, I am not against adding them, but I prefer it to be a conscious decision, not a copy-paste leftover. 😅
It's the debug logs that are verbose.
`-Djdk.httpclient.HttpClient.log=requests,responses,headers,errors` should not flood the output unless you make 100's of requests.
> test/jdk/java/net/httpclient/http2/H2SelectorVTTest.java line 108:
>
>> 106: * -Djdk.internal.httpclient.tcp.selector.useVirtualThreads=garbage
>> 107: * -Djdk.httpclient.HttpClient.log=requests,responses,headers,errors
>> 108: * H2SelectorVTTest
>
> Suggestion:
>
> * ${test.main.class}
Good point. Maybe in a followup.
> test/jdk/java/net/httpclient/http2/H2SelectorVTTest.java line 113:
>
>> 111: class H2SelectorVTTest implements HttpServerAdapters {
>> 112:
>> 113: private static SSLContext sslContext;
>
> 1. Is SSL necessary for this test?
> 2. Do we need two tests: with and without SSL?
One is enough - and using SSL makes it simpler (avoids the upgrade and ensure we're fully HTTP/2)
> test/jdk/java/net/httpclient/http2/H2SelectorVTTest.java line 147:
>
>> 145: h2Server.start();
>> 146: System.out.println("Server started at " + h2Server.getAddress());
>> 147: requestURI = "https://" + h2Server.serverAuthority() + "/hello";
>
> I suggest salting the path (to avoid unintentionally getting responded by a foreign server) and suffixing the request URI with `/x` (to guard against [JDK-8272758]):
>
> Suggestion:
>
> h2Server.addHandler((exchange) -> exchange.sendResponseHeaders(200, 0), "/H2SelectorVTTest");
> h2Server.start();
> System.out.println("Server started at " + h2Server.getAddress());
> requestURI = "https://" + h2Server.serverAuthority() + "/H2SelectorVTTest/x";
>
>
> [JDK-8272758]: https://bugs.openjdk.org/browse/JDK-8272758
Could do that in a followup
> test/jdk/java/net/httpclient/http2/H2SelectorVTTest.java line 201:
>
>> 199: // selector thread is a platform thread. Otherwise, it assumes
>> 200: // that the thread is virtual.
>> 201: private static void assertSelectorThread(HttpClient client) {
>
> Since `SelectorManager` has been converted to a `Runnable`, would iterating through stack traces of existing platform threads and checking if any `StackTraceElement` matches `SelectorManager::run` qualify a more accurate check?
Interesting idea. Would avoid the dependency on the thread name.
-------------
PR Comment: https://git.openjdk.org/jdk/pull/28395#issuecomment-3575057133
PR Review Comment: https://git.openjdk.org/jdk/pull/28395#discussion_r2559556843
PR Review Comment: https://git.openjdk.org/jdk/pull/28395#discussion_r2559557966
PR Review Comment: https://git.openjdk.org/jdk/pull/28395#discussion_r2559553629
PR Review Comment: https://git.openjdk.org/jdk/pull/28395#discussion_r2559551392
PR Review Comment: https://git.openjdk.org/jdk/pull/28395#discussion_r2559549151