Repository navigation
Run request hooks on WebSocket upgrades; add free-form connection_init payload - #119
Merged
Merged
Conversation
…m connection_init payload WSClient never called ConfigureRequestAsync, so WithAccessToken/ WithRequestConfiguration/ConfigureRequest reached queries and SSE but not the WebSocket subscription upgrade (issue Linq2GraphQL#117). The connection factory is now async (Websocket.Client Func<Uri, CancellationToken, Task<WebSocket>>) and runs the hooks on a dummy GET for the subscription URL before every connect - initial and each reconnect - copying the resulting headers onto the upgrade request (handshake-owned headers skipped). On browsers, where custom WebSocket headers are impossible, the hook's Authorization value is captured for the init payload instead. GraphClientOptions.ConnectionInitPayload (new) sets the connection_init payload to any JSON object (graphql-ws/Apollo connection-parameter convention, e.g. Authorization/authToken); it is called before every (re)connect. SendAuthorizationInConnectionInit (new) merges the hook's Authorization header into that payload for Blazor WebAssembly. WSConnectionInitPayload is obsolete (still honored as fallback). Tests: upgrade header assertions for both WS protocols (server records snapshotted upgrade headers), token refresh across reconnect, free-form and legacy payloads, merge behaviour, hook failure on initial connect.
From four-axis review (standards, spec, graphql-architect, performance-engineer): - ConnectTimeout 30s (was Websocket.Client default 5s): request hooks (token fetch) run inside the connect budget and a slow IdP round-trip must not fail the connect. - Failed initial connect no longer retries in the background: InitializeAsync marks the failure terminal before rethrowing from Start(), and the retry loop re-checks the terminal flag after each backoff delay. Previously a deterministic failure (e.g. a throwing token hook) was retried 5x with backoff and surfaced a second, delayed error after Start() had already thrown. - SendAuthorizationInConnectionInit merge accepts any string-keyed dictionary shape (IEnumerable<KeyValuePair<string,object>>), not just IDictionary<string,object>; error message and docs state the contract. - Docs: fixed broken GraphClient.WithAccessToken cref (generated-client method), documented GraphClient.ConnectionInitPayload (and added it to the CHANGELOG), wss:// note for tokens in connection_init, connect- timeout budget note, CHANGELOG dictionary example wrapped correctly. - Tests: no-background-retry regression test, IReadOnlyDictionary merge test.
Self-review loop round: InitializeAsync now marks ANY initial connect/handshake failure terminal (missing subprotocol, connection_init build throw, ack timeout), not only a StartOrFail transport error. Previously the server's later close (e.g. 4408 on its own ack timeout) started the bounded retry loop after Start() had already thrown, re-running hooks and reporting a second, delayed error. Also: ConnectionInitPayload doc and the merge InvalidOperationException state the exact dictionary shape (string keys, object values); CHANGELOG wording broadened to connect-or-handshake. Tests: non-dictionary payload + SendAuthorizationInConnectionInit throws with guidance; a request hook slower than Websocket.Client's 5 s default ConnectTimeout still connects (pins the raised 30 s budget).
ricred
changed the title
…llow-up) - Resolve the connection_init payload in the connection factory, before the socket opens (within ConnectTimeout), so a slow token fetch cannot run into the server's connection_init wait (graphql-ws closes with 4408). - Send HttpClient.DefaultRequestHeaders on the upgrade request (hook headers win on a name conflict) and use DefaultRequestHeaders.Authorization as the SendAuthorizationInConnectionInit fallback, matching the query path. - Merge the Authorization parameter into any payload that serializes to a JSON object (anonymous objects, Dictionary<string,string>, JsonObject, JsonElement, POCOs); other shapes still throw. - Surface exceptions from request hooks and the payload delegate as their original type instead of Websocket.Client's WebsocketException wrapper (inner exception of the terminal IOException on reconnect). Cancellation stays wrapped. - Make HookThrowingOnInitialConnect_DoesNotRetryInBackground deterministic via an internal RetryDelay seam instead of outwaiting the real backoff. Tests: 10 new cases; each fix verified by a mutation that the tests kill.
IdleTimeoutStream armed one shared timer at the start of each read and never
disarmed it, so time the consumer spent between reads (handling an event, or a
continuation starved of a thread) counted as idle. A single stall longer than
the window failed a healthy stream with "no data received" - the cause of the
CI flake in SlowButRegularData_DoesNotTimeOut (one stalled continuation on a
loaded runner exhausted the 750 ms window).
- Each read gets its own CancellationTokenSource, linked to the caller's token,
with CancelAfter(idleTimeout), disposed when the read completes. Consumer time
between reads never counts, and a timer that fired during a read which still
returned data cannot poison the next read.
- An OperationCanceledException the inner stream raises on its own is no longer
mislabelled as an idle timeout.
- The timeout message states the configured value exactly ("750 ms", "30 s",
"<1 ms") instead of rounding to whole seconds ("1s", "0s").
Tests: 7 new cases (consumer pause, delay before first read, fired timer with
data returned, message format theory, inner cancellation), and the external
cancel test now also asserts that the cancel propagates promptly. Each fix is
verified by a mutation that the tests kill.
Contributor
Author
|
@joadan can you please review this PR ? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #117.
WebSocket subscriptions ignored the client''s request hooks:
GraphClientOptions.ConfigureRequestand theWithRequestConfiguration/WithAccessTokenhelpers that generated clients expose ran for queries, mutations, and server-sent-event subscriptions — but never for the WebSocket transport. Subscriptions to authenticated endpoints failed the upgrade (or the handshake) with no way to attach credentials.What changed
Request hooks run on every WebSocket connect. The transport now uses the Websocket.Client 5.3.0 async connection factory (
Func<Uri, CancellationToken, Task<WebSocket>>), which runs for the initial connect and for every reconnect attempt of the bounded retry loop. Before opening the socket, it:GETrequest for the subscription URL,ClientWebSocketOptions.SetRequestHeader, skipping handshake-owned headers (Host,Connection,Upgrade,Sec-WebSocket-*),Authorizationvalue for the browser path below.A refreshed token is therefore picked up on reconnect.
Connect timeout raised to 30 s. The library''s 5 s default covers the whole connection factory — hooks included — so a token fetch with a slow identity-provider round-trip would otherwise abort the connect.
Browser support (
OperatingSystem.IsBrowser). Browsers cannot send custom headers on WebSocket upgrade requests, so the header copy is skipped there and the hook''sAuthorizationvalue travels insideconnection_initinstead (opt-in below).New:
GraphClientOptions.ConnectionInitPayload(plus a matchingGraphClient.ConnectionInitPayloadproperty). AFunc<GraphClient, CancellationToken, ValueTask<object>>returning any JSON object the server expects, for example{ ["Authorization"] = $"Bearer {token}" }(the graphql-ws / Apollo connection-parameter convention). Called before every (re)connect.New:
GraphClientOptions.SendAuthorizationInConnectionInit(defaultfalse). Merges the hook-producedAuthorizationvalue into theconnection_initpayload — the Blazor WebAssembly path for per-user tokens. Merge contract:null{ "Authorization": <hook value> }IDictionary<string, object>or the read-only equivalent)AuthorizationoverwrittenInvalidOperationExceptionnaming the expected shapeBehavior change: no background retry after a failed initial connect or handshake. Any failure during
InitializeAsyncis terminal and surfaces once fromStartAsync. Previously the transport''sErrordisconnect also started the reconnect loop, re-running the hooks up to five times and reporting a second, delayed error for a failure the caller had already handled. Reconnection after an established connection drops is unchanged.Compatibility
Non-breaking:
WSConnectionInitPayloadis[Obsolete](warning only) and still honored whenConnectionInitPayloadis unset — including on theSendAuthorizationInConnectionInitmerge path.Migration for the obsolete option: replace
WSConnectionInitPayload = client => ...withConnectionInitPayload = (client, ct) => ValueTask.FromResult<object>(...), returning the payload''s JSON shape directly instead of aGraphQLRequest.Testing
graphql-transport-wsand legacy Apollo), a fresh token per reconnect, free-form payload (delivered verbatim, re-asked per reconnect), precedence over the legacy option, every merge branch, hook-failure semantics (failsStart, no background retry), and a request hook slower than the old 5 s timeout that must still connect (pins the 30 s budget).scripts/regenerate-test-clients.ps1 -Check: clean.Reviewer notes
The core review surface is
WSClient.cs(connection factory,BuildConnectionInitPayloadAsync, terminal handling inInitializeAsync/ the retry loop),GraphClient.cs(GetConnectionInitPayloadAsync), andGraphClientOptions.cs. The remaining files are tests and documentation.Not covered by execution
The
OperatingSystem.IsBrowserbranches (header skip and payload-based token delivery) have no browser host in this repository and were verified by code reading only.