Skip to content

fix: release transport-owned HttpClient references on close#904

Open
TsengX wants to merge 1 commit intomodelcontextprotocol:mainfrom
TsengX:issue-620-selector-manager-leak
Open

fix: release transport-owned HttpClient references on close#904
TsengX wants to merge 1 commit intomodelcontextprotocol:mainfrom
TsengX:issue-620-selector-manager-leak

Conversation

@TsengX
Copy link
Copy Markdown

@TsengX TsengX commented Apr 6, 2026

Summary

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant