-
Notifications
You must be signed in to change notification settings - Fork 297
Add new data channel high / low water mark approach for packet congestion control #2013
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
3b101cd
c4aefaf
35215c7
2b509cd
f7d349d
2031267
ccc1669
cbd6a13
22ffc35
756e8e3
23da009
e6fa0d7
95dd9fa
0461bc2
ed2ac47
808bc5a
d6faabc
cc79fd7
3f63fbb
facfe90
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "livekit-client": patch | ||
| --- | ||
|
|
||
| Fix the reliable data channel dying under concurrent / multi-packet writes (#1995). Data-channel sends now use two-watermark flow control (fill up to a high-water mark, resume when drained to a low-water mark) and serialize contended senders through a per-kind lock, which prevents the SCTP send buffer from overflowing while keeping throughput saturated for data tracks. |
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,8 +1,9 @@ | ||||||||||||
| import { DataPacket, DataPacket_Kind, UserPacket } from '@livekit/protocol'; | ||||||||||||
| import { afterEach, describe, expect, it, vi } from 'vitest'; | ||||||||||||
| import type { DataPacketBuffer, DataPacketItem } from '../utils/dataPacketBuffer'; | ||||||||||||
| import RTCEngine, { DataChannelKind } from './RTCEngine'; | ||||||||||||
| import { roomOptionDefaults } from './defaults'; | ||||||||||||
| import { PublishDataError } from './errors'; | ||||||||||||
| import { PublishDataError, UnexpectedConnectionState } from './errors'; | ||||||||||||
|
|
||||||||||||
| describe('RTCEngine', () => { | ||||||||||||
| const originalRTCRtpSender = window.RTCRtpSender; | ||||||||||||
|
|
@@ -230,7 +231,7 @@ describe('RTCEngine', () => { | |||||||||||
| const send = vi.fn(); | ||||||||||||
| Object.assign(engine as unknown as Record<string, unknown>, { | ||||||||||||
| ensurePublisherConnected: vi.fn().mockResolvedValue(undefined), | ||||||||||||
| waitForBufferStatusLow: vi.fn().mockResolvedValue(undefined), | ||||||||||||
| waitForBufferHeadroomWithLock: vi.fn().mockResolvedValue(undefined), | ||||||||||||
| updateAndEmitDCBufferStatus: vi.fn(), | ||||||||||||
| dataChannelForKind: vi.fn(() => ({ send })), | ||||||||||||
| pcManager: { | ||||||||||||
|
|
@@ -298,6 +299,278 @@ describe('RTCEngine', () => { | |||||||||||
| }); | ||||||||||||
| }); | ||||||||||||
|
|
||||||||||||
| class FakeDataChannel extends EventTarget { | ||||||||||||
| bufferedAmount = 0; | ||||||||||||
|
|
||||||||||||
| bufferedAmountLowThreshold = 64 * 1024; | ||||||||||||
|
|
||||||||||||
| send = vi.fn(); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| const tick = () => new Promise<void>((resolve) => setTimeout(resolve, 0)); | ||||||||||||
|
|
||||||||||||
| describe('resendReliableMessagesForResume', () => { | ||||||||||||
| it('does not let a concurrent reliable send interleave into the resume replay', async () => { | ||||||||||||
| const engine = new RTCEngine(roomOptionDefaults); | ||||||||||||
| const dc = new FakeDataChannel(); | ||||||||||||
| Object.assign(engine as unknown as Record<string, unknown>, { | ||||||||||||
| _isClosed: false, | ||||||||||||
| ensurePublisherConnected: vi.fn().mockResolvedValue(undefined), | ||||||||||||
| dataChannelForKind: vi.fn(() => dc), | ||||||||||||
| pcManager: { | ||||||||||||
| getMaxPublisherMessageSize: vi.fn(() => 64 * 1024 - 1), | ||||||||||||
| }, | ||||||||||||
| }); | ||||||||||||
|
|
||||||||||||
| // Two messages queued for replay, and a full buffer so the XX its first send. | ||||||||||||
| const replayed1 = new Uint8Array([1]); | ||||||||||||
| const replayed2 = new Uint8Array([2]); | ||||||||||||
| const buffer = ( | ||||||||||||
| engine as unknown as { reliableMessageBuffer: { push: (item: DataPacketItem) => void } } | ||||||||||||
| ).reliableMessageBuffer; | ||||||||||||
| buffer.push({ data: replayed1, sequence: 1, sent: true }); | ||||||||||||
| buffer.push({ data: replayed2, sequence: 2, sent: true }); | ||||||||||||
| dc.bufferedAmount = 2 * 1024 * 1024; // above the reliable high-water mark | ||||||||||||
|
|
||||||||||||
| const replay = ( | ||||||||||||
| engine as unknown as { resendReliableMessagesForResume: (seq: number) => Promise<void> } | ||||||||||||
| ).resendReliableMessagesForResume(0); | ||||||||||||
| await tick(); | ||||||||||||
|
|
||||||||||||
| // A send racing the replay: its sequence is assigned immediately, but it must not hit the | ||||||||||||
| // wire before the replayed (lower-sequence) messages, or receivers discard those as dupes. | ||||||||||||
| const concurrentSend = engine.sendDataPacket( | ||||||||||||
| new DataPacket({ | ||||||||||||
| kind: DataPacket_Kind.RELIABLE, | ||||||||||||
| value: { case: 'user', value: new UserPacket({ payload: new Uint8Array([3]) }) }, | ||||||||||||
| }), | ||||||||||||
| DataChannelKind.RELIABLE, | ||||||||||||
| ); | ||||||||||||
| await tick(); | ||||||||||||
|
|
||||||||||||
| // Buffer drains: the replay must finish its whole batch before the concurrent send. | ||||||||||||
| dc.bufferedAmount = 0; | ||||||||||||
| dc.dispatchEvent(new Event('bufferedamountlow')); | ||||||||||||
| await Promise.all([replay, concurrentSend]); | ||||||||||||
|
|
||||||||||||
| expect(dc.send).toHaveBeenCalledTimes(3); | ||||||||||||
| expect(dc.send.mock.calls[0][0]).toBe(replayed1); | ||||||||||||
| expect(dc.send.mock.calls[1][0]).toBe(replayed2); | ||||||||||||
| expect(dc.send.mock.calls[2][0]).not.toBe(replayed1); | ||||||||||||
| expect(dc.send.mock.calls[2][0]).not.toBe(replayed2); | ||||||||||||
| }); | ||||||||||||
|
|
||||||||||||
| it('transmits a packet deferred mid-replay instead of marking it sent without sending', async () => { | ||||||||||||
| const engine = new RTCEngine(roomOptionDefaults); | ||||||||||||
| const dc = new FakeDataChannel(); | ||||||||||||
| Object.assign(engine as unknown as Record<string, unknown>, { | ||||||||||||
| _isClosed: false, | ||||||||||||
| // Reconnect still active: a send arriving during replay takes the deferral path. | ||||||||||||
| attemptingReconnect: true, | ||||||||||||
| ensurePublisherConnected: vi.fn().mockResolvedValue(undefined), | ||||||||||||
| dataChannelForKind: vi.fn(() => dc), | ||||||||||||
| pcManager: { | ||||||||||||
| getMaxPublisherMessageSize: vi.fn(() => 64 * 1024 - 1), | ||||||||||||
| }, | ||||||||||||
| }); | ||||||||||||
|
|
||||||||||||
| const buffer = (engine as unknown as { reliableMessageBuffer: DataPacketBuffer }) | ||||||||||||
| .reliableMessageBuffer; | ||||||||||||
|
Comment on lines
+377
to
+378
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nitpick: it might be nice to have some test specific helpers to avoid all the casting to access these private members. Maybe something like
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At one point during the work on this PR I was already half way through changing those casts to ts-ignores when I realised that this pattern just emerged because it's how the other tests are doing it as well. |
||||||||||||
| const replayed = new Uint8Array([1]); | ||||||||||||
| buffer.push({ data: replayed, sequence: 1, sent: true }); | ||||||||||||
| // Full buffer so replay parks on waitForBufferHeadroomWithLock before its first send. | ||||||||||||
| dc.bufferedAmount = 2 * 1024 * 1024; | ||||||||||||
|
|
||||||||||||
| const replay = ( | ||||||||||||
| engine as unknown as { resendReliableMessagesForResume: (seq: number) => Promise<void> } | ||||||||||||
| ).resendReliableMessagesForResume(0); | ||||||||||||
| await tick(); | ||||||||||||
|
|
||||||||||||
| // A reliable send arrives while replay is parked; attemptingReconnect defers it into the | ||||||||||||
| // buffer as sent:false, after the replay's first drain pass already started. | ||||||||||||
| const deferred = new Uint8Array([2]); | ||||||||||||
| const deferredSend = engine.sendDataPacket( | ||||||||||||
| new DataPacket({ | ||||||||||||
| kind: DataPacket_Kind.RELIABLE, | ||||||||||||
| value: { case: 'user', value: new UserPacket({ payload: deferred }) }, | ||||||||||||
| }), | ||||||||||||
| DataChannelKind.RELIABLE, | ||||||||||||
| ); | ||||||||||||
| await tick(); | ||||||||||||
|
|
||||||||||||
| dc.bufferedAmount = 0; | ||||||||||||
| dc.dispatchEvent(new Event('bufferedamountlow')); | ||||||||||||
| await Promise.all([replay, deferredSend]); | ||||||||||||
|
|
||||||||||||
| // Pre-fix, replay sends only its snapshot ([replayed]) and blanket-marks the deferred packet | ||||||||||||
| // sent without ever transmitting it — one send. The drain loop must transmit both (the | ||||||||||||
| // deferred packet is serialized through sendDataPacket, so it's distinct from `replayed`), | ||||||||||||
| // leaving nothing unsent for a later align to strand. | ||||||||||||
| const sent = dc.send.mock.calls.map(([d]) => d); | ||||||||||||
| expect(sent).toHaveLength(2); | ||||||||||||
| expect(sent[0]).toBe(replayed); | ||||||||||||
| expect(sent[1]).not.toBe(replayed); | ||||||||||||
| expect(buffer.getUnsent()).toHaveLength(0); | ||||||||||||
| }); | ||||||||||||
| }); | ||||||||||||
|
|
||||||||||||
| describe('reliable sends during teardown windows', () => { | ||||||||||||
| const makePacket = (byte: number) => | ||||||||||||
| new DataPacket({ | ||||||||||||
| kind: DataPacket_Kind.RELIABLE, | ||||||||||||
| value: { case: 'user', value: new UserPacket({ payload: new Uint8Array([byte]) }) }, | ||||||||||||
| }); | ||||||||||||
|
|
||||||||||||
| function stubEngine(engine: RTCEngine, dc: FakeDataChannel) { | ||||||||||||
| Object.assign(engine as unknown as Record<string, unknown>, { | ||||||||||||
| _isClosed: false, | ||||||||||||
| ensurePublisherConnected: vi.fn().mockResolvedValue(undefined), | ||||||||||||
| dataChannelForKind: vi.fn(() => dc), | ||||||||||||
| pcManager: { | ||||||||||||
| getMaxPublisherMessageSize: vi.fn(() => 64 * 1024 - 1), | ||||||||||||
| }, | ||||||||||||
| }); | ||||||||||||
| return (engine as unknown as { reliableMessageBuffer: DataPacketBuffer }) | ||||||||||||
| .reliableMessageBuffer; | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| it('resolves and queues the packet for replay when the wait is torn down transiently', async () => { | ||||||||||||
| const engine = new RTCEngine(roomOptionDefaults); | ||||||||||||
| const dc = new FakeDataChannel(); | ||||||||||||
| const buffer = stubEngine(engine, dc); | ||||||||||||
|
|
||||||||||||
| // Park the send on a full buffer, then invalidate the channel (reconnect/replacement). | ||||||||||||
| dc.bufferedAmount = 2 * 1024 * 1024; | ||||||||||||
| const send = engine.sendDataPacket(makePacket(1), DataChannelKind.RELIABLE); | ||||||||||||
| await tick(); | ||||||||||||
| ( | ||||||||||||
| engine as unknown as { invalidateDataChannelWaiters: (reason: string) => void } | ||||||||||||
| ).invalidateDataChannelWaiters('data channels recreated'); | ||||||||||||
|
Comment on lines
+442
to
+448
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: To make sure I understand properly, this is simulating a full reconnect?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. any condition in which the data channels would get recreated, this can also happen on resume, see client-sdk-js/src/room/RTCEngine.ts Lines 1511 to 1515 in cc79fd7
|
||||||||||||
|
|
||||||||||||
| // The send must not surface the teardown — the packet is queued for the resume replay. | ||||||||||||
| await expect(send).resolves.toBeUndefined(); | ||||||||||||
| expect(dc.send).not.toHaveBeenCalled(); | ||||||||||||
| expect(buffer.length).toBe(1); | ||||||||||||
| expect(buffer.getAll()[0].sent).toBe(false); | ||||||||||||
|
|
||||||||||||
| // The replay then delivers it. | ||||||||||||
| dc.bufferedAmount = 0; | ||||||||||||
| await ( | ||||||||||||
| engine as unknown as { resendReliableMessagesForResume: (seq: number) => Promise<void> } | ||||||||||||
| ).resendReliableMessagesForResume(0); | ||||||||||||
| expect(dc.send).toHaveBeenCalledTimes(1); | ||||||||||||
| expect(buffer.getAll()[0].sent).toBe(true); | ||||||||||||
| }); | ||||||||||||
|
|
||||||||||||
| it('still rejects when the engine is closed while waiting', async () => { | ||||||||||||
| const engine = new RTCEngine(roomOptionDefaults); | ||||||||||||
| const dc = new FakeDataChannel(); | ||||||||||||
| stubEngine(engine, dc); | ||||||||||||
|
|
||||||||||||
| dc.bufferedAmount = 2 * 1024 * 1024; | ||||||||||||
| const send = engine.sendDataPacket(makePacket(1), DataChannelKind.RELIABLE); | ||||||||||||
| await tick(); | ||||||||||||
| Object.assign(engine as unknown as Record<string, unknown>, { _isClosed: true }); | ||||||||||||
| ( | ||||||||||||
| engine as unknown as { invalidateDataChannelWaiters: (reason: string) => void } | ||||||||||||
| ).invalidateDataChannelWaiters('engine closed'); | ||||||||||||
|
|
||||||||||||
| await expect(send).rejects.toBeInstanceOf(UnexpectedConnectionState); | ||||||||||||
| expect(dc.send).not.toHaveBeenCalled(); | ||||||||||||
| }); | ||||||||||||
|
|
||||||||||||
| it('queues without waiting while a reconnect attempt is in progress', async () => { | ||||||||||||
| const engine = new RTCEngine(roomOptionDefaults); | ||||||||||||
| const dc = new FakeDataChannel(); | ||||||||||||
| const buffer = stubEngine(engine, dc); | ||||||||||||
| Object.assign(engine as unknown as Record<string, unknown>, { attemptingReconnect: true }); | ||||||||||||
|
|
||||||||||||
| // Even with a full buffer, the send resolves immediately instead of parking. | ||||||||||||
| dc.bufferedAmount = 2 * 1024 * 1024; | ||||||||||||
| await expect( | ||||||||||||
| engine.sendDataPacket(makePacket(1), DataChannelKind.RELIABLE), | ||||||||||||
| ).resolves.toBeUndefined(); | ||||||||||||
| expect(dc.send).not.toHaveBeenCalled(); | ||||||||||||
| expect(buffer.length).toBe(1); | ||||||||||||
| expect(buffer.getAll()[0].sent).toBe(false); | ||||||||||||
| }); | ||||||||||||
| }); | ||||||||||||
|
|
||||||||||||
| describe('sendLossyBytes', () => { | ||||||||||||
| it('ensures the publisher is connected before sending (direct data-track path)', async () => { | ||||||||||||
|
Comment on lines
+499
to
+500
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: I only see one place where
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the refactor PR makes this a bit obsolete, so I'll hold off on adding this here now, but happy to add something in that regard on the refactor PR if you still see a gap there |
||||||||||||
| const engine = new RTCEngine(roomOptionDefaults); | ||||||||||||
| const dc = new FakeDataChannel(); | ||||||||||||
| // The channel only becomes available once the publisher connection has been established — | ||||||||||||
| // mirroring the lazily negotiated publisher case that Room's packetAvailable path hits. | ||||||||||||
| let connected = false; | ||||||||||||
| const ensurePublisherConnected = vi.fn(async () => { | ||||||||||||
| connected = true; | ||||||||||||
| }); | ||||||||||||
| Object.assign(engine as unknown as Record<string, unknown>, { | ||||||||||||
| _isClosed: false, | ||||||||||||
| ensurePublisherConnected, | ||||||||||||
| dataChannelForKind: vi.fn(() => (connected ? dc : undefined)), | ||||||||||||
| }); | ||||||||||||
|
|
||||||||||||
| await engine.sendLossyBytes(new Uint8Array([1]), DataChannelKind.DATA_TRACK_LOSSY, 'wait'); | ||||||||||||
|
|
||||||||||||
| expect(ensurePublisherConnected).toHaveBeenCalledWith(DataChannelKind.DATA_TRACK_LOSSY); | ||||||||||||
| expect(dc.send).toHaveBeenCalledTimes(1); | ||||||||||||
| }); | ||||||||||||
|
|
||||||||||||
| it('only counts LOSSY bytes into the byterate stat that tunes the lossy drop threshold', async () => { | ||||||||||||
| const engine = new RTCEngine(roomOptionDefaults); | ||||||||||||
| const dc = new FakeDataChannel(); | ||||||||||||
| Object.assign(engine as unknown as Record<string, unknown>, { | ||||||||||||
| _isClosed: false, | ||||||||||||
| ensurePublisherConnected: vi.fn().mockResolvedValue(undefined), | ||||||||||||
| dataChannelForKind: vi.fn(() => dc), | ||||||||||||
| }); | ||||||||||||
| const stat = () => | ||||||||||||
| (engine as unknown as { lossyDataStatCurrentBytes: number }).lossyDataStatCurrentBytes; | ||||||||||||
|
|
||||||||||||
| // Data-track traffic must not move the stat — it would inflate the LOSSY channel's | ||||||||||||
| // dynamically tuned drop threshold with traffic that channel never carries. | ||||||||||||
| await engine.sendLossyBytes(new Uint8Array(1000), DataChannelKind.DATA_TRACK_LOSSY, 'wait'); | ||||||||||||
| expect(stat()).toBe(0); | ||||||||||||
|
|
||||||||||||
| await engine.sendLossyBytes(new Uint8Array(100), DataChannelKind.LOSSY, 'drop'); | ||||||||||||
| expect(stat()).toBe(100); | ||||||||||||
| }); | ||||||||||||
| }); | ||||||||||||
|
|
||||||||||||
| describe('waitForBufferHeadroomWithLock', () => { | ||||||||||||
| it('rejects parked waiters and releases the lock when the data channels are invalidated', async () => { | ||||||||||||
| const engine = new RTCEngine(roomOptionDefaults); | ||||||||||||
| const dc = new FakeDataChannel(); | ||||||||||||
| Object.assign(engine as unknown as Record<string, unknown>, { | ||||||||||||
| _isClosed: false, | ||||||||||||
| dataChannelForKind: vi.fn(() => dc), | ||||||||||||
| }); | ||||||||||||
|
|
||||||||||||
| // Park a waiter: buffer above the reliable high-water mark, holding the headroom lock. | ||||||||||||
| dc.bufferedAmount = 2 * 1024 * 1024; | ||||||||||||
| const parked = engine.waitForBufferHeadroomWithLock(DataChannelKind.RELIABLE); | ||||||||||||
| // Swallow the expected rejection so it can't surface as unhandled before we assert on it. | ||||||||||||
| parked.catch(() => {}); | ||||||||||||
| await tick(); | ||||||||||||
|
|
||||||||||||
| // The channel object gets abandoned (e.g. createDataChannels on the Safari resume path). | ||||||||||||
| ( | ||||||||||||
| engine as unknown as { invalidateDataChannelWaiters: (reason: string) => void } | ||||||||||||
| ).invalidateDataChannelWaiters('data channels recreated'); | ||||||||||||
|
|
||||||||||||
| await expect(parked).rejects.toBeInstanceOf(UnexpectedConnectionState); | ||||||||||||
|
|
||||||||||||
| // The lock must be free again: a wait against the fresh, drained channel resolves instead | ||||||||||||
| // of queueing forever behind the stranded waiter. | ||||||||||||
| dc.bufferedAmount = 0; | ||||||||||||
| await expect( | ||||||||||||
| engine.waitForBufferHeadroomWithLock(DataChannelKind.RELIABLE), | ||||||||||||
| ).resolves.toBeUndefined(); | ||||||||||||
| }); | ||||||||||||
| }); | ||||||||||||
|
|
||||||||||||
| describe('handleDataChannelClose', () => { | ||||||||||||
| function stubCloseEnv( | ||||||||||||
| engine: RTCEngine, | ||||||||||||
|
|
||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: To make sure I understand, this test is exercising the case where a new data packet is enqueued when the data channel's fullness is above the high water mark?
If so, a suggested better test name:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, this tests that if another packet is enqueued while resuming (landing in our own intermediate buffer), but the replay has already started, the new packet should either not be marked as sent when the replay finishes