Skip to content

refactor: add a hook to SSE client to allow changing the target#526

Open
beekld wants to merge 15 commits intomainfrom
beeklimt/SDK-2126-1
Open

refactor: add a hook to SSE client to allow changing the target#526
beekld wants to merge 15 commits intomainfrom
beeklimt/SDK-2126-1

Conversation

@beekld
Copy link
Copy Markdown
Contributor

@beekld beekld commented Apr 27, 2026

Added a new hook to the SSE client builder to allow setting a callback function that can modify the target before each connection. The target includes the URL's path and query parameters, but not the host or scheme. This allows changing the selector passed to the backend while still letting the SSE client handle retries itself internally.

I chose the http::request object as the thing to be mutable because it's already a public part of the API, and because changing anything else (such as the host or scheme) would be a much more involved change.

Changed the tests to run for both Curl and Foxy, instead of only Curl. This found a couple of minor Foxy bugs, which are fixed.

There's a lot of churn from running clang-format. I apologize it makes the review more difficult, but I think it's the right thing to do. :-/


Note

Medium Risk
Touches connection initiation/reconnect behavior and request construction in both Foxy and CURL backends; incorrect hook interactions or URL/redirect handling could break connectivity or retry semantics.

Overview
Adds Builder::on_connect (a ConnectionHook) to let callers mutate the outgoing http::request immediately before every connection attempt (initial connect and reconnect), enabling per-connection changes like varying the request target/headers/body.

Wires the hook through both networking backends: Foxy now invokes the hook before async_connect, re-prepares payload, and updates redirect handling to also change host_/port_; CURL now applies the hook before each transfer, rebuilds the URL from the possibly-mutated target, and explicitly reasserts client-managed Last-Event-ID semantics.

Hardens request construction by ensuring an empty URL path is sent as /, and replaces CURL-only tests with a unified client_test.cpp suite (including new hook behavior tests).

Reviewed by Cursor Bugbot for commit c7d13d8. Bugbot is set up for automated code reviews on this repo. Configure here.

@beekld beekld marked this pull request as ready for review April 27, 2026 21:20
@beekld beekld requested a review from a team as a code owner April 27, 2026 21:20
Comment thread libs/server-sent-events/src/client.cpp
using EventReceiver = std::function<void(Event)>;
using LogCallback = std::function<void(std::string)>;
using ErrorCallback = std::function<void(Error)>;
using ConnectionHook =
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.

Of course in newer C++ we could make this noexcept without a bunch of hoopla, but not in C++17.

So I am going to suggest we potentially have a try-catch around the connection_hook invocations?

Or we just use strong wording on the signature.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't follow. We don't have a try-catch around LogCallback or ErrorCallback. Are you worried about us accidentally throwing from inside the hook?

What would you wanna do if we did? Mark the connection as immediately failed and generate an error?

Comment thread libs/server-sent-events/src/client.cpp
Comment thread libs/server-sent-events/src/client.cpp Outdated
Copy link
Copy Markdown
Member

@kinyoklion kinyoklion left a comment

Choose a reason for hiding this comment

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

Requesting changes for body length recalculation. Details in comment.

Comment thread libs/server-sent-events/src/client.cpp
@beekld beekld requested a review from kinyoklion April 27, 2026 23:53
Comment thread libs/server-sent-events/src/curl_client.cpp
Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 4ec32df. Configure here.

Comment thread libs/server-sent-events/src/client.cpp
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants