8371471: HttpClient: Log HTTP/3 handshake failures if logging errors is enabled #28196
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
|
src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicConnectionImpl.java
Outdated
Show resolved
Hide resolved
| // this is a bit fragile and may need to be updated if the | ||
| // place where we log the exception from changes. | ||
| String expectedClassName = QuicConnectionImpl.class.getName() | ||
| + "$HandshakeFlow"; |
There was a problem hiding this comment.
Would just asserting for the (expected) log message and the presence of stacktrace be enough? Irrespective of which class logs it?
There was a problem hiding this comment.
Possibly. Let's keep that for now.
| final HttpRequest req1 = reqBuilder.copy().GET().build(); | ||
| System.out.println("Issuing request: " + req1); | ||
| final HttpResponse<Void> resp1 = client.send(req1, BodyHandlers.discarding()); | ||
| Assert.assertEquals(resp1.statusCode(), 200, "unexpected response code for GET request"); |
There was a problem hiding this comment.
Sorry I missed htis in the previous round of review. Shouldn't this line be a fail("Request expected to fail but didn't")?
There was a problem hiding this comment.
Oh sorry I meant to just remove this line. I thought it was better to not configure HTTP_3_URI_ONLY on the request in order to trigger the fallback on TCP. So something might actually respond. If something responds then we could get a response. On the other hand this might impacts other tests that might run on the machine...
hmmm... maybe I should fix that.
There was a problem hiding this comment.
The test now creates a TCP ServerSocket bound to the same port and which will close accepted connections. That should ensure safe fallback on TCP. If the ServerSocket cannot be created (address already in use) then the request will be configured with HTTP3_URI_ONLY to prevent fallback to TCP.
| if (Log.errors()) { | ||
| Log.logError("%s QUIC handshake failed: %s" | ||
| .formatted(logTag(), cause)); | ||
| Log.logError(cause); |
There was a problem hiding this comment.
This produces 2 error messages like:
Nov 07, 2025 6:01:57 PM jdk.internal.net.http.quic.QuicConnectionImpl$HandshakeFlow failHandshakeCFs
INFO: ERROR: QuicServerConnection(1) QUIC handshake failed: java.io.IOException: Connection closed by client peer: CRYPTO_ERROR|certificate_unknown certificate_unknown
Nov 07, 2025 6:01:57 PM jdk.internal.net.http.quic.QuicConnectionImpl$HandshakeFlow failHandshakeCFs
INFO: ERROR: java.io.IOException: Connection closed by client peer: CRYPTO_ERROR|certificate_unknown certificate_unknown
java.io.IOException: Connection closed by client peer: CRYPTO_ERROR|certificate_unknown certificate_unknown
at java.net.http/jdk.internal.net.http.quic.TerminationCause.toReportedCause(TerminationCause.java:129)
Can we remove the duplicate information? Currently there are 2 timestamps, and the exception message is printed 3 times.
There was a problem hiding this comment.
I have considered this. This would require adding a new logError metod to Log:
public void logError(String msg, Throwable throwable) {
....
}
I refrained from doing that because there may be place where we call logError with a message and a throwable and don't expect to see the stack trace. That would require examining all such call sites and cast the Throwable to Object - or add a call to toString() - so that they keep on calling logError(String, Object...).
I'd rather do that in a separate cleanup.
FWIW - what you are seeing in the quoted log are a mix of client traces and server traces: the message on the server talks of the client peer
3 participants
Surprisingly if the QUIC handshake fails exceptionally, and
-Djdk.httpclient.HttpClient.log=errorsis specified, the exception is not logged. This change fixes that.Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/28196/head:pull/28196$ git checkout pull/28196Update a local copy of the PR:
$ git checkout pull/28196$ git pull https://git.openjdk.org/jdk.git pull/28196/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 28196View PR using the GUI difftool:
$ git pr show -t 28196Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/28196.diff
Using Webrev
Link to Webrev Comment