Repository navigation
Fix waiting for the data channel send buffer to drain - #1267
hiroshihorie wants to merge 12 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.
…on-terminal-paths
waitForBufferStatusLow() threw, or completed its wait with, the plain string 'Engine disconnected' once the engine closed. Data stream writes surface that error, so an app catching LiveKitException missed it. Both paths now use UnexpectedConnectionState, which client-sdk-js also throws when its engine is closed.
…-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}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
A full reconnect creates its new, empty data channel before it finishes. Resetting the cached status there released a blocked stream write mid reconnect, and sendDataPacket then parked the chunk for a resume replay that a full reconnect never runs, so the write completed and the chunk was never sent. The wait now also holds while the engine is reconnecting, the way rust-sdks makes publish_data wait for a reconnection, so the write goes out on the new channel or fails if the engine closes. The mock peer connection reports each reliable channel it creates, so the test can watch the replacement channel.
waitForBufferStatusLow checked its condition every 10 ms. Everything it waits on is engine state: a channel's buffer status, the reconnect flag and closure. Each of those now completes a shared signal when it changes, through _setBufferStatus, the _isReconnecting setter and markClosed, and the wait checks its condition again each time the signal fires. A wait wakes as soon as the channel reports a drain, and nothing runs while it waits. flutter_webrtc calls onBufferedAmountLow on every native buffered amount change below the threshold, so a drain always reaches the signal. The one channel that never reports a drain is one a full reconnect replaced, and creating its replacement resets the status. The test that drained the mock channel without any event now replaces it with a real full reconnect.
| if (isReady()) return; | ||
| await _sendStateChanged.future; |
There was a problem hiding this comment.
🔴 Stream writes hang after silent buffer drain
When a full channel drains without a low-buffer callback, waitForBufferStatusLow never wakes. _sendStateChanged only fires for explicit status updates, reconnects, or closure, so stream writes stay blocked.
Learn more
A stream writer calls waitForBufferStatusLow before each chunk. The wait stores a full buffer status after a send, then relies on a callback to update it. If the channel drains without that callback, neither the cached status nor _sendStateChanged changes. The previous timed check also read the live channel, so a silent drain eventually released the write.
Example: A reliable channel reports 10 MiB buffered, then drains to zero without invoking onBufferedAmountLow. The next stream chunk stays pending indefinitely instead of sending on the empty channel.
Recommended fix: Preserve a timed fallback that periodically checks isBufferStatusLow(kind) while retaining notifications for prompt wakeups and markClosed() for prompt failure. Cancel the fallback when the wait resolves.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
This isn't a regression. The previous loop only polled the cached _dcBufferStatus flag, not the live channel, so a drain that never updated the flag hung there too. On iOS, macOS and Android, flutter_webrtc updates bufferedAmount from the buffered amount change event, and that event also calls onBufferedAmountLow whenever the amount is below the threshold, so a drain can't be silent. Windows and Linux are different: they send no buffered amount events, so the wait can stall once more than 2 MiB is queued. That was already true before this PR, and a timer that re-reads isBufferStatusLow wouldn't help there because the Dart side only learns the new amount from getBufferedAmount(). I'll handle desktop in a follow-up.
…-paths' into hiroshi/fix-buffer-status-wait # Conflicts: # lib/src/core/engine.dart
Middle of stack 1276: #1268 below, #1277 on top. The PR's own commits are eccdb11, bb1c5b8, 64e9ccd and daf740d, the rest are merges of #1268.
Engine.waitForBufferStatusLowis the backpressure wait for data stream writes. Onmainit had three bugs:disconnect()the wait resolved as if the buffer had drained and the error went uncaught. Ondispose()the 10 ms poll never ended.EngineClosingEventlistener was never cancelled.The wait now
The wait is event-driven.
_sendStateChangedis aReusableCompleterpulsed by every buffer status write, by every change of the reconnecting state, and bymarkClosed().waitForBufferStatusLowis_waitUntil(() => !_isReconnecting && (cached low || live channel low)), and_waitUntilre-checks on each pulse and throwsUnexpectedConnectionStateif the engine closes first. No listener, no timer.Two consequences:
sendDataPacketduring the reconnect, which parked it, and the chunk was lost. That was Devin's lost-chunk finding on this PR.Bug 2 depends on #1268 marking 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.
Known gap, out of scope
On Windows and Linux, flutter_webrtc sends no buffered-amount events, so a wait on a buffer past 2 MiB only ends on a reconnect or closure.
mainbehaves the same. A desktop fallback has to callgetBufferedAmount()on a timer, which is a follow-up.Tests
test/core/buffer_status_wait_test.darthas five tests: a wait ends when a full reconnect replaces the full channel, wakes as soon as the channel reports a drain, does not leave an engine listener behind, still fails when the engine closes, and holds until a full reconnect finishes. All five fail against #1268'slib.MockDataChannelgainsfakeBufferedAmountandonMockReliableChannelCreated. Full suite at 0df11e5: 526 passed, 1 skipped. analyze, format, import_sorter and version check pass.The 32 KB low-water-mark e2e runs quoted in an earlier version of this description were against the polling design and are not repeated here. On the stack top, the scenarios tagged
reconnect,dataandsignalingin e2e-flutter pass, see #1277.