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 I personally liked the clean-up of the state changes in `Http2Connection`, yet the change is non-trivial. I leave the judgement of that to reviewers more versed in `Http2Connection`. src/java.net.http/share/classes/jdk/internal/net/http/Exchange.java line 715: > 713: if (t != null) { > 714: if (!cached) > 715: c.close(); We remove the `if (!cached) c.close()` logic. Where do we restore this functionality? If not, why not? src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java line 234: > 232: if (cancelled) { > 233: if (debug.on()) { > 234: debug.log("Not initiating idle connection > close"); Suggestion: debug.log("Connection is already cancelled, skipping idle connection close"); src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java line 889: > 887: * If the connection hasn't yet been terminated then this method > returns an empty Optional. > 888: */ > 889: final Optional<IOException> getTerminationException() { There is one place this method is used and it does `getTerminationException().orElse(null)`. I guess we can drop the `Optional` ceremony. src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java line 2056: > 2054: private void doTerminate() { > 2055: final Http2TerminationCause tc = terminationCause.get(); > 2056: assert tc != null : "missing termination cause"; `doTerminate()` can receive the termination cause from `terminate()`, which would render these two lines redundant. src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java line 2071: > 2069: peerVisibleReason.getBytes(UTF_8)); > 2070: } > 2071: sendGoAway(goAway); Do we need to take `sendGoAway()` failures into account? src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java line 2076: > 2074: Log.logError("Closing connection due to: {0}", tc); > 2075: } else { > 2076: if (debug.on()) { You can consider simplifying this as `else if (debug.on())`. src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java line 2134: > 2132: // that way when Http2Connection.isOpen() returns false > in that situation, then this > 2133: // getTerminationCause() will return a termination > cause. > 2134: terminate(Http2TerminationCause.forException(new > IOException("channel is not open"))); Terminating the connection in a getter doesn't feel all right. Would you mind elaborating on the rationale, the absence of a better alternative, please? src/java.net.http/share/classes/jdk/internal/net/http/Http2TerminationCause.java line 35: > 33: > 34: /** > 35: * Termination cause for a {@linkplain Http2Connection HTTP/2 connection} Suggestion: * Termination cause for an {@linkplain Http2Connection HTTP/2 connection} src/java.net.http/share/classes/jdk/internal/net/http/Http2TerminationCause.java line 71: > 69: * Returns the IOException that is considered the cause of the > connection termination. > 70: * Even a normal {@linkplain #isErroneousClose() non-erroneous} > termination will have > 71: * a IOException associated with it, so this method will always > return a non-null instance. Suggestion: * Returns the {@link IOException} that is considered the cause of the connection termination. * Even a normal {@linkplain #isErroneousClose() non-erroneous} termination will have * an {@code IOException} associated with it, so this method will always return a non-null instance. src/java.net.http/share/classes/jdk/internal/net/http/Http2TerminationCause.java line 144: > 142: */ > 143: public static Http2TerminationCause idleTimedOut() { > 144: return new NoError("HTTP/2 connection idle timed out", "idle > timed out"); Any particular reason this is not cached in `NoError.IDLE_TIMED_OUT` in a similar manner to `NoError.INSTANCE`? I could not see a place where its stack trace would be of value. src/java.net.http/share/classes/jdk/internal/net/http/Http2TerminationCause.java line 176: > 174: } else { > 175: return new IOException(original); > 176: } I presume we don't need to peel off any `CompletionException` and/or `ExecutionException`, right? test/jdk/java/net/httpclient/http2/BurstyRequestsTest.java line 102: > 100: private static Field openConnections; // Set<> > jdk.internal.net.http.HttpClientImpl#openedConnections > 101: > 102: private static SSLContext sslContext; Is SSL a necessity for this test? Put another way, does SSL play a role in the connection leakage? test/jdk/java/net/httpclient/http2/BurstyRequestsTest.java line 128: > 126: > 127: /* > 128: * Issues a burst of 100 HTTP/2 requests to the same server > (host/port) and expects all of `numRequests` is actually 20, not 100: Suggestion: * Issues a burst of HTTP/2 requests to the same server (host/port) and expects all of test/jdk/java/net/httpclient/http2/BurstyRequestsTest.java line 134: > 132: */ > 133: @Test > 134: void testOpenConnections() throws Exception { Shall we also introduce following tests? 1. Verify secondary request bursts always reuse the pooled connection, and don't change the state of the pool. (Remember that, as reported in the associated ticket, the secondary bursts were causing the orphan connection pile up.) 2. Repeat all tests with a small idle timeout, say, 5s, and ensure that after 5s pool is completely emptied. test/jdk/java/net/httpclient/http2/BurstyRequestsTest.java line 195: > 193: // using reflection, return the jdk.internal.net.http.HttpClientImpl > instance held > 194: // by the given client > 195: private static Object reflectHttpClientImplInstance(final HttpClient > client) throws Exception { Instead, you can use `Http3ConnectionAccess::impl` too. ------------- PR Review: https://git.openjdk.org/jdk/pull/28233#pullrequestreview-3452322562 PR Review Comment: https://git.openjdk.org/jdk/pull/28233#discussion_r2517465442 PR Review Comment: https://git.openjdk.org/jdk/pull/28233#discussion_r2517499729 PR Review Comment: https://git.openjdk.org/jdk/pull/28233#discussion_r2517719632 PR Review Comment: https://git.openjdk.org/jdk/pull/28233#discussion_r2517572919 PR Review Comment: https://git.openjdk.org/jdk/pull/28233#discussion_r2517726876 PR Review Comment: https://git.openjdk.org/jdk/pull/28233#discussion_r2517606995 PR Review Comment: https://git.openjdk.org/jdk/pull/28233#discussion_r2517739033 PR Review Comment: https://git.openjdk.org/jdk/pull/28233#discussion_r2517552232 PR Review Comment: https://git.openjdk.org/jdk/pull/28233#discussion_r2517554830 PR Review Comment: https://git.openjdk.org/jdk/pull/28233#discussion_r2517550218 PR Review Comment: https://git.openjdk.org/jdk/pull/28233#discussion_r2517584896 PR Review Comment: https://git.openjdk.org/jdk/pull/28233#discussion_r2517824929 PR Review Comment: https://git.openjdk.org/jdk/pull/28233#discussion_r2517833332 PR Review Comment: https://git.openjdk.org/jdk/pull/28233#discussion_r2517866370 PR Review Comment: https://git.openjdk.org/jdk/pull/28233#discussion_r2517842337
