RFR: 8326498: java.net.http.HttpClient connection leak using http/2 [v2]
Volkan Yazici
vyazici at openjdk.org
Wed Nov 12 11:22:28 UTC 2025
On Tue, 11 Nov 2025 14:18:56 GMT, Jaikiran Pai <jpai at openjdk.org> 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
More information about the net-dev
mailing list