Bound main-thread work when catching up on websocket messages (fixes resume-frame freeze) - #225
Open
harlan wants to merge 4 commits into
Open
Bound main-thread work when catching up on websocket messages (fixes resume-frame freeze)#225harlan wants to merge 4 commits into
harlan wants to merge 4 commits into
Conversation
Exposes how many received messages are waiting to be dequeued. Receiving runs
on a background timer thread while consumers dequeue from Unity's main loop,
so the depth is the only measure of how far behind the consumer is, and until
now nothing could observe it.
Used by the stalled-pump warning added later in this series.
Deliberately read-only: there is no companion "discard the backlog" call,
because a dropped protocol event cannot be recovered. The only catch-up
mechanism is /sync, which the server rejects past roughly 1000 missed events,
so discarding would leave local channel state permanently stale.
Purely additive to SDK behavior, though note it is a new member on the public
IWebsocketClient interface, so an integrator supplying their own
implementation will need to add it. Implemented on both bundled ones:
* WebsocketClient — ConcurrentQueue.Count (all platforms but WebGL)
* NativeWebSocketWrapper — Queue.Count (UNITY_WEBGL only, so it is not
covered by a non-WebGL compile)
StreamChatLowLevelClient.Update drained the receive queue to empty every
frame:
while (_websocketClient.TryDequeueMessage(out var msg))
HandleNewWebsocketMessage(msg);
Messages arrive on a background timer thread but are only handled here, on
Unity's main loop, which the OS stops while the app is backgrounded. So the
queue fills with nothing draining it, and the entire backlog is then handled
in the FIRST frame after resume. Per-message cost in a real consumer is not
small — in our app each message drives a persistence write, profile
resolution, and a feed reload — so that frame locks the app up for seconds.
This was a reported bug: iOS players backgrounding during chat traffic and
returning to a frozen app.
Now at most MaxMessagesHandledPerUpdate (20) are handled per Update and the
rest stay queued for following frames. Nothing is dropped or reordered; one
catastrophic frame becomes ~50 briefly degraded ones.
The cap does not bind in steady state. WebsocketClient's reader enqueues at
most one message per 50ms tick (UpdatesPerSecond = 20), while the drain runs
at 20/frame = 600/s at 30fps — 30x headroom, so the loop still exits on an
empty queue exactly as before. It only meters catch-up after a stall. WebGL's
NativeWebSocketWrapper is browser-paced with no such ceiling, so there the cap
does spread a burst over frames, which is the intent anyway.
Happy to make the constant configurable via IStreamClientConfig if you would
prefer that over a hardcoded value.
On every reconnect, StreamChatClient.OnConnected calls
RestoreStateLostDuringDisconnect -> FetchAndProcessEventsSinceLastReceivedEvent,
which fetches the events missed while disconnected and handled ALL of them in a
single foreach, inline:
foreach (var e in response.Events) { ...; HandleNewWebsocketMessage(serializedMsg); }
That bypassed the per-frame drain cap from the previous commit entirely, so the
cap only covered live socket messages and left the bigger offender untouched.
This replay is at its largest precisely when an app returns from the
background — the socket died while suspended, so the resume reconnect asks for
everything missed, up to the ~1000-event /sync limit — and every event costs a
deserialize plus full downstream handling. On iOS, where the process is
suspended and no live backlog can accumulate, this replay IS the resume freeze.
Now the events are queued into _pendingReplayEvents and drained by Update under
the same MaxMessagesHandledPerUpdate budget as socket messages. Replay drains
before live messages, so ordering is preserved within a frame (replayed events
are older). Nothing is dropped. The queue is a plain Queue<string>, matching the
main-thread assumption the direct HandleNewWebsocketMessage call already made
here.
The queue is also cleared when the connection state goes to Disconnected. Those
events are stale, and keeping them is self-compounding: _lastEventReceivedAt
only advances as events are HANDLED, so the sync point captured on disconnect
excludes everything still queued — the next /sync re-fetches exactly those
events and enqueues them BEHIND the undrained remainder. Each reconnect flap
would add another duplicate copy to an unbounded queue, and FIFO order would
replay older events after newer ones had already been handled, dragging
_lastEventReceivedAt backwards mid-drain. Clearing is safe precisely because the
sync point does not count them: whatever is discarded is re-fetched on the next
catch-up.
Logs once, when more than ReceiveBacklogWarningThreshold (500) received messages are waiting to be handled — i.e. the main loop stopped pumping long enough to fall far behind. Re-arms only after the backlog clears, so it cannot spam per frame. Counts the reconnect replay queue as well as the socket's. Both are drained by the same per-frame budget, so both contribute to how far behind we are, and the replay queue is the one that actually gets large: a /sync catch-up can hand back hundreds of events at once, whereas the socket queue is bounded by its ~20/s arrival rate against a 20/frame drain. Diagnostic ONLY. It deliberately does not discard the backlog, and neither should a consumer: reconnecting does not re-hydrate channel state (the sole catch-up is /sync, itself capped at roughly 1000 missed events), so discarding would drop those events permanently and leave local channel state silently stale — strictly worse than a slow drain. The backlog is bounded in practice anyway: health-check pings only go out from this Update, so a stalled client stops pinging and the server closes the socket within ~30s. Happy to make the threshold configurable via IStreamClientConfig if you would prefer that over a hardcoded value.
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
1 participant
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.
Fixes a multi-second freeze when a mobile app resumes from the background during chat traffic. We hit this in production on iOS: players background the app with a busy channel open, come back, and the app is locked up for seconds.
Cause
Websocket messages arrive on a background timer thread but are only handled from
StreamChatLowLevelClient.Update, on Unity's main loop, which the OS stops while the app is backgrounded. Two things then land entirely in the first frame after resume:Updatedrained to empty every frame:while (TryDequeueMessage(out var msg)) HandleNewWebsocketMessage(msg);FetchAndProcessEventsSinceLastReceivedEventhandled its whole/syncresponse inline in oneforeach. This is the bigger offender, and it is largest exactly on resume — the socket died while suspended, so the reconnect asks for everything missed, up to the ~1000-event/synclimit.Per-message cost in a real consumer is not small; in our app each message drives a persistence write, profile resolution, and a feed reload.
Change
Four commits, reviewable in order:
Add IWebsocketClient.QueuedMessageCount— exposes receive-queue depth. Purely additive, but note it is a new member on a public interface, so an integrator supplying their ownIWebsocketClientwill need to add it. Implemented on both bundled clients.Cap websocket messages handled per Update— at mostMaxMessagesHandledPerUpdate(20) per frame.Pace the reconnect missed-event replay through the per-frame budget— the/syncreplay is queued into_pendingReplayEventsand drained byUpdateunder the same budget, replay first so ordering holds (replayed events are older). Also clears that queue on disconnect; the commit message explains why keeping it is self-compounding.Warn once when the receive backlog shows a stalled pump— one-off warning above 500 combined backlog, re-arming only after it clears.Nothing is dropped or reordered — one catastrophic frame becomes ~50 briefly degraded ones.
Why nothing is discarded
Deliberately no "drop the backlog" path, and consumers should not add one: reconnecting does not re-hydrate channel state, and the only catch-up is
/sync, itself capped around 1000 events. Discarding would lose those events permanently and leave local state silently stale — strictly worse than a slow drain. An earlier revision of this work did discard and reset the connection; review showed the "re-sync" it assumed does not exist, and it was removed.The cap does not bind in steady state
WebsocketClient's reader enqueues at most one message per 50ms tick (UpdatesPerSecond = 20), while the drain runs at 20/frame = 600/s at 30fps — 30x headroom, so the loop still exits on an empty queue exactly as before. It only meters catch-up after a stall. WebGL'sNativeWebSocketWrapperis browser-paced with no such ceiling, so there the cap does spread a burst across frames, which is the intent anyway.Open questions
MaxMessagesHandledPerUpdate(20) and the 500 warning threshold are hardcoded. Happy to move either or both ontoIStreamClientConfigif you'd prefer.StreamChatLowLevelClientTestsstubTryDequeueMessageto yield one message thenfalse, well inside the cap, andQueuedMessageCountreturns 0 on an NSubstitute mock — so neither behavior perturbs them. I did not add new tests since driving a >20-message backlog needs a pump harness that doesn't exist yet; glad to add one if you want it.