8326498: java.net.http.HttpClient connection leak using http/2 #28233

jaikiran · 2025-11-11T12:58:04Z

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.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8326498: java.net.http.HttpClient connection leak using http/2 (Bug - P3)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/28233/head:pull/28233
$ git checkout pull/28233

Update a local copy of the PR:
$ git checkout pull/28233
$ git pull https://git.openjdk.org/jdk.git pull/28233/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 28233

View PR using the GUI difftool:
$ git pr show -t 28233

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/28233.diff

Using Webrev

Link to Webrev Comment

…rrored or completed

bridgekeeper · 2025-11-11T12:59:13Z

👋 Welcome back jpai! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-11-11T12:59:51Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

openjdk · 2025-11-11T13:01:06Z

@jaikiran The following label will be automatically applied to this pull request:

net

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2025-11-11T13:05:08Z

Webrevs

01: Full - Incremental (3e4574b4)
00: Full (6c983026)

dfuch · 2025-11-11T13:17:18Z

src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java

+    /**
+     * Closes the connection normally (with a NO_ERROR termination cause), if not already closed.
+     */
+    final void close() {


Maybe we should mark the Http2Connection as Closeable.

I think that's reasonable. I've updated the PR to include this change.

djelinski · 2025-11-12T08:27:05Z

src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java

+        if (!isOpen()) {
+            final Http2TerminationCause tc = this.connTerminator.getTerminationCause();
+            assert tc != null : "termination cause is null for a closed connection";
+            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.

djelinski · 2025-11-12T08:28:46Z

src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java

+     * {@linkplain NetworkChannel#isOpen() channel is open}. false otherwise.
+     */
+    final boolean isOpen() {
+        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?

A channel can be closed asynchronously by the peer. So it may be closed even if close() has not been called.

djelinski · 2025-11-12T08:53:48Z

src/java.net.http/share/classes/jdk/internal/net/http/Http2TerminationCause.java

+     * is not considered as an erroneous termination and this method returns {@code false} for
+     * such cases.
+     */
+    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?

That that erroneous close has been used in several other contexts; in code, in comments, etc. If this gets updated, I'd appreciate other relevant occurrences get updated too.

vy

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.

vy · 2025-11-12T09:00:35Z

src/java.net.http/share/classes/jdk/internal/net/http/Exchange.java

-                                    if (!cached)
-                                        c.close();


We remove the if (!cached) c.close() logic. Where do we restore this functionality? If not, why not?

If the terminationException is not null the connection is already closed (or being closed by another thread) so there's no need to call close() again.

vy · 2025-11-12T09:08:43Z

src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java

-                    if (debug.on()) {
-                        debug.log("Unexpected state %s, skipping idle connection shutdown",
-                                describeClosedState(closedState));
+                        debug.log("Not initiating idle connection close");


Suggested change

debug.log("Not initiating idle connection close");

debug.log("Connection is already cancelled, skipping idle connection close");

vy · 2025-11-12T09:23:21Z

src/java.net.http/share/classes/jdk/internal/net/http/Http2TerminationCause.java

+     * being idle.
+     */
+    public static Http2TerminationCause idleTimedOut() {
+        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.

vy · 2025-11-12T09:23:56Z

src/java.net.http/share/classes/jdk/internal/net/http/Http2TerminationCause.java

+import jdk.internal.net.http.frame.ErrorFrame;
+
+/**
+ * Termination cause for a {@linkplain Http2Connection HTTP/2 connection}


Suggested change

* Termination cause for a {@linkplain Http2Connection HTTP/2 connection}

* Termination cause for an {@linkplain Http2Connection HTTP/2 connection}

vy · 2025-11-12T09:24:38Z

src/java.net.http/share/classes/jdk/internal/net/http/Http2TerminationCause.java

+     * Returns the IOException that is considered the cause of the connection termination.
+     * Even a normal {@linkplain #isErroneousClose() non-erroneous} termination will have
+     * a IOException associated with it, so this method will always return a non-null instance.


Suggested change

* Returns the IOException that is considered the cause of the connection termination.

* Even a normal {@linkplain #isErroneousClose() non-erroneous} termination will have

* a IOException associated with it, so this method will always return a non-null instance.

* 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.

vy · 2025-11-12T10:20:44Z

src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java

+                // if the underlying SocketChannel isn't open, then terminate the connection.
+                // that way when Http2Connection.isOpen() returns false in that situation, then this
+                // getTerminationCause() will return a termination cause.
+                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?

vy · 2025-11-12T10:47:30Z

test/jdk/java/net/httpclient/http2/BurstyRequestsTest.java

+
+    private static Field openConnections; // Set<> jdk.internal.net.http.HttpClientImpl#openedConnections
+
+    private static SSLContext sslContext;


Is SSL a necessity for this test? Put another way, does SSL play a role in the connection leakage?

vy · 2025-11-12T10:50:01Z

test/jdk/java/net/httpclient/http2/BurstyRequestsTest.java

+    }
+
+    /*
+     * Issues a burst of 100 HTTP/2 requests to the same server (host/port) and expects all of


numRequests is actually 20, not 100:

Suggested change

* Issues a burst of 100 HTTP/2 requests to the same server (host/port) and expects all of

* Issues a burst of HTTP/2 requests to the same server (host/port) and expects all of

vy · 2025-11-12T10:53:05Z

test/jdk/java/net/httpclient/http2/BurstyRequestsTest.java

+
+    // using reflection, return the jdk.internal.net.http.HttpClientImpl instance held
+    // by the given client
+    private static Object reflectHttpClientImplInstance(final HttpClient client) throws Exception {


Instead, you can use Http3ConnectionAccess::impl too.

vy · 2025-11-12T11:01:07Z

test/jdk/java/net/httpclient/http2/BurstyRequestsTest.java

+     * HttpClientImpl to verify that the client is holding on to at most 1 connection.
+     */
+    @Test
+    void testOpenConnections() throws Exception {


Shall we also introduce following tests?

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.)

Repeat all tests with a small idle timeout, say, 5s, and ensure that after 5s pool is completely emptied.

jaikiran added 6 commits November 11, 2025 17:08

8326498: java.net.http.HttpClient connection leak using http/2

abb9cb8

introduce a test to reproduce the leak and verify the fix

b8e2866

Daniel's review suggestion - stop the scheduler when TubeSubscriber e…

88c6bbc

…rrored or completed

Return false from isOpen() if underlying channel is not open

4cf52bb

update test

392f694

cleanup

6c98302

openjdk bot added the net net-dev@openjdk.org label Nov 11, 2025

openjdk bot added the rfr Pull request is ready for review label Nov 11, 2025

dfuch reviewed Nov 11, 2025

View reviewed changes

jaikiran added 2 commits November 11, 2025 19:37

reduce number of concurrent requests

1a70acc

mark jdk.internal.net.http.Http2Connection as Closable

3e4574b

djelinski reviewed Nov 12, 2025

View reviewed changes

vy reviewed Nov 12, 2025

View reviewed changes

	debug.log("Not initiating idle connection close");
	debug.log("Connection is already cancelled, skipping idle connection close");

	* Termination cause for a {@linkplain Http2Connection HTTP/2 connection}
	* Termination cause for an {@linkplain Http2Connection HTTP/2 connection}


		private static Field openConnections; // Set<> jdk.internal.net.http.HttpClientImpl#openedConnections

		private static SSLContext sslContext;

	* Issues a burst of 100 HTTP/2 requests to the same server (host/port) and expects all of
	* Issues a burst of HTTP/2 requests to the same server (host/port) and expects all of

8326498: java.net.http.HttpClient connection leak using http/2 #28233

Are you sure you want to change the base?

8326498: java.net.http.HttpClient connection leak using http/2 #28233

Conversation

jaikiran commented Nov 11, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewing

Uh oh!

bridgekeeper bot commented Nov 11, 2025

Uh oh!

openjdk bot commented Nov 11, 2025

Uh oh!

openjdk bot commented Nov 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlbridge bot commented Nov 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dfuch Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

jaikiran commented Nov 11, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Nov 11, 2025 •

edited

Loading

mlbridge bot commented Nov 11, 2025 •

edited

Loading

dfuch Nov 12, 2025 •

edited

Loading