On Wed, 15 Oct 2025 15:58:23 GMT, Daniel Fuchs <[email protected]> wrote:
> This change makes it possible to use a VirtualThread for the > QuicSelectorThread. The default will be to use a VirtualThread on all > platforms except the Windows platform. We might consider switching the > default for Windows platform too once > [JDK-8334574](https://bugs.openjdk.org/browse/JDK-8334574) is fixed. > > The change should be transparent for users of the API. However, using a > VirtualThread may result in subtle differences in thread scheduling and class > loading, so this change also includes an escape hatch (a non documented > internal system property) that could be used to revert to a platform thread > in case of unexpected issues. That property may be removed in a future > version of the JDK. src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicEndpoint.java line 148: > 146: MAX_BUFFERED_LOW = maxBufferLow; > 147: String useVtForSelector = > 148: > System.getProperty("jdk.internal.httpclient.quic.useVTForSelector", > "default"); We already have a `jdk.internal.httpclient.quic.poller.usePlatformThreads`. To follow the same naming convention, you might consider renaming this to `j.i.h.q.selector.useVirtualThreads`. src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicSelector.java line 510: > 508: } > 509: > 510: private record QuicSelectorThread(Thread thread) { AFAICS, all methods – `start()`, `join()`, `ofPlatform()`, `ofVirtual()`, and `of()` – can be `private`. test/jdk/java/net/httpclient/http3/H3QuicVTTest.java line 2: > 1: /* > 2: * Copyright (c) 2023, 2025, Oracle and/or its affiliates. All rights > reserved. Suggestion: * Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved. test/jdk/java/net/httpclient/http3/H3QuicVTTest.java line 54: > 52: * @bug 8369920 > 53: * @summary Basic test to verify that the QuicSelector > 54: * uses a VirtualThread Suggestion: * @summary Verifies whether `QuicSelector` uses virtual threads when no explicit configuration is provided test/jdk/java/net/httpclient/http3/H3QuicVTTest.java line 57: > 55: * @library /test/lib /test/jdk/java/net/httpclient/lib > 56: * @build jdk.test.lib.net.SimpleSSLContext > 57: * jdk.httpclient.test.lib.common.HttpServerAdapters @sormuras, in this JUnit test, given these `@build` classes are already included in `@library`, do we still need this explicit `@build`? test/jdk/java/net/httpclient/http3/H3QuicVTTest.java line 66: > 64: * @bug 8369920 > 65: * @summary Basic test to verify that the QuicSelector > 66: * uses a VirtualThread This summary contradicts with this particular `@test`. That is, this `@test` verifies `QuicSelector` does *not* use a virtual thread. While I agree that `@test id=...` hints about the semantics, having an accurate `@summary` would not hurt: Suggestion: * @summary Verifies that `QuicSelector` does *not* use virtual threads when explicitly configured so test/jdk/java/net/httpclient/http3/H3QuicVTTest.java line 79: > 77: * @bug 8369920 > 78: * @summary Basic test to verify that the QuicSelector > 79: * uses a VirtualThread Suggestion: * @summary Verifies that `QuicSelector` does *always* use virtual threads when explicitly configured so test/jdk/java/net/httpclient/http3/H3QuicVTTest.java line 92: > 90: * @bug 8369920 > 91: * @summary Basic test to verify that the QuicSelector > 92: * uses a VirtualThread Suggestion: * @summary Verifies whether `QuicSelector` uses virtual threads when `default` is explicitly configured test/jdk/java/net/httpclient/http3/H3QuicVTTest.java line 105: > 103: * @bug 8369920 > 104: * @summary Basic test to verify that the QuicSelector > 105: * uses a VirtualThread Suggestion: * @summary Verifies whether `QuicSelector` uses virtual threads when it is configured using an invalid value test/jdk/java/net/httpclient/http3/H3QuicVTTest.java line 115: > 113: */ > 114: // -Djava.security.debug=all > 115: public class H3QuicVTTest implements HttpServerAdapters { Suggestion: class H3QuicVTTest implements HttpServerAdapters { test/jdk/java/net/httpclient/http3/H3QuicVTTest.java line 132: > 130: } > 131: > 132: static boolean isQuicSelectorThreadVirtual() { Suggestion: private static boolean isQuicSelectorThreadVirtual() { test/jdk/java/net/httpclient/http3/H3QuicVTTest.java line 141: > 139: > 140: @BeforeAll > 141: public static void beforeClass() throws Exception { Suggestion: static void beforeClass() throws Exception { test/jdk/java/net/httpclient/http3/H3QuicVTTest.java line 155: > 153: > 154: @AfterAll > 155: public static void afterClass() throws Exception { Suggestion: static void afterClass() throws Exception { test/jdk/java/net/httpclient/http3/H3QuicVTTest.java line 166: > 164: */ > 165: @Test > 166: public void testBasicRequests() throws Exception { Suggestion: void testBasicRequests() throws Exception { test/jdk/java/net/httpclient/http3/H3QuicVTTest.java line 199: > 197: } > 198: > 199: void assertSelectorThread(HttpClient client) { Suggestion: private static void assertSelectorThread(HttpClient client) { test/jdk/java/net/httpclient/http3/H3QuicVTTest.java line 200: > 198: > 199: void assertSelectorThread(HttpClient client) { > 200: Objects.requireNonNull(client); I doubt if this is needed. Suggestion: test/jdk/java/net/httpclient/http3/H3QuicVTTest.java line 206: > 204: .map(Thread::getName) > 205: .toList()); > 206: boolean found = threads.contains(name); Using thread name for this check looks very fragile. Can we devise a more programmatic method? E.g., ensuring no `StackTraceElement` contains `QuicSelectorThread`? test/jdk/java/net/httpclient/http3/H3QuicVTTest.java line 224: > 222: System.out.printf("%s: %s%n", status, msg); > 223: } > 224: Assertions.assertEquals(!isQuicSelectorThreadVirtual(), > threads.contains(name), msg); Suggestion: Assertions.assertEquals(!isQuicSelectorThreadVirtual(), found, msg); ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/27827#discussion_r2487864987 PR Review Comment: https://git.openjdk.org/jdk/pull/27827#discussion_r2487758171 PR Review Comment: https://git.openjdk.org/jdk/pull/27827#discussion_r2487761972 PR Review Comment: https://git.openjdk.org/jdk/pull/27827#discussion_r2487818322 PR Review Comment: https://git.openjdk.org/jdk/pull/27827#discussion_r2487846396 PR Review Comment: https://git.openjdk.org/jdk/pull/27827#discussion_r2487823188 PR Review Comment: https://git.openjdk.org/jdk/pull/27827#discussion_r2487831510 PR Review Comment: https://git.openjdk.org/jdk/pull/27827#discussion_r2487834314 PR Review Comment: https://git.openjdk.org/jdk/pull/27827#discussion_r2487835902 PR Review Comment: https://git.openjdk.org/jdk/pull/27827#discussion_r2487846842 PR Review Comment: https://git.openjdk.org/jdk/pull/27827#discussion_r2487848843 PR Review Comment: https://git.openjdk.org/jdk/pull/27827#discussion_r2487847519 PR Review Comment: https://git.openjdk.org/jdk/pull/27827#discussion_r2487847819 PR Review Comment: https://git.openjdk.org/jdk/pull/27827#discussion_r2487847208 PR Review Comment: https://git.openjdk.org/jdk/pull/27827#discussion_r2487849223 PR Review Comment: https://git.openjdk.org/jdk/pull/27827#discussion_r2487850445 PR Review Comment: https://git.openjdk.org/jdk/pull/27827#discussion_r2487801799 PR Review Comment: https://git.openjdk.org/jdk/pull/27827#discussion_r2487764764
