You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Fixes the Java 17 HttpClient-* SelectorManager thread retention reported in #547 and #620 by releasing transport-owned HttpClient references on close instead of forcing internal client shutdown.
This change:
preserves the current public builder API
keeps Java 17 compatibility
avoids reflection, Unsafe, and JDK-internal shutdown hooks
keeps closeGracefully() non-blocking
adds deterministic Docker-free leak reproducer tests for both SSE and streamable HTTP transports
This is an alternative to the approaches explored in #610 and #868.
Motivation and Context
The root problem was that HttpClientSseClientTransport and HttpClientStreamableHttpTransport retained a strong reference to their internally created HttpClient even after transport close.
If user code retained closed transport objects, those internal HttpClient instances also remained reachable, which in turn kept HttpClient-* SelectorManager threads alive.
Instead of trying to forcibly shut down JDK HttpClient internals, this PR treats the issue as an ownership problem:
transports own the internally created HttpClient
once the transport is closed, that owned reference is released
after that, the client becomes eligible for normal JVM cleanup
Additionally, DefaultMcpTransportSession.closeGracefully() now always disposes tracked connections even if onClose fails, which makes close-path cleanup reliable.
How Has This Been Tested?
Locally verified with targeted mcp-core regression tests:
DefaultMcpTransportSessionTests
HttpClientSseClientTransportLeakTests
HttpClientStreamableHttpTransportLeakTests
These tests:
repeatedly create and close transports
intentionally keep closed transport objects reachable
assert that HttpClient-* SelectorManager threads return to baseline after GC stabilization
Also verified with existing Docker-backed integration tests:
HttpClientSseClientTransportTests
HttpClientStreamableHttpTransportTest
Breaking Changes
None.
Types of changes
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation update
Checklist
I have read the [MCP Documentation (https://modelcontextprotocol.io/)
My code follows the repository's style guidelines
New and existing tests pass locally
I have added appropriate error handling
I have added or updated documentation as needed
Additional context
Implementation notes:
Added an internal OwnedHttpClient abstraction to track transport-owned clients.
HttpClientSseClientTransport and HttpClientStreamableHttpTransport now release their owned client reference on close.
DefaultMcpTransportSession.closeGracefully() now disposes tracked connections even when onClose errors.
The fix intentionally avoids reflection-based shutdown of JDK HttpClient internals.
We hit this one too. Our workaround is to pass one shared HttpClient into both transports' builders, since each build() allocates its own and closeGracefully() never lets go of it. With a client per (user, backend) pair the selector threads pile up: one of our gateways went from roughly 700 to 19,000 JVM threads over a week and then crash-restarted.
One thing to check here. On the streamable path, releaseAfterClose() runs after sessionClose, with onErrorResume for the failure case. If the session close hangs rather than fails (#547, where dispose() is gated on the session-termination DELETE with no bound), the release never runs either. On the path that actually leaked for us, this fix needs that one as well.
One more thing for the PR description: at Java 17 there is no HttpClient.close(), so dropping the reference is the portable option and the release is GC-timed. That bounds the growth rather than removing it, which is still better than the current behaviour, but it is worth being explicit about for anyone sizing a per-session client workload.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
area/clientbugSomething isn't workingP2Moderate issues affecting some users, edge cases, potentially valuable featureready for workThe goal is clear and work towards it can be commenced
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the Java 17
HttpClient-* SelectorManagerthread retention reported in#547and#620by releasing transport-ownedHttpClientreferences on close instead of forcing internal client shutdown.This change:
Unsafe, and JDK-internal shutdown hookscloseGracefully()non-blockingThis is an alternative to the approaches explored in
#610and#868.Motivation and Context
The root problem was that
HttpClientSseClientTransportandHttpClientStreamableHttpTransportretained a strong reference to their internally createdHttpClienteven after transport close.If user code retained closed transport objects, those internal
HttpClientinstances also remained reachable, which in turn keptHttpClient-* SelectorManagerthreads alive.Instead of trying to forcibly shut down JDK
HttpClientinternals, this PR treats the issue as an ownership problem:HttpClientAdditionally,
DefaultMcpTransportSession.closeGracefully()now always disposes tracked connections even ifonClosefails, which makes close-path cleanup reliable.How Has This Been Tested?
Locally verified with targeted
mcp-coreregression tests:DefaultMcpTransportSessionTestsHttpClientSseClientTransportLeakTestsHttpClientStreamableHttpTransportLeakTestsThese tests:
HttpClient-* SelectorManagerthreads return to baseline after GC stabilizationAlso verified with existing Docker-backed integration tests:
HttpClientSseClientTransportTestsHttpClientStreamableHttpTransportTestBreaking Changes
None.
Types of changes
Checklist
Additional context
Implementation notes:
OwnedHttpClientabstraction to track transport-owned clients.HttpClientSseClientTransportandHttpClientStreamableHttpTransportnow release their owned client reference on close.DefaultMcpTransportSession.closeGracefully()now disposes tracked connections even whenonCloseerrors.HttpClientinternals.