Repository navigation
Fix waiting for the data channel send buffer to drain - #1267
hiroshihorie wants to merge 4 commits into
Conversation
waitForBufferStatusLow had three problems: - It polled only the cached buffer status, which changes on a send or on the channel's low-buffer event. When a full reconnect replaced a backed up channel, the new one fired neither, so a waiter could hang until disconnect. The cache is now reset when the channels are created, and the wait also checks the live channel. - When the engine closed during a wait, the wait resolved normally and the caller went on to send. It now fails. - Every wait that blocked left an EngineClosingEvent listener behind until the engine closed. The listener is now cancelled. MockDataChannel gains fakeBufferedAmount to simulate a channel that has not drained.
…-paths' into hiroshi/fix-buffer-status-wait # Conflicts: # lib/src/core/engine.dart
Every terminal path now marks the engine closed synchronously, so the wait can check that state directly. This drops the listener the wait had to remove again, and a wait started after closure fails the same way.
…-paths' into hiroshi/fix-buffer-status-wait waitForBufferStatusLow() keeps this branch's polling wait on the closed state. The base now throws UnexpectedConnectionState instead of a string, so the wait throws that too.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| ), | ||
| ); | ||
| _reliableDCPub?.bufferedAmountLowThreshold = 2 * 1024 * 1024; | ||
| _dcBufferStatus[Reliability.reliable] = true; |
There was a problem hiding this comment.
🔴 Stream chunks lost during full reconnect
When a full reconnect replaces a full channel, _dcBufferStatus releases a waiting stream write before reconnection finishes. sendDataPacket defers that chunk, but full reconnect never resends it; the write succeeds without delivery.
Learn more
A data stream write waits in WritableStream.write whenever the reliable channel is full. During restartConnection, the engine remains reconnecting while the replacement data channels are created. Resetting the cached status here releases the waiting write as soon as that channel exists, before the reconnect succeeds. sendDataPacket then buffers the reliable packet and returns early because _isReconnecting is true. Only the resume response calls _resendReliableMessagesForResume; a full restart does not send those queued packets.
Example: A stream chunk waits on a full channel. A full reconnect creates an empty reliable channel, releasing the write. The write completes while reconnecting, but the recipient never gets the chunk.
Recommended fix: Keep buffer waits pending until the replacement channel is usable and reconnection has completed, or explicitly resend reliable packets deferred during a full reconnect. Test an in-flight stream write across an actual full reconnect and assert receipt of the chunk.
Was this helpful? React with 👍 or 👎 to provide feedback.
Stacked on #1268. Review only the last commit, bb1c5b8. The merge commit before it brings in #1268.
Engine.waitForBufferStatusLowis the backpressure wait for data stream writes. It had three bugs:'Engine disconnected'.EngineClosingEventlistener was never cancelled. The wait no longer uses a listener at all.Bugs 2 and 3 are fixed by checking the engine's closed state in the wait loop instead of listening for
EngineClosingEvent. That only works because #1268 marks the engine closed synchronously on every terminal path, including room disposal and a reconnect that gives up. Hence the stack.This matters more with #1166, where every native data stream packet goes through this wait.
Tests
test/core/buffer_status_wait_test.darthas one test per bug. All three fail on #1268 and onmain, and pass here.MockDataChannelgainsfakeBufferedAmount, identical to #1266's, so the two merge cleanly. analyze, format, import_sorter, version check andflutter testall pass.E2E (macOS, local
livekit-server --dev)A local server drains the channel faster than the 2 MB low-water mark, so these runs used test-only builds with the mark lowered to 32 KB (not in this PR):
mainOn macOS the old channel's low-buffer event during teardown releases the waiter, so bug 1 doesn't show there. Other platforms are unchecked.
Merge order
#1264, then #1268, then this PR. Once #1268 merges, this PR retargets to
mainand I'll mergemainback in.