Add new data channel high / low water mark approach for packet congestion control#2013
Add new data channel high / low water mark approach for packet congestion control#20131egoman wants to merge 4 commits into
Conversation
…congestion control
🦋 Changeset detectedLatest commit: 2b509cd The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
size-limit report 📦
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| await new TypedPromise<void, UnexpectedConnectionState>((resolve, reject) => { | ||
| const onBufferedAmountLow = () => resolve(); | ||
| dc.addEventListener('bufferedamountlow', onBufferedAmountLow, { once: true }); | ||
| // Proxy along any error caused by the engine closing while we wait. | ||
| this.bufferStatusLowClosingFuture.promise.catch((e) => { | ||
| dc.removeEventListener('bufferedamountlow', onBufferedAmountLow); | ||
| reject(e); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🔴 Flow-control wait can deadlock all senders if the data channel closes unexpectedly without the engine closing
The per-kind mutex is held while awaiting the browser's drain event (await lock.lock() at src/room/RTCEngine.ts:1784), but if the data channel closes independently of the engine, the drain event never fires and the mutex is never released, so all subsequent sends for that channel kind hang forever.
Impact: If a data channel is aborted (e.g. by an oversized message), all future reliable or data-track sends silently hang instead of failing with an error.
Trigger condition and mechanism
The waitUntilBelowHighWaterMark method (line 1778) acquires a per-kind mutex, then if the buffer is above the high-water mark, awaits a bufferedamountlow event on the data channel (line 1798). The only escape hatch is bufferStatusLowClosingFuture which rejects only when the engine emits EngineEvent.Closing (src/room/RTCEngine.ts:302).
However, handleDataChannelClose (src/room/RTCEngine.ts:1107-1117) only logs an error — it does not reject the future or release the mutex. So if the DC closes while a sender is waiting (e.g. because an oversized message aborted the SCTP association), the TypedPromise at line 1796 never resolves or rejects. The finally block (line 1805) never runs, so unlock() is never called. All subsequent callers of waitUntilBelowHighWaterMark for that kind will block on lock.lock() at line 1784 indefinitely.
Before this PR, a single caller would hang but other callers could still attempt sends. The mutex serialization introduced by this PR turns a single-caller hang into a full deadlock for all callers of that kind.
Was this helpful? React with 👍 or 👎 to provide feedback.
1egoman
force-pushed
the
data-channel-buffer-range
branch
from
a34512a to
2b509cd
Compare
| const dc = this.dataChannelForKind(kind); | ||
| if (dc) { | ||
| if (!this.isBufferStatusLow(kind)) { | ||
| if (!this.isBelowHighWaterMark(kind)) { |
There was a problem hiding this comment.
🔴 Lossy data channel drops messages too late, breaking adaptive latency control
The lossy channel's drop-or-send decision now compares against a fixed 256 KB ceiling (isBelowHighWaterMark at src/room/RTCEngine.ts:1676) instead of the adaptive threshold that targeted ~100 ms of buffered latency, so low-rate lossy streams can accumulate seconds of data before any messages are dropped.
Impact: Lossy data (e.g. real-time sensor telemetry) may experience significantly higher latency at low data rates because the buffer must fill to 256 KB before dropping kicks in, whereas the old adaptive threshold could be as low as 8 KB.
Mechanism: adaptive threshold is computed but no longer consulted for the send gate
The adaptive threshold is still computed every second and written to dc.bufferedAmountLowThreshold (src/room/RTCEngine.ts:977-980), but the send gate in sendLossyBytes now calls isBelowHighWaterMark(kind) (src/room/RTCEngine.ts:1676) which compares dc.bufferedAmount against the fixed dataChannelHighWaterMark(DataChannelKind.LOSSY) = 256 KB (src/room/RTCEngine.ts:122). Previously, isBufferStatusLow compared against dc.bufferedAmountLowThreshold, which was the adaptive value clamped between 8 KB and 256 KB based on the measured byte rate. The comment at src/room/RTCEngine.ts:141-142 even states the lossy channel "keeps its adaptive single threshold and never consults these" watermarks, confirming the code contradicts the intended design.
Prompt for agents
In sendLossyBytes, the drop decision for the lossy channel (DataChannelKind.LOSSY) should use the adaptive bufferedAmountLowThreshold rather than the fixed high-water mark. The old code checked dc.bufferedAmount <= dc.bufferedAmountLowThreshold which respected the adaptive threshold computed every second in the setInterval at line 969. The new isBelowHighWaterMark function compares against the fixed 256KB constant, which defeats the adaptive latency control. Consider either: (1) restoring the old adaptive check for LOSSY kind specifically (e.g. keep using dc.bufferedAmountLowThreshold for LOSSY), or (2) using isBelowHighWaterMark only for RELIABLE and DATA_TRACK_LOSSY kinds. The same issue applies to the updateAndEmitDCBufferStatus function at line 1729 which now uses isBelowLowWaterMark (fixed 8KB) instead of the adaptive threshold for the DCBufferStatusChanged event.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
TODO: add notes in here