Conversation
45e2b3a to
26bef5e
Compare
26bef5e to
bf3e8ed
Compare
| case ServerMessage.OneOffQueryResult: | ||
| /* OneOffQuery is async and handles its own responses */ | ||
| case ServerMessage.OneOffQueryResult(var result): | ||
| if (waitingOneOffQueries.TryRemove(result.RequestId, out var completion)) |
There was a problem hiding this comment.
This change would make OneOffQuery not async anymore. We used to call this exact thing on the ParseMessage thread, this change would move it to the same thread that calls FrameTick(). This can also add a performance regression if the one off query has a large result, it can stall the update processing.
| } | ||
|
|
||
| _parseCancellationTokenSource.Cancel(); | ||
| EndConnection(); |
There was a problem hiding this comment.
This refactor could lead to an infinite recursion if you call Disconnect() from OnDisconnect. We can probably just check if (isClosing) return; in here, before calling EndConnection(). If we don't and a user calls Disconnect inside of OnDisconnect, we will call the callback on line 594, which will in turncall OnDisconnect again and we'll get infinite recursion until the stack explodes.
| QueryStrings = querySqls.ToList(), | ||
| } | ||
| )); | ||
| subscriptionQueries[querySetId] = (string[])querySqls.Clone(); |
There was a problem hiding this comment.
Retaining this subscriptionQueries is unnecessary when auto reconnect is not enabled. We would be calling .Clone() and retaining memory for no reason.
We can change SendSubscription to receive the querySqls directly. We're calling SendSubscription in 2 places:
- Line 882, we could call
SendSubscription(querySetId, querySqls) - Line 265 (deferred initial subscriptions), we could call
SendSubscription(id, subscriptionQueries[id])
We can also very likely get rid of the .Clone() here and the .ToList() inside SendSubscription?
| } | ||
| }; | ||
|
|
||
| webSocket = CreateWebSocket(); |
There was a problem hiding this comment.
We're immediately destroying this web socket in StartSocket in Reconnect.cs. We can also probably combine DbConnectionBase into only one declaration. I'm not opposed to partial classes, but the separation here isn't clean. We're calling CreateWebSocket here and CreateWebSocket is defined in Reconnect.cs.
This branch has not been deployed
2 participants
Description of Changes
C# SDK: automatic reconnection
Builds reconnection into DbConnection itself, so the connection object, the client cache, table handles and registered callbacks all stay usable across a reconnect and applications manage no replacement objects.
WithAutomaticReconnect()enables this. Losing an established connection fires OnDisconnect, each failed attempt fires OnConnectError, and both now report the number of and delay before the next attempt, or undefined when the SDK will not retry. Retries are unlimited with exponential backoff (1s base, 30s cap, 0.5 jitter), stopping only on an explicit Disconnect(), a failed initial connection, or a reconnect under a different identity.Each reconnect is a new connection which fires OnConnect again. Live subscriptions are replayed in one SubscribeBatch under fresh query set ids, and the results are reconciled against the stale cache with synthesized deletes, so rows unchanged across the outage produce no callbacks and changed rows produce ordinary update or delete callbacks.
A stable client-generated session_id is sent on every connection so the server can teardown the previous connection if still open (idle timeout didn't fire yet) before accepting a new one for the same session.
The token the server issued is saved and reused, keeping the identity stable for anonymous clients;
WithTokenProvider()supplies a fresh token when the saved one is close to expiring or was rejected.Calls made while disconnected fail fast with DisconnectedError, and calls in flight when the connection dropped settle with UnknownCallResultError, since they may or may not have run.
Behavior is unchanged for connections which do not opt in.
API and ABI breaking changes
No
Rollback safety impact
Requires #5877 to have been deployed in a previous release
Expected complexity level and risk
3 - Change is limited to the typescript SDK and is opt in only (withAutomaticReconnect and withTokenProvider are the new APIs). Change is large but not overly complex.
Testing
Test app that fakes dropping connections and reconnecting and uses spacetime's own token provider to test token with expiry claim in the reconnect scenario as well.