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 |
|
@dfuch This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 7 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
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
djelinski
left a comment
djelinski
left a comment
There was a problem hiding this comment.
Doing this in a separate cleanup is fine by me. Note that one of the duplicates can be easily removed already.
| if (errors()) { | ||
| String s = Utils.stackTrace(t); | ||
| logger.log(Level.INFO, "ERROR: " + s); | ||
| logger.log(Level.INFO, "ERROR: " + t, t); |
There was a problem hiding this comment.
| logger.log(Level.INFO, "ERROR: " + t, t); | |
| logger.log(Level.INFO, "ERROR:", t); |
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
Reviewers
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