feat: Support per-attempt URIs and typed unrecoverable status errors in the SSE client#296
feat: Support per-attempt URIs and typed unrecoverable status errors in the SSE client#296kinyoklion wants to merge 3 commits into
Conversation
…in the SSE client A new optional uriProvider is invoked before every connection attempt (including automatic reconnects), allowing query parameters to vary between attempts -- for example a state selector that advances as data is received. Unrecoverable HTTP status codes are now reported as a typed UnrecoverableStatusError carrying the status code and response headers, so consumers can distinguish terminal failures from transient ones and read service directives delivered on error responses.
The html implementation constructs a fresh EventSource for every connection attempt (its error handling restarts with its own backoff rather than relying on native reconnection), so each attempt now consults the uriProvider the same way the HTTP implementation does. Also document that the html implementation is incapable of reporting unrecoverable errors: the native EventSource exposes no HTTP status codes or response headers, so every failure is retried indefinitely and UnrecoverableStatusError is never produced on web.
There was a problem hiding this comment.
So, currently the event source isn't reporting unrecoverable errors. This will allow for that determination to be made and the data source to stop.
In the web this type of status cannot be reported. Instead the FDv2 data source will be interrupted until eventually it will fall back to polling. Polling would then get an unrecoverable error and we would shutdown the event source.
|
bugbot review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit fb41f85. Configure here.
tanderson-ld
left a comment
There was a problem hiding this comment.
Just a doc change request.
| HtmlSseClient(Uri uri, Set<String> eventTypes, EventSourceLogger? logger) | ||
| /// to the [uri] provided, or to the [uriProvider] result when one is | ||
| /// given. Every connection attempt constructs a fresh `EventSource`, | ||
| /// so the provider is consulted on each reconnect. |
There was a problem hiding this comment.
Can uri be eliminated in favor of only uriProvider? Perhaps that is breaking and so that was not done.
If uri cannot be eliminated, the comment on the code could be more explicit that uriProvider is consulted first and then uri is used if uriProvider can't provide. As the comment is written, it isn't obvious that uri isn't used initially. The "consulted on each reconnect" part makes one wonder if it is consulted on first connect if not looking at the code.
There was a problem hiding this comment.
I think it could instead me a URI modifier with a default implementation that returns the URI.
There was a problem hiding this comment.
Actually, maybe just a comment update. There isn't a strong need for it to be a modifier. I am leaving the URI for compatibility.
The fixed uri is used only when no provider is given; a provider is consulted for every attempt, not just reconnects.
What this adds
uriProvider: an optional callback onSSEClientthat is invoked before every connection attempt (including automatic reconnects) and whose result is used in place of the fixed URI. This is core to FDv2: streaming reconnects must carry the currentbasisselector as a query parameter, which advances as payloads are received. Without it, a reconnect re-establishes with a stale basis and the server replays changes the SDK already has. Supported on all platforms — the html implementation builds a freshEventSourceper attempt (its error handling restarts with its own backoff rather than relying on native reconnection), so it consults the provider the same way the HTTP implementation does.UnrecoverableStatusError: unrecoverable HTTP status codes (anything other than 200, 400, 408, 429, or 5xx) were previously reported as aClientExceptionwith the status embedded in the message text. They are now a typed error carrying the status code and response headers, so consumers can distinguish terminal failures from transient ones and read service directives delivered on error responses (e.g. the FDv2 fallback directive). The client's behavior is otherwise unchanged: it still stops reconnecting and reports the error on the stream.Platform limitation, now documented
When running in the browser, the native
EventSourceexposes no HTTP status codes or response headers, so the html implementation is incapable of reporting terminal errors: every failure is treated as recoverable and retried with backoff indefinitely, andUnrecoverableStatusErroris never produced on web. Consumers that need to react to unrecoverable statuses (e.g. invalid credentials) must detect them through a transport that can observe HTTP responses, such as a polling request. This is now stated onSSEClient,HtmlSseClient, andUnrecoverableStatusError.Both API changes are additive; existing consumers are unaffected.
Testing
New unit tests cover the per-attempt URI resolution (fixed URI without a provider, provider invoked per attempt) and the typed error's status code and headers. The html implementation was additionally compile-checked for the web target.
SDK-2186
Note
Low Risk
Additive API and error-type change in the SSE client package; existing callers without
uriProviderbehave the same aside from a more specific stream error type on HTTP.Overview
Adds optional
uriProvideronSSEClientso each connect and reconnect uses a freshly resolved URI (viaStateValues.connectUrion HTTP and a newEventSourceon web), enabling query params such as an advancing FDv2basisselector.Non-retryable HTTP failures on the HTTP transport are now surfaced as exported
UnrecoverableStatusError(status + headers) instead of aClientExceptionwith the code in the message; reconnect behavior is unchanged. Docs call out that browserEventSourcecannot observe HTTP responses, so web never emits this error and keeps retrying with backoff.Unit tests cover fixed vs per-attempt URIs and typed 401 errors with response headers.
Reviewed by Cursor Bugbot for commit 023ad08. Bugbot is set up for automated code reviews on this repo. Configure here.