On Mon, 24 Nov 2025 12:13:14 GMT, Daniel Fuchs <[email protected]> wrote:
>> Each HttpClient instance creates an additional platform thread for its >> SelectorManager. With recent updates to NIO/VirtualThreads that thread could >> now become a VirtualThread. This would avoid having each HttpClient instance >> use up one platform thread. >> This is similar to what was done for the HttpClient QuicSelectorThread in >> [JDK-8369920](https://bugs.openjdk.org/browse/JDK-8369920). >> This should be transparent for users of the API. >> An undocumented internal system property is introduced that can revert the >> change in case of unforeseen trouble. > > 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. src/java.net.http/share/classes/jdk/internal/net/http/HttpClientImpl.java line 1332: > 1330: String getName() { > 1331: return owner.selmgrThread.getName(); > 1332: } Where is this used? 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. 😅 test/jdk/java/net/httpclient/http2/H2SelectorVTTest.java line 56: > 54: * @run junit/othervm > 55: * > -Djdk.httpclient.HttpClient.log=requests,responses,headers,errors > 56: * H2SelectorVTTest Suggestion: * ${test.main.class} test/jdk/java/net/httpclient/http2/H2SelectorVTTest.java line 69: > 67: * > -Djdk.internal.httpclient.tcp.selector.useVirtualThreads=never > 68: * > -Djdk.httpclient.HttpClient.log=requests,responses,headers,errors > 69: * H2SelectorVTTest Suggestion: * ${test.main.class} test/jdk/java/net/httpclient/http2/H2SelectorVTTest.java line 82: > 80: * > -Djdk.internal.httpclient.tcp.selector.useVirtualThreads=always > 81: * > -Djdk.httpclient.HttpClient.log=requests,responses,headers,errors > 82: * H2SelectorVTTest Suggestion: * ${test.main.class} test/jdk/java/net/httpclient/http2/H2SelectorVTTest.java line 95: > 93: * > -Djdk.internal.httpclient.tcp.selector.useVirtualThreads=default > 94: * > -Djdk.httpclient.HttpClient.log=requests,responses,headers,errors > 95: * H2SelectorVTTest Suggestion: * ${test.main.class} 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} 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? 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 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? ------------- PR Comment: https://git.openjdk.org/jdk/pull/28395#issuecomment-3572548313 PR Review Comment: https://git.openjdk.org/jdk/pull/28395#discussion_r2556118575 PR Review Comment: https://git.openjdk.org/jdk/pull/28395#discussion_r2556146525 PR Review Comment: https://git.openjdk.org/jdk/pull/28395#discussion_r2556131588 PR Review Comment: https://git.openjdk.org/jdk/pull/28395#discussion_r2556134313 PR Review Comment: https://git.openjdk.org/jdk/pull/28395#discussion_r2556135012 PR Review Comment: https://git.openjdk.org/jdk/pull/28395#discussion_r2556136192 PR Review Comment: https://git.openjdk.org/jdk/pull/28395#discussion_r2556136716 PR Review Comment: https://git.openjdk.org/jdk/pull/28395#discussion_r2556153878 PR Review Comment: https://git.openjdk.org/jdk/pull/28395#discussion_r2557546712 PR Review Comment: https://git.openjdk.org/jdk/pull/28395#discussion_r2557594027
