8372159: HttpClient SelectorManager thread could be a VirtualThread #28395
Conversation
|
👋 Welcome back dfuchs! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
Webrevs
|
| static final long IDLE_CONNECTION_TIMEOUT_H3 = getTimeoutProp("jdk.httpclient.keepalive.timeout.h3", IDLE_CONNECTION_TIMEOUT_H2); | ||
|
|
||
| static final UseVTForSelector USE_VT_FOR_SELECTOR = | ||
| Utils.useVTForSelector("jdk.internal.httpclient.tcp.selector.useVirtualThreads", "default"); |
There was a problem hiding this comment.
Hello Daniel, I vaguely remember we discussed whether or not to use a value called "default" for this property. But I don't remember what we decided. Looking at this now, I am wondering whether we should just do something like:
System.getProperty("jdk.internal.httpclient.tcp.selector.useVirtualThreads", "true")to keep it simple?
There was a problem hiding this comment.
For tcp we only need true/false - but for quic we have a default behavior which depends on the platform. I decided to keep "always/never/default" for both properties for consistency. Let me think about it. I am not sure if that change (true/false for quic too) should be made in this PR.
| if (sslContext == null) { | ||
| throw new AssertionError("Unexpected null sslContext"); | ||
| } | ||
| // create an H3 only server |
There was a problem hiding this comment.
The use of "H3" here looks like a typo.
| // GET | ||
| final HttpRequest req1 = reqBuilder.copy().GET().build(); | ||
| System.out.println("\nIssuing request: " + req1); | ||
| final HttpResponse<?> resp1 = client.send(req1, BodyHandlers.ofString()); |
There was a problem hiding this comment.
Nit - this line and the other 3 send() lines could perhaps use BodyHandlers.discarding()? If you prefer to use ofString(), that's fine too - it wasn't clear to me if this was an oversight.
There was a problem hiding this comment.
If you look at the log you will see that there is an exception being logged by Log "errors" at the end of each method. The exception is "selector manager closed". At first I thought it was caused by using BodyHandlers.discarding() - which I suspected might return a result early and discard data in the background.
This is why I changed all to ofString(). But the exception is still there.
The exception is logged because the selector is closed before all connections have been closed.
Trying to fix that I stumbled on an another issue - JDK-8372198 - so I decided to revert my attempted fix dd4acad and fix that later in a follow up. But I kept ofString.
2 participants
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.
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.
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/28395/head:pull/28395$ git checkout pull/28395Update a local copy of the PR:
$ git checkout pull/28395$ git pull https://git.openjdk.org/jdk.git pull/28395/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 28395View PR using the GUI difftool:
$ git pr show -t 28395Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/28395.diff
Using Webrev
Link to Webrev Comment