Dispose the POST response in SseClientSessionTransport to stop leaking a connection per message - #1841
Open
yalcinfu22 wants to merge 3 commits into
Open
Conversation
SendMessageAsync sends every message with HttpCompletionOption.ResponseHeadersRead (via McpHttpClient) but never disposed the returned HttpResponseMessage on the success path, so the underlying connection was never returned to the pool. Each JSON-RPC POST therefore opened and stranded a new connection, which was only reclaimed by GC or an idle timeout. Disposing the response returns the connection to the pool deterministically; subsequent POSTs reuse a single connection. Fixes modelcontextprotocol#1840 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
SendMessageAsync_Disposes_Response_On_Success drives the SSE transport through the public HttpClientTransport ConnectAsync + SendMessageAsync path against the existing MockHttpHandler. The mocked POST response carries content that records its own disposal, and the test asserts the response has been disposed by the time SendMessageAsync returns - no sockets, no GC, no timing dependence. Fails on the parent commit (response never disposed on the success path), passes with the fix, on net10.0, net9.0, net8.0, and net472. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Author
|
Per CONTRIBUTING.md's "tests are included for new features or bug fixes": added a deterministic regression test, |
HttpResponseMessage and HttpContent have no finalizer, so cleanup never happens implicitly via GC finalization; only an explicit Dispose() releases the connection. Reworded the comment and assert message to say cleanup becomes nondeterministic instead of claiming GC finalizes the response.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes #1840
What
One-word change in
SseClientSessionTransport.SendMessageAsync: theHttpResponseMessagereturned for each POSTed JSON-RPC message is now wrapped in
using. A deterministicregression test is included.
Why
McpHttpClient.SendAsyncsends every request withHttpCompletionOption.ResponseHeadersRead,so the underlying connection stays checked out until the response is consumed or disposed.
The POST path never did either on the success path (a
202 Acceptedwhose body is unused),so every sent message stranded one ESTABLISHED connection — never returned to the pool,
never reused, reclaimed only by GC or an idle timeout. Measurements and the full analysis
are in #1840 (per client: 1 live SSE connection + one stranded connection per POST —
initialize,notifications/initialized,tools/list).The surrounding code already does this correctly: the SSE GET response is wrapped in
using var responsein the same file, and the failure path reads the body before throwing.Only the success path was missing the dispose.
Regression test
HttpClientTransportTests.SendMessageAsync_Disposes_Response_On_Successdrives the SSEtransport through the public
HttpClientTransport.ConnectAsync+SendMessageAsyncpathagainst the existing
MockHttpHandler. The mocked POST response carries content thatrecords its own disposal, and the test asserts the response has been disposed by the time
SendMessageAsyncreturns. No sockets, no GC, no timing dependence: neitherHttpResponseMessagenorHttpContenthas a finalizer, so nothing but the transport'sexplicit dispose can set the flag.
SseClientSessionTransport.cs): fails on everytarget framework — net10.0, net9.0, net8.0, and net472.
HttpClientTransportTestsclass is 17/17 on each TFM.ModelContextProtocol.Testssuite: green locally on net10.0 (2357 passed,0 failed, 6 skipped — the tests requiring external credentials or Docker).
ModelContextProtocol.AspNetCore.Tests(which exercises the SSE client transportagainst a real in-memory Kestrel server) is also green locally on net10.0, net9.0,
and net8.0 (615 passed, 0 failed, 30 skipped each).
Notes
HttpRequestExceptionis fullymaterialized from
StatusCode/ReasonPhrase/body before theusingscope unwinds; theexception holds no reference to the response.
TreatWarningsAsErrorsenabled.🤖 Generated with Claude Code