On Mon, 24 Nov 2025 20:14:02 GMT, Volkan Yazici <[email protected]> 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 <[email protected]> > > 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
