fix(client): drain POST response bodies so legacy streamable HTTP reuses connections - #3284
Open
guillaume-flambard wants to merge 1 commit into
Conversation
…ses connections
In streamable-HTTP legacy mode every JSON-RPC exchange owns its POST
response stream. The client called `response.aclose()` the moment the
reply SSE event arrived, abandoning the body unread; httpx cannot return
an undrained streaming response's TCP connection to its pool, so each
exchange opened a fresh connection (plus a TCP handshake + TLS + slow
start) even against the same host.
Drain the body to EOF instead:
- SSE request responses are drained via the raw stream, because aiter_raw
raises StreamConsumed once the EventSource has started iterating;
- 202 and notification POST bodies (empty) drain instantly via aread().
Observed with the SDK's own server + a tracking transport:
before: initialize, notifications/initialized, tools/list, DELETE each
on its own connection
after: all four share one connection (the GET resumption stream is the
only separate one)
Fixes modelcontextprotocol#3281
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/mcp/client/streamable_http.py">
<violation number="1" location="src/mcp/client/streamable_http.py:340">
P1: A server that keeps an SSE response stream open after sending the final JSON-RPC reply can now leave this POST task blocked indefinitely, because draining waits for EOF even though stream termination is only a protocol SHOULD. This is particularly problematic for long-lived response streams and can retain the connection/task until a timeout or client shutdown. Consider retaining the previous close-on-completion behavior for streams without a known finite body, and only drain responses whose framing guarantees EOF is immediately available.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| "server answered a request with 202 Accepted", | ||
| code=INVALID_REQUEST, | ||
| ) | ||
| await self._drain_response(response) |
There was a problem hiding this comment.
P1: A server that keeps an SSE response stream open after sending the final JSON-RPC reply can now leave this POST task blocked indefinitely, because draining waits for EOF even though stream termination is only a protocol SHOULD. This is particularly problematic for long-lived response streams and can retain the connection/task until a timeout or client shutdown. Consider retaining the previous close-on-completion behavior for streams without a known finite body, and only drain responses whose framing guarantees EOF is immediately available.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/mcp/client/streamable_http.py, line 340:
<comment>A server that keeps an SSE response stream open after sending the final JSON-RPC reply can now leave this POST task blocked indefinitely, because draining waits for EOF even though stream termination is only a protocol SHOULD. This is particularly problematic for long-lived response streams and can retain the connection/task until a timeout or client shutdown. Consider retaining the previous close-on-completion behavior for streams without a known finite body, and only drain responses whose framing guarantees EOF is immediately available.</comment>
<file context>
@@ -337,6 +337,7 @@ async def _handle_post_request(self, ctx: RequestContext) -> None:
"server answered a request with 202 Accepted",
code=INVALID_REQUEST,
)
+ await self._drain_response(response)
return
</file context>
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 #3281
Problem
In streamable-HTTP legacy mode, every JSON-RPC exchange owns its POST response stream. The client called
await response.aclose()the moment the reply SSE event arrived (_handle_sse_response), abandoning the body unread. httpx cannot return an undrained streaming response's TCP connection to its pool, so each exchange opened a fresh TCP connection — handshake + TLS + slow start — even against the same host. A notification POST (202, empty body) and the DELETE teardown hit the same path and discarded their connections too.The issue's own reproduction shows a plain httpx control reusing 1 connection for the same three exchanges where the SDK used one per exchange.
Fix
Add
_drain_response()and use it on the three POST paths that previously discarded their connection:_handle_sse_response,is_complete): drain the body to EOF.aiter_rawraisesStreamConsumedonce theEventSourcehas started iterating, so the remaining bytes are drained via the rawresponse.stream.await response.aread()(empty body, instant).await response.aread().Verification
Real server (the SDK's own
streamable_http_app()) + a tracking transport that recordsnetwork_streamidentity per request:New regression test
test_legacy_mode_reuses_tcp_connections_across_exchangesassertsdistinct connections < exchanges; it fails without the fix and passes with it.tests/clientsuite: 711 passed, 1 skipped, 1 xfailed. Ruff + format clean.