Fix Streamable HTTP: surface invalid JSON response as transport error - #1151
NewPeople-star wants to merge 4 commits into
Conversation
In HttpClientStreamableHttpTransport.sendMessage, the application/json branch completed the delivery sink before deserializing the payload. When the server returned a body that is not valid JSON, the parse failure only travelled through the response Flux; the delivery sink had already completed, so McpClientSession.sendRequest never received the error, never removed its pending response entry, and the caller waited for the full request timeout and only saw a TimeoutException. The original parsing exception was missing from the terminal chain. Complete the sink only after the payload has been parsed successfully. Notifications keep completing before their early return since they have no response to parse. Fixes modelcontextprotocol#1147
…criber (modelcontextprotocol#1079) HttpClient-based transports used to capture the enclosing sseSink in the subscriber, leading to HttpClient leaks when the client was closed. This PR addresses this, and adds many other improvements to HttpClient-based transports. This has no public API change. Improved transport reliability: - Closed transports no longer keep their `HttpClient` alive, so selector threads and memory stop piling up - `closeGracefully()` now releases open connections even when the session DELETE fails, e.g. when the server is down - `connect()` on the legacy SSE transport no longer hangs when it gets the stream ends (error, stream closed,`closeGracefully()`, ...) before the first event or - `sendMessage()` on Streamable HTTP no longer hangs when the SSE stream is closed without response or before the response arrives - Responses the client never reads are always released (e.g. `DELETE`), so connections go back to the pool. Errors surface immediately instead of as timeouts: - On Streamable HTTP, a JSON response that can't be read (malformed, or over maxResponseSize) now fails the request immediately with the real cause, instead of a TimeoutException after requestTimeout` - A server that answers a request with an empty JSON body now makes that request fail instead of silently timing out. An empty body in reply to a notification is still tolerated. - Server-caused errors are now McpTransportException instead of a plain RuntimeException, and the message includes the response body the server sent. - Errors that happen after connect() or sendMessage() has already completed now reach the transport's exception handler instead of Reactor "onErrorDropped" logs. - A 404 or 400 invalidates the session only if the request that got it carried a session id. Fixes a race condition where are reconnect got a new session while another request was already in flight (with the old session). The old request could have ended up invalidating the new session. Performance - Large SSE responses, such as multi-MB tool results, are no longer slow to receive. SSE parsing spec compliance - Unknown fields such as retry: are ignored instead of failing the stream with "Invalid SSE response". - The event type resets after each event, so a message that follows a named event is no longer misclassified and dropped. - A data: line containing U+2028, U+2029 or U+0085 is no longer truncated. - The legacy SSE transport skips empty "primer" events and unknown event types instead of failing. - An empty id: clears the last event id. Fixes modelcontextprotocol#547 Fixes modelcontextprotocol#620 Fixes modelcontextprotocol#1042 Fixes modelcontextprotocol#1047 Fixes modelcontextprotocol#1147 Signed-off-by: Daniel Garnier-Moiroux <git@garnier.wf> Signed-off-by: Dariusz Jędrzejczyk <dariusz.jedrzejczyk@broadcom.com>
|
One protocol-fidelity issue looks worth fixing in the new SSE parser before merge.
A safer shape would be:
A focused regression with an id like This is especially relevant here because this PR centralizes/replaces the SSE parsing path, so it is a good point to preserve exact event-stream semantics rather than carry the older trimming behavior forward. AI-assisted review; I checked the current PR head and the SSE parsing rules before posting. |
Problem
HttpClientStreamableHttpTransport.sendMessagecompleted the delivery sink beforedeserializing the
application/jsonpayload. When the server returned a body thatis not valid JSON, the parse failure only travelled through the response Flux; the
sink had already completed, so
McpClientSession.sendRequestnever received theerror, kept its pending response entry, and the caller waited for the full request
timeout and only saw a
TimeoutException. The original parsing exception wasmissing from the terminal chain.
Change
Complete the sink only after the payload has been parsed successfully. Notifications
keep completing before their early return since they have no response to parse.
Test
New
HttpClientStreamableHttpTransportInvalidJsonResponseTestserves an invalidJSON body over a real socket and asserts
sendMessageterminates withMcpTransportExceptioninstead of completing (fails on main withexpected: onError(); actual: onComplete()).Fixes #1147