On Tue, 11 Nov 2025 14:18:56 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 two additional > commits since the last revision: > > - mark jdk.internal.net.http.Http2Connection as Closable > - reduce number of concurrent requests src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java line 904: > 902: final Http2TerminationCause tc = > this.connTerminator.getTerminationCause(); > 903: assert tc != null : "termination cause is null for a closed > connection"; > 904: return Optional.of(tc.getCloseCause()); Please remove this code block. The comment seems to imply that it fixes the race, but it doesn't. I assume you verified that all callers of getTerminationException are properly synchronized, so that we don't leak resources if the method returns an empty optional while the connection is closed in another thread. src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java line 1247: > 1245: */ > 1246: final boolean isOpen() { > 1247: return this.connTerminator.terminationCause.get() == null && > connection.channel().isOpen(); Can we ever observe a situation where channel is not open but termination cause is not set? As far as I could tell, channel.isOpen only returns false after close() is called, and close() is only called from doTerminate after the termination cause is set. What am I missing? src/java.net.http/share/classes/jdk/internal/net/http/Http2TerminationCause.java line 84: > 82: * such cases. > 83: */ > 84: public abstract boolean isErroneousClose(); nit: can we use a different word here? "Erroneous close" feels vague here; would "is(Non)Graceful", "isAbrupt" or "hasErrorCode" capture the intent? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/28233#discussion_r2517366540 PR Review Comment: https://git.openjdk.org/jdk/pull/28233#discussion_r2517371220 PR Review Comment: https://git.openjdk.org/jdk/pull/28233#discussion_r2517443404
