Skip to content

Refactor HttpClient-based transports to Publisher-based approach - #1079

Open
Kehrlann wants to merge 13 commits into
mainfrom
dgarnier/issue-620-refactor-http-client-chains
Open

Kehrlann wants to merge 13 commits into
mainfrom
dgarnier/issue-620-refactor-http-client-chains

Conversation

@Kehrlann

@Kehrlann Kehrlann commented Aug 7, 2026 •

Copy link
Copy Markdown
Contributor

Refactor HttpClient use to avoid capturing the sseSink that leads to leaking the httpClient reference due to a cycle (see #620).
This rewrites the HttpClient streaming support from a BodySubscriber pushing messages to a Publisher-based approach.

This avoids the HttpClient cyclic reference we had in the previous implementation. The tradeoff is that we have to perform line parsing manually with a Utf8LineDecoder we maintain.
We can batch this rework with performance fixes reading large SSE events (see #1042)

Fixes #1042
Fixes #620

(Rebased @chemicL 's work on issue #620)

@Kehrlann Kehrlann added this to the 2.1.0 Planning milestone Aug 7, 2026
@Kehrlann Kehrlann self-assigned this Aug 7, 2026
@Kehrlann Kehrlann added area/client P1 Significant bug affecting many users, highly requested feature area/transport do not merge labels Aug 7, 2026
@Kehrlann
Kehrlann force-pushed the dgarnier/issue-620-refactor-http-client-chains branch from 16fee90 to 30dc4d3 Compare August 7, 2026 16:14
@Kehrlann
Kehrlann force-pushed the dgarnier/issue-620-refactor-http-client-chains branch from 30dc4d3 to bf47603 Compare August 27, 2026 11:49
@Kehrlann
Kehrlann marked this pull request as ready for review August 31, 2026 15:11
@Kehrlann
Kehrlann force-pushed the dgarnier/issue-620-refactor-http-client-chains branch from b897c12 to bbb3330 Compare August 31, 2026 15:11
@Kehrlann Kehrlann changed the title [WIP] Refactor HttpClient use to avoid leaking references Refactor HttpClient-based transports Aug 31, 2026
@Kehrlann Kehrlann changed the title Refactor HttpClient-based transports Refactor HttpClient-based transports to Publisher-based approach Aug 31, 2026
@Kehrlann
Kehrlann requested a review from chemicL September 3, 2026 12:42
chemicL and others added 7 commits September 18, 2026 11:44
…leaking the httpClient reference due to a cycle

Signed-off-by: Daniel Garnier-Moiroux <git@garnier.wf>
Signed-off-by: Dariusz Jędrzejczyk <2554306+chemicL@users.noreply.github.com>
Fixes #1042

Signed-off-by: Daniel Garnier-Moiroux <git@garnier.wf>
Signed-off-by: Daniel Garnier-Moiroux <git@garnier.wf>
Signed-off-by: Daniel Garnier-Moiroux <git@garnier.wf>
Signed-off-by: Daniel Garnier-Moiroux <git@garnier.wf>
Signed-off-by: Daniel Garnier-Moiroux <git@garnier.wf>
- This discards all BodySubscribers to align with
  a6c88fccf50277ddf705765154baae4caa906e15, which introduced a
  publisher-based architecture instead of using subscribers.
- Bounding the reads now happens within the reactive chains, e.g. in
  Utf8LineDecoder, decodeAggregateResponse and drain* methods.

Signed-off-by: Daniel Garnier-Moiroux <git@garnier.wf>
@Kehrlann
Kehrlann force-pushed the dgarnier/issue-620-refactor-http-client-chains branch from 9df07a5 to f8271c2 Compare September 23, 2026 12:14
Also renamed ResponseSubscribers, refactored test code

Signed-off-by: Dariusz Jędrzejczyk <dariusz.jedrzejczyk@broadcom.com>
@chemicL
chemicL force-pushed the dgarnier/issue-620-refactor-http-client-chains branch from 81a3dc9 to 6a01d9b Compare September 29, 2026 13:23
The SSE parser now follows the SSE spec: an event's type resets after
each event and defaults to "message", and an empty id clears the last
event id. Before, the type carried over from the previous event, so a
message following an "endpoint" event was taken for another endpoint.

Behaviour changes compared to main:

- Errors caused by the server (non-2xx responses, unknown content
  types, malformed messages) are McpTransportException instead of
  plain RuntimeException. Error messages no longer include the
  response body.
- A 400 on the streamable GET stream no longer invalidates the session.
- A non-2xx SSE connect with an empty body now fails instead of
  hanging, and sendMessage() completes when the server closes an SSE
  response without sending any events.
- Errors that happen after connect() or sendMessage() has completed
  now reach the exception handler and stop showing up as Reactor
  onErrorDropped logs.

Signed-off-by: Dariusz Jędrzejczyk <dariusz.jedrzejczyk@broadcom.com>
…nt session id

Signed-off-by: Dariusz Jędrzejczyk <dariusz.jedrzejczyk@broadcom.com>
Signed-off-by: Dariusz Jędrzejczyk <dariusz.jedrzejczyk@broadcom.com>
Signed-off-by: Dariusz Jędrzejczyk <dariusz.jedrzejczyk@broadcom.com>
@chemicL

chemicL commented Sep 29, 2026 •

Copy link
Copy Markdown
Member

I re-visited this PR @Kehrlann with some commits. Thank you for refreshing it and integrating it with the recent changes!

Here's a summary of what is addressed here altogether:

Reliability

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. Fixes Streamable HTTP: invalid JSON response is reported as a timeout #1147; partially addresses Propagate body-level errors as synthetic JSON-RPC responses #896.
  • 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.

Performance

SSE parsing (now follows the spec)

Memory safety

  • Discarded response bodies are now capped at maxResponseSize in total. Before, only each line was capped, so a server could keep the client reading a never-ending discarded body. The per-message limit on SSE events and JSON responses is unchanged.

API

  • No public API changes.

Signed-off-by: Dariusz Jędrzejczyk <dariusz.jedrzejczyk@broadcom.com>

This branch has not been deployed

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

Labels

area/client area/transport do not merge P1 Significant bug affecting many users, highly requested feature

Projects

None yet

2 participants