Skip to content

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

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

Kehrlann wants to merge 15 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>
…opped logs

Signed-off-by: Dariusz Jędrzejczyk <dariusz.jedrzejczyk@broadcom.com>
@chemicL
chemicL force-pushed the dgarnier/issue-620-refactor-http-client-chains branch from eb4009f to 3a8745b Compare September 30, 2026 08:42
Comment on lines +219 to +240
return Mono.<HttpResponse<Publisher<List<ByteBuffer>>>>create(sink -> {
CompletableFuture<HttpResponse<Publisher<List<ByteBuffer>>>> exchange = httpClient.sendAsync(request,
HttpResponse.BodyHandlers.ofPublisher());
sink.onCancel(() -> exchange.cancel(true));
exchange.whenComplete((response, error) -> {
if (error == null) {
// Emit the response so the body can be consumed.
// If the surrounding Mono was cancelled though and due to a race
// the headers were already parsed, the below call will simply
// discard the response.
sink.success(response);
return;
}
Throwable cause = error instanceof CompletionException && error.getCause() != null ? error.getCause()
: error;
if (cause instanceof CancellationException) {
sink.success();
}
else {
sink.error(cause);
}
});

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the Mono.create dance combined with sink.onCancel, combined with exchange.whenComplete is caused by an issue with reactor-core's handling of HttpClient's CancellationException wrapping inside a CompletionException: reactor/reactor-core#4415

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 P1 Significant bug affecting many users, highly requested feature

Projects

None yet

2 participants