On Mon, 17 Nov 2025 12:28:35 GMT, Jaikiran Pai <[email protected]> wrote:
>> Can I please get a review for this fix which addresses a connection leak in >> HttpClient when dealing with HTTP/2 requests? >> >> I have added a comment in https://bugs.openjdk.org/browse/JDK-8326498 which >> explains what the issue is. The fix here addresses the issue by cleaning up >> the `Http2Connection` closing logic and centralizing it to a connection >> terminator. The terminator then ensures that the right resources are closed >> (including the underlying SocketChannel) when the termination happens. >> >> A new jtreg test has been introduced which reproduces the issue and verifies >> the fix. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > fix typo in test code comment src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java line 2022: > 2020: if (debug.on()) { > 2021: debug.log("sending GOAWAY " + goAway); > 2022: } Suggestion: if (Log.trace()) { Log.logTrace("{0} sending GOAWAY {1}", connection, goAway); } else if (debug.on()) { debug.log("sending GOAWAY " + goAway); } src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java line 2083: > 2081: ", GOAWAY sent=" + goAwaySent.get(); > 2082: debug.log("Closing connection (" + stateStr + ") due > to: " + tc); > 2083: } The message logged for debug is sufficiently different from the one logged with Log that we should consider actually logging both without fearing the repetition. Either that, or consider logging the same message in both cases, in which case the else could stay. Suggestion: if (Log.errors()) { Log.logError("Closing connection due to: {0}", tc); } if (debug.on()) { final String stateStr = "Abnormal close=" + tc.isAbnormalClose() + ", has active streams=" + isActive() + ", GOAWAY received=" + goAwayRecvd.get() + ", GOAWAY sent=" + goAwaySent.get(); debug.log("Closing connection (" + stateStr + ") due to: " + tc); } ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/28233#discussion_r2534576009 PR Review Comment: https://git.openjdk.org/jdk/pull/28233#discussion_r2534589681
