From eccdb1186ba822cf2a4d13ea918408f3ca40a745 Mon Sep 17 00:00:00 2001 From: Hiroshi Horie <548776+hiroshihorie@users.noreply.github.com> Date: Thu, 29 Oct 2026 15:45:57 +0800 Subject: [PATCH 1/8] Fix waiting for the data channel send buffer to drain 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. --- .changes/data-channel-buffer-wait | 1 + lib/src/core/engine.dart | 36 +++++------ test/core/buffer_status_wait_test.dart | 89 ++++++++++++++++++++++++++ test/mock/datachannel_mock.dart | 8 ++- 4 files changed, 112 insertions(+), 22 deletions(-) create mode 100644 .changes/data-channel-buffer-wait create mode 100644 test/core/buffer_status_wait_test.dart diff --git a/.changes/data-channel-buffer-wait b/.changes/data-channel-buffer-wait new file mode 100644 index 000000000..aab8dd362 --- /dev/null +++ b/.changes/data-channel-buffer-wait @@ -0,0 +1 @@ +patch type="fixed" "Fix data channel sends that could hang after a full reconnect, keep waiting after the engine closed, or leak a listener while waiting for the send buffer to drain" diff --git a/lib/src/core/engine.dart b/lib/src/core/engine.dart index 0f46a92f8..c129e5491 100644 --- a/lib/src/core/engine.dart +++ b/lib/src/core/engine.dart @@ -395,29 +395,22 @@ class Engine extends Disposable with EventsEmittable { } Future waitForBufferStatusLow(Reliability kind) async { - final Completer completer = Completer(); + if (isBufferStatusLow(kind) == true) return; - if (isBufferStatusLow(kind) == true) { - completer.complete(); - } else { - onClosing() { - if (!completer.isCompleted) { - completer.completeError('Engine disconnected'); - } - } - - events.once((e) => onClosing()); - - while (!completer.isCompleted && !_dcBufferStatus[kind]!) { - await Future.delayed(const Duration(milliseconds: 10)); - } - if (completer.isCompleted) { - return; + var closing = false; + final cancelClosingListener = events.once((_) => closing = true); + try { + // The cached status only changes on a send or on the channel's low-buffer event. A channel + // that a full reconnect replaced fires neither, so the live channel is checked as well. + while (!closing && !_dcBufferStatus[kind]! && isBufferStatusLow(kind) != true) { + await Future.delayed(const Duration(milliseconds: 10)); } - completer.complete(); + } finally { + await cancelClosingListener?.call(); + } + if (closing) { + return Future.error('Engine disconnected'); } - - return completer.future; } Future _resendReliableMessagesForResume(int lastMessageSeq) async { @@ -849,6 +842,8 @@ class Engine extends Disposable with EventsEmittable { ); // _onDCStateUpdated(Reliability.lossy, state) _lossyDCPub?.bufferedAmountLowThreshold = 2 * 1024 * 1024; + // A new channel starts empty, whatever the one it replaces had buffered. + _dcBufferStatus[Reliability.lossy] = true; _lossyDCPub?.onBufferedAmountLow = (_) { _dcBufferStatus[Reliability.lossy] = (_lossyDCPub!.bufferedAmount! <= _lossyDCPub!.bufferedAmountLowThreshold!); }; @@ -872,6 +867,7 @@ class Engine extends Disposable with EventsEmittable { ), ); _reliableDCPub?.bufferedAmountLowThreshold = 2 * 1024 * 1024; + _dcBufferStatus[Reliability.reliable] = true; _reliableDCPub?.onBufferedAmountLow = (_) { _dcBufferStatus[Reliability.reliable] = (_reliableDCPub!.bufferedAmount! <= _reliableDCPub!.bufferedAmountLowThreshold!); diff --git a/test/core/buffer_status_wait_test.dart b/test/core/buffer_status_wait_test.dart new file mode 100644 index 000000000..50b82ff8a --- /dev/null +++ b/test/core/buffer_status_wait_test.dart @@ -0,0 +1,89 @@ +// Copyright 2026 LiveKit, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import 'package:flutter_test/flutter_test.dart'; + +import 'package:livekit_client/livekit_client.dart'; +import 'package:livekit_client/src/internal/events.dart'; +import '../mock/e2e_container.dart'; +import '../mock/peerconnection_mock.dart'; + +const _full = 10 * 1024 * 1024; + +void main() { + late E2EContainer container; + + setUp(() async { + resetMockDataChannels(); + container = E2EContainer(); + await container.connectRoom(); + }); + + tearDown(() async { + await container.dispose(); + }); + + void setBufferedAmount(int? amount) { + for (final dc in mockDataChannels) { + dc.fakeBufferedAmount = amount; + } + } + + /// Fills the reliable channel and sends once, so the engine's cached status also says it is full. + Future fillReliableChannel() async { + // The mock loops published data back into this room as if a remote sent it, which the room + // rejects. These tests only need the send to refresh the engine's cached status. + for (final dc in mockDataChannels) { + dc.onMessageSend = null; + } + setBufferedAmount(_full); + await container.room.localParticipant!.publishData([1], reliable: true); + expect(container.engine.isBufferStatusLow(Reliability.reliable), isFalse); + } + + test('a wait ends once the channel is empty, even without a low-buffer event', () async { + await fillReliableChannel(); + final wait = container.engine.waitForBufferStatusLow(Reliability.reliable); + + // An empty channel that never reports the drain, as when a full reconnect replaces the + // channel the wait started on. + setBufferedAmount(null); + + await wait.timeout(const Duration(seconds: 1)); + }); + + test('a wait does not leave its closing listener behind', () async { + final before = container.engine.events.listeners.length; + await fillReliableChannel(); + final wait = container.engine.waitForBufferStatusLow(Reliability.reliable); + + setBufferedAmount(null); + for (final dc in mockDataChannels) { + dc.onBufferedAmountLow?.call(0); + } + await wait.timeout(const Duration(seconds: 1)); + + expect(container.engine.events.listeners.length, before); + }); + + test('a wait still fails when the engine closes', () async { + await fillReliableChannel(); + final wait = container.engine.waitForBufferStatusLow(Reliability.reliable); + final outcome = wait.then((_) => 'resolved', onError: (Object e) => e); + + container.engine.events.emit(EngineClosingEvent()); + + expect(await outcome.timeout(const Duration(seconds: 2)), isNot('resolved')); + }); +} diff --git a/test/mock/datachannel_mock.dart b/test/mock/datachannel_mock.dart index e63121f83..e015b8630 100644 --- a/test/mock/datachannel_mock.dart +++ b/test/mock/datachannel_mock.dart @@ -50,11 +50,15 @@ class MockDataChannel extends RTCDataChannel { // ignore: overridden_fields int? bufferedAmountLowThreshold = 0; + /// Overrides the reported buffered amount, to simulate a channel that hasn't drained. Defaults to + /// the low threshold, i.e. never above it. + int? fakeBufferedAmount; + @override - int? get bufferedAmount => bufferedAmountLowThreshold; + int? get bufferedAmount => fakeBufferedAmount ?? bufferedAmountLowThreshold; @override - Future getBufferedAmount() => Future.value(bufferedAmountLowThreshold); + Future getBufferedAmount() => Future.value(bufferedAmount); @override int? get id => _id; From eaa46889c25cf1c447d220ce51bc9ad33840bed0 Mon Sep 17 00:00:00 2001 From: Hiroshi Horie <548776+hiroshihorie@users.noreply.github.com> Date: Thu, 29 Oct 2026 17:13:23 +0800 Subject: [PATCH 2/8] Announce engine closure on terminal lifecycle paths --- .changes/engine-terminal-closure | 1 + lib/src/core/engine.dart | 21 +++++- test/core/engine_closing_test.dart | 116 +++++++++++++++++++++++++++++ 3 files changed, 134 insertions(+), 4 deletions(-) create mode 100644 .changes/engine-terminal-closure create mode 100644 test/core/engine_closing_test.dart diff --git a/.changes/engine-terminal-closure b/.changes/engine-terminal-closure new file mode 100644 index 000000000..1a8d6e5ce --- /dev/null +++ b/.changes/engine-terminal-closure @@ -0,0 +1 @@ +patch type="fixed" "Announce engine closure when a room is disposed or reconnection gives up, and reject data channel buffer waits started after closure" diff --git a/lib/src/core/engine.dart b/lib/src/core/engine.dart index 0f46a92f8..5ffdaa966 100644 --- a/lib/src/core/engine.dart +++ b/lib/src/core/engine.dart @@ -242,7 +242,7 @@ class Engine extends Disposable with EventsEmittable { _setUpSignalListeners(); onDispose(() async { - _isClosed = true; + _markClosed(); await cleanUp(); await events.dispose(); await _signalListener.dispose(); @@ -395,6 +395,11 @@ class Engine extends Disposable with EventsEmittable { } Future waitForBufferStatusLow(Reliability kind) async { + // A wait started after closure cannot observe the closing event. + if (_isClosed || isDisposed) { + throw 'Engine disconnected'; + } + final Completer completer = Completer(); if (isBufferStatusLow(kind) == true) { @@ -1129,7 +1134,7 @@ class Engine extends Disposable with EventsEmittable { if (_reconnectAttempts >= _reconnectCount) { logger.fine('reconnectAttempts exceeded, disconnecting...'); - _isClosed = true; + _markClosed(); await cleanUp(); events.emit( @@ -1241,6 +1246,7 @@ class Engine extends Disposable with EventsEmittable { unawaited(handleReconnect(ClientDisconnectReason.reconnectRetry)); } else { logger.fine('attemptReconnect: disconnecting...'); + _markClosed(); // clean up before emitting, room's EngineDisconnectedEvent handler // drops the event while fullReconnectOnNext is still true and // cleanUp() is what resets it @@ -1644,11 +1650,18 @@ class Engine extends Disposable with EventsEmittable { events.emit(EngineRoomMovedEvent(response: event.response)); }); + // Announce terminal closure before tearing down transports or event listeners. + // A full reconnect also disposes transports, but must keep the engine open. + void _markClosed() { + if (_isClosed) return; + _isClosed = true; + events.emit(EngineClosingEvent()); + } + Future disconnect({ DisconnectReason reason = DisconnectReason.clientInitiated, }) async { - _isClosed = true; - events.emit(EngineClosingEvent()); + _markClosed(); if (connectionState == ConnectionState.connected) { await signalClient.sendLeave(); } else { diff --git a/test/core/engine_closing_test.dart b/test/core/engine_closing_test.dart new file mode 100644 index 000000000..fbde44407 --- /dev/null +++ b/test/core/engine_closing_test.dart @@ -0,0 +1,116 @@ +// Copyright 2026 LiveKit, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +@Timeout(Duration(seconds: 5)) +library; + +import 'package:flutter_test/flutter_test.dart'; + +import 'package:livekit_client/livekit_client.dart'; +import 'package:livekit_client/src/core/engine.dart'; +import 'package:livekit_client/src/internal/events.dart'; +import 'package:livekit_client/src/types/internal.dart'; +import '../mock/e2e_container.dart'; +import '../mock/peerconnection_mock.dart'; + +void main() { + TestWidgetsFlutterBinding.ensureInitialized(); + + late E2EContainer container; + late List events; + + setUp(() async { + resetMockDataChannels(); + container = E2EContainer(); + await container.connectRoom(); + events = []; + container.engine.events.listen(events.add); + }); + + tearDown(() async { + await container.dispose(); + }); + + test('disposing the room announces engine closure', () async { + await container.room.dispose(); + + expect(events.whereType(), hasLength(1)); + expect(container.engine.isClosed, isTrue); + }); + + test('an unrecoverable reconnect announces closure before disconnection', () async { + container.wsConnector.connectError = CertificatePinningException( + 'Certificate pin mismatch', + host: 'www.example.com', + ); + final disconnected = container.engine.events.waitFor( + duration: const Duration(seconds: 1), + ); + + await container.engine.attemptReconnect(ClientDisconnectReason.reconnectRetry); + expect((await disconnected).reason, DisconnectReason.signalingConnectionFailure); + + expect(container.engine.isClosed, isTrue); + expect(events.whereType(), hasLength(1)); + expect( + events.indexWhere((event) => event is EngineClosingEvent), + lessThan(events.indexWhere((event) => event is EngineDisconnectedEvent)), + ); + }); + + test('exhausting reconnect attempts announces engine closure', () async { + // A generic resume failure stays recoverable without escalating to a full reconnect. + container.wsConnector.connectError = Exception('temporary connection failure'); + final disconnected = container.engine.events.waitFor( + duration: const Duration(seconds: 1), + ); + + // Drive the attempts directly rather than waiting through the retry delays. + for (var i = 0; i < defaultRetryDelaysInMs.length; i++) { + await container.engine.attemptReconnect(ClientDisconnectReason.reconnectRetry); + } + expect((await disconnected).reason, DisconnectReason.reconnectAttemptsExceeded); + + expect(container.engine.isClosed, isTrue); + expect(events.whereType(), hasLength(1)); + expect( + events.indexWhere((event) => event is EngineClosingEvent), + lessThan(events.indexWhere((event) => event is EngineDisconnectedEvent)), + ); + }); + + test('a recoverable reconnect does not announce engine closure', () async { + container.wsConnector.connectError = Exception('temporary connection failure'); + + await container.engine.attemptReconnect(ClientDisconnectReason.reconnectRetry); + + expect(container.engine.isClosed, isFalse); + expect(events.whereType(), isEmpty); + expect(events.whereType(), isEmpty); + }); + + test('a buffer wait started after disposal rejects even when the channel reports low', () async { + await container.room.dispose(); + + await expectLater(container.engine.waitForBufferStatusLow(Reliability.reliable), throwsA('Engine disconnected')); + }); + + test('disconnect followed by disposal announces closure only once', () async { + await container.engine.disconnect(); + await expectLater(container.engine.waitForBufferStatusLow(Reliability.reliable), throwsA('Engine disconnected')); + await container.room.dispose(); + + expect(events.whereType(), hasLength(1)); + }); +} From 6f0346bd0091a2c12beeb3559bd61fe5d3ed2dc5 Mon Sep 17 00:00:00 2001 From: Hiroshi Horie <548776+hiroshihorie@users.noreply.github.com> Date: Fri, 30 Oct 2026 03:52:55 +0800 Subject: [PATCH 3/8] Signal engine closure before disposal callbacks --- .changes/engine-terminal-closure | 2 +- lib/src/core/engine.dart | 17 ++++++--- lib/src/core/room.dart | 7 ++++ test/core/engine_closing_test.dart | 57 ++++++++++++++++++++++++++++++ 4 files changed, 77 insertions(+), 6 deletions(-) diff --git a/.changes/engine-terminal-closure b/.changes/engine-terminal-closure index 1a8d6e5ce..0429b4483 100644 --- a/.changes/engine-terminal-closure +++ b/.changes/engine-terminal-closure @@ -1 +1 @@ -patch type="fixed" "Announce engine closure when a room is disposed or reconnection gives up, and reject data channel buffer waits started after closure" +patch type="fixed" "Announce engine closure before room disposal callbacks and when reconnection gives up, and reject data channel buffer waits started after closure" diff --git a/lib/src/core/engine.dart b/lib/src/core/engine.dart index 5ffdaa966..198775eeb 100644 --- a/lib/src/core/engine.dart +++ b/lib/src/core/engine.dart @@ -242,7 +242,6 @@ class Engine extends Disposable with EventsEmittable { _setUpSignalListeners(); onDispose(() async { - _markClosed(); await cleanUp(); await events.dispose(); await _signalListener.dispose(); @@ -250,6 +249,13 @@ class Engine extends Disposable with EventsEmittable { }); } + @override + Future dispose() { + // Disposal callbacks run in reverse registration order and may await engine work. + markClosed(); + return super.dispose(); + } + Future connect( String url, String token, { @@ -1134,7 +1140,7 @@ class Engine extends Disposable with EventsEmittable { if (_reconnectAttempts >= _reconnectCount) { logger.fine('reconnectAttempts exceeded, disconnecting...'); - _markClosed(); + markClosed(); await cleanUp(); events.emit( @@ -1246,7 +1252,7 @@ class Engine extends Disposable with EventsEmittable { unawaited(handleReconnect(ClientDisconnectReason.reconnectRetry)); } else { logger.fine('attemptReconnect: disconnecting...'); - _markClosed(); + markClosed(); // clean up before emitting, room's EngineDisconnectedEvent handler // drops the event while fullReconnectOnNext is still true and // cleanUp() is what resets it @@ -1652,7 +1658,8 @@ class Engine extends Disposable with EventsEmittable { // Announce terminal closure before tearing down transports or event listeners. // A full reconnect also disposes transports, but must keep the engine open. - void _markClosed() { + @internal + void markClosed() { if (_isClosed) return; _isClosed = true; events.emit(EngineClosingEvent()); @@ -1661,7 +1668,7 @@ class Engine extends Disposable with EventsEmittable { Future disconnect({ DisconnectReason reason = DisconnectReason.clientInitiated, }) async { - _markClosed(); + markClosed(); if (connectionState == ConnectionState.connected) { await signalClient.sendLeave(); } else { diff --git a/lib/src/core/room.dart b/lib/src/core/room.dart index d9ca2843b..3d2605662 100644 --- a/lib/src/core/room.dart +++ b/lib/src/core/room.dart @@ -242,6 +242,13 @@ class Room extends DisposableChangeNotifier with EventsEmittable { }); } + @override + Future dispose() { + // Room cleanup tears down transports before Engine.dispose() is reached. + engine.markClosed(); + return super.dispose(); + } + /// prepareConnection should be called as soon as the page is loaded, in order /// to speed up the connection attempt. This function will /// - perform DNS resolution and pre-warm the DNS cache diff --git a/test/core/engine_closing_test.dart b/test/core/engine_closing_test.dart index fbde44407..dbcbf3934 100644 --- a/test/core/engine_closing_test.dart +++ b/test/core/engine_closing_test.dart @@ -15,6 +15,8 @@ @Timeout(Duration(seconds: 5)) library; +import 'dart:async'; + import 'package:flutter_test/flutter_test.dart'; import 'package:livekit_client/livekit_client.dart'; @@ -49,6 +51,61 @@ void main() { expect(container.engine.isClosed, isTrue); }); + test('room closure is announced while the first transport teardown is blocked', () async { + final teardownStarted = Completer(); + final releaseTeardown = Completer(); + container.engine.publisher!.onDispose(() async { + teardownStarted.complete(); + await releaseTeardown.future; + }); + + final disposing = container.room.dispose(); + await teardownStarted.future; + final closedDuringTeardown = container.engine.isClosed; + final closingEventsDuringTeardown = events.whereType().length; + final emitterDisposedDuringTeardown = container.engine.events.isDisposed; + releaseTeardown.complete(); + await disposing; + + expect(closedDuringTeardown, isTrue); + expect(closingEventsDuringTeardown, 1); + expect(emitterDisposedDuringTeardown, isFalse); + }); + + for (final owner in ['room', 'engine']) { + test('$owner is marked closed before later disposal callbacks run', () async { + final disposable = owner == 'room' ? container.room : container.engine; + bool? closedInCallback; + disposable.onDispose(() async { + closedInCallback = container.engine.isClosed; + }); + + if (owner == 'room') { + await container.room.dispose(); + } else { + await container.engine.dispose(); + } + + expect(closedInCallback, isTrue); + expect(events.whereType(), hasLength(1)); + }); + } + + test('engine closure is delivered while disposing with its emitter still alive', () async { + bool? engineDisposedWhenDelivered; + bool? emitterDisposedWhenDelivered; + container.engine.events.once((_) { + engineDisposedWhenDelivered = container.engine.isDisposed; + emitterDisposedWhenDelivered = container.engine.events.isDisposed; + }); + + await container.engine.dispose(); + + expect(engineDisposedWhenDelivered, isTrue); + expect(emitterDisposedWhenDelivered, isFalse); + expect(container.engine.events.isDisposed, isTrue); + }); + test('an unrecoverable reconnect announces closure before disconnection', () async { container.wsConnector.connectError = CertificatePinningException( 'Certificate pin mismatch', From bb1c5b8c2b81adff031f83395da23ec69ec04d75 Mon Sep 17 00:00:00 2001 From: Hiroshi Horie <548776+hiroshihorie@users.noreply.github.com> Date: Fri, 30 Oct 2026 04:05:53 +0800 Subject: [PATCH 4/8] Wait on the engine's closed state instead of a closing listener 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. --- lib/src/core/engine.dart | 23 +++++------------------ test/core/buffer_status_wait_test.dart | 5 ++--- 2 files changed, 7 insertions(+), 21 deletions(-) diff --git a/lib/src/core/engine.dart b/lib/src/core/engine.dart index f2a52eac5..57ce8ed4b 100644 --- a/lib/src/core/engine.dart +++ b/lib/src/core/engine.dart @@ -401,27 +401,14 @@ class Engine extends Disposable with EventsEmittable { } Future waitForBufferStatusLow(Reliability kind) async { - // A wait started after closure cannot observe the closing event. + // The cached status only changes on a send or on the channel's low-buffer event. A channel + // that a full reconnect replaced fires neither, so the live channel is checked as well. + while (!_isClosed && !isDisposed && !_dcBufferStatus[kind]! && isBufferStatusLow(kind) != true) { + await Future.delayed(const Duration(milliseconds: 10)); + } if (_isClosed || isDisposed) { throw 'Engine disconnected'; } - - if (isBufferStatusLow(kind) == true) return; - - var closing = false; - final cancelClosingListener = events.once((_) => closing = true); - try { - // The cached status only changes on a send or on the channel's low-buffer event. A channel - // that a full reconnect replaced fires neither, so the live channel is checked as well. - while (!closing && !_dcBufferStatus[kind]! && isBufferStatusLow(kind) != true) { - await Future.delayed(const Duration(milliseconds: 10)); - } - } finally { - await cancelClosingListener?.call(); - } - if (closing) { - return Future.error('Engine disconnected'); - } } Future _resendReliableMessagesForResume(int lastMessageSeq) async { diff --git a/test/core/buffer_status_wait_test.dart b/test/core/buffer_status_wait_test.dart index 50b82ff8a..c965047f1 100644 --- a/test/core/buffer_status_wait_test.dart +++ b/test/core/buffer_status_wait_test.dart @@ -15,7 +15,6 @@ import 'package:flutter_test/flutter_test.dart'; import 'package:livekit_client/livekit_client.dart'; -import 'package:livekit_client/src/internal/events.dart'; import '../mock/e2e_container.dart'; import '../mock/peerconnection_mock.dart'; @@ -63,7 +62,7 @@ void main() { await wait.timeout(const Duration(seconds: 1)); }); - test('a wait does not leave its closing listener behind', () async { + test('a wait does not leave an engine listener behind', () async { final before = container.engine.events.listeners.length; await fillReliableChannel(); final wait = container.engine.waitForBufferStatusLow(Reliability.reliable); @@ -82,7 +81,7 @@ void main() { final wait = container.engine.waitForBufferStatusLow(Reliability.reliable); final outcome = wait.then((_) => 'resolved', onError: (Object e) => e); - container.engine.events.emit(EngineClosingEvent()); + container.engine.markClosed(); expect(await outcome.timeout(const Duration(seconds: 2)), isNot('resolved')); }); From 60bbac96ea9a1c6e650694d229a4d85127db448a Mon Sep 17 00:00:00 2001 From: Hiroshi Horie <548776+hiroshihorie@users.noreply.github.com> Date: Tue, 3 Nov 2026 01:07:14 +0800 Subject: [PATCH 5/8] Reject buffer waits with a LiveKit exception instead of a string 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. --- .changes/buffer-wait-typed-error | 1 + lib/src/core/engine.dart | 4 ++-- test/core/engine_closing_test.dart | 10 ++++++++-- 3 files changed, 11 insertions(+), 4 deletions(-) create mode 100644 .changes/buffer-wait-typed-error diff --git a/.changes/buffer-wait-typed-error b/.changes/buffer-wait-typed-error new file mode 100644 index 000000000..a83e7896e --- /dev/null +++ b/.changes/buffer-wait-typed-error @@ -0,0 +1 @@ +patch type="fixed" "Data stream writes on a closed engine fail with UnexpectedConnectionState instead of a plain string error" \ No newline at end of file diff --git a/lib/src/core/engine.dart b/lib/src/core/engine.dart index 757cf6f4b..118cb247b 100644 --- a/lib/src/core/engine.dart +++ b/lib/src/core/engine.dart @@ -425,7 +425,7 @@ class Engine extends Disposable with EventsEmittable { Future waitForBufferStatusLow(Reliability kind) async { // A wait started after closure cannot observe the closing event. if (_isClosed || isDisposed) { - throw 'Engine disconnected'; + throw UnexpectedConnectionState('Engine disconnected'); } final Completer completer = Completer(); @@ -435,7 +435,7 @@ class Engine extends Disposable with EventsEmittable { } else { onClosing() { if (!completer.isCompleted) { - completer.completeError('Engine disconnected'); + completer.completeError(UnexpectedConnectionState('Engine disconnected')); } } diff --git a/test/core/engine_closing_test.dart b/test/core/engine_closing_test.dart index dbcbf3934..7c5de99f3 100644 --- a/test/core/engine_closing_test.dart +++ b/test/core/engine_closing_test.dart @@ -160,12 +160,18 @@ void main() { test('a buffer wait started after disposal rejects even when the channel reports low', () async { await container.room.dispose(); - await expectLater(container.engine.waitForBufferStatusLow(Reliability.reliable), throwsA('Engine disconnected')); + await expectLater( + container.engine.waitForBufferStatusLow(Reliability.reliable), + throwsA(isA()), + ); }); test('disconnect followed by disposal announces closure only once', () async { await container.engine.disconnect(); - await expectLater(container.engine.waitForBufferStatusLow(Reliability.reliable), throwsA('Engine disconnected')); + await expectLater( + container.engine.waitForBufferStatusLow(Reliability.reliable), + throwsA(isA()), + ); await container.room.dispose(); expect(events.whereType(), hasLength(1)); From 64e9ccd6c668eef34f290983e3b29941fcb312eb Mon Sep 17 00:00:00 2001 From: Hiroshi Horie <548776+hiroshihorie@users.noreply.github.com> Date: Tue, 3 Nov 2026 13:27:39 +0800 Subject: [PATCH 6/8] Hold buffer waits while the engine reconnects 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. --- lib/src/core/engine.dart | 6 +++- test/core/buffer_status_wait_test.dart | 43 ++++++++++++++++++++++++++ test/mock/peerconnection_mock.dart | 6 ++++ 3 files changed, 54 insertions(+), 1 deletion(-) diff --git a/lib/src/core/engine.dart b/lib/src/core/engine.dart index d4981b938..8617bfb55 100644 --- a/lib/src/core/engine.dart +++ b/lib/src/core/engine.dart @@ -425,7 +425,11 @@ class Engine extends Disposable with EventsEmittable { Future waitForBufferStatusLow(Reliability kind) async { // The cached status only changes on a send or on the channel's low-buffer event. A channel // that a full reconnect replaced fires neither, so the live channel is checked as well. - while (!_isClosed && !isDisposed && !_dcBufferStatus[kind]! && isBufferStatusLow(kind) != true) { + // A full reconnect creates its empty channel before it finishes, and a send made in between + // is parked for a resume that never comes, so the wait holds until the reconnect is over. + while (!_isClosed && + !isDisposed && + (_isReconnecting || (!_dcBufferStatus[kind]! && isBufferStatusLow(kind) != true))) { await Future.delayed(const Duration(milliseconds: 10)); } if (_isClosed || isDisposed) { diff --git a/test/core/buffer_status_wait_test.dart b/test/core/buffer_status_wait_test.dart index c965047f1..ee3c64bc8 100644 --- a/test/core/buffer_status_wait_test.dart +++ b/test/core/buffer_status_wait_test.dart @@ -15,8 +15,10 @@ import 'package:flutter_test/flutter_test.dart'; import 'package:livekit_client/livekit_client.dart'; +import 'package:livekit_client/src/proto/livekit_models.pb.dart' as lk_models; import '../mock/e2e_container.dart'; import '../mock/peerconnection_mock.dart'; +import 'signal_client_test.dart' show joinResponse, offerResponse; const _full = 10 * 1024 * 1024; @@ -85,4 +87,45 @@ void main() { expect(await outcome.timeout(const Duration(seconds: 2)), isNot('resolved')); }); + + test('a wait holds until a full reconnect finishes', () async { + final engine = container.engine; + final ws = container.wsConnector; + await fillReliableChannel(); + + final marker = lk_models.DataPacket( + kind: lk_models.DataPacket_Kind.RELIABLE, + user: lk_models.UserPacket(payload: [42]), + ); + var markerSent = false; + onMockReliableChannelCreated = (dc) => dc.onMessageSend = (message) { + final packet = lk_models.DataPacket.fromBuffer(message.binary); + if (packet.hasUser() && packet.user.payload.length == 1 && packet.user.payload.first == 42) { + markerSent = true; + } + }; + + // What a stream writer does with each chunk. + final write = engine + .waitForBufferStatusLow(Reliability.reliable) + .then((_) => engine.sendDataPacket(marker, reliability: Reliability.reliable)); + + final previousHandlers = ws.handlers; + engine.fullReconnectOnNext = true; + ws.onDispose(); + for (var i = 0; i < 200 && identical(ws.handlers, previousHandlers); i++) { + await Future.delayed(const Duration(milliseconds: 10)); + } + await Future.delayed(const Duration(milliseconds: 1)); + ws.onData(joinResponse.writeToBuffer()); + + // The new channel exists and is empty, but the reconnect waits for the subscriber + // transport. A real one takes longer than the wait's poll interval. + await Future.delayed(const Duration(milliseconds: 300)); + expect(markerSent, isFalse, reason: 'the write went out before the reconnect finished'); + + ws.onData(offerResponse.writeToBuffer()); + await write.timeout(const Duration(seconds: 2)); + expect(markerSent, isTrue, reason: 'the write never reached the new channel'); + }); } diff --git a/test/mock/peerconnection_mock.dart b/test/mock/peerconnection_mock.dart index 65f3e41d4..5e31ad29e 100644 --- a/test/mock/peerconnection_mock.dart +++ b/test/mock/peerconnection_mock.dart @@ -35,12 +35,17 @@ MockDataChannel? findMockDataChannelByLabel(String label, {bool requireListener /// publisher↔subscriber loopback. List get mockDataChannels => _dataChannels; +/// Called with every reliable channel the SDK creates, so a test can watch the +/// channel a full reconnect creates from the moment it exists. +void Function(MockDataChannel dc)? onMockReliableChannelCreated; + /// Reset the global mock-channel list. Tests that create a fresh `E2EContainer` /// after an earlier one finished must call this before re-connecting, otherwise /// the publisher↔subscriber pairing (which only fires when the list reaches /// length 2) will not happen on the new channels. void resetMockDataChannels() { _dataChannels.clear(); + onMockReliableChannelCreated = null; } class MockPeerConnection extends RTCPeerConnection { @@ -239,6 +244,7 @@ a=rtpmap:32 MPV/90000 final dc = MockDataChannel(dataChannelDict.id, label); if (label == '_reliable') { _dataChannels.add(dc); + onMockReliableChannelCreated?.call(dc); } return dc; } From daf740dd901f077ccd8d884f38d92d71e9513f79 Mon Sep 17 00:00:00 2001 From: Hiroshi Horie <548776+hiroshihorie@users.noreply.github.com> Date: Tue, 3 Nov 2026 13:47:55 +0800 Subject: [PATCH 7/8] Wake buffer waits on state changes instead of polling 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. --- lib/src/core/engine.dart | 58 +++++++++++++++++--------- test/core/buffer_status_wait_test.dart | 58 ++++++++++++++++++-------- 2 files changed, 80 insertions(+), 36 deletions(-) diff --git a/lib/src/core/engine.dart b/lib/src/core/engine.dart index 8617bfb55..af32afa34 100644 --- a/lib/src/core/engine.dart +++ b/lib/src/core/engine.dart @@ -41,6 +41,7 @@ import '../publication/local.dart'; import '../support/disposable.dart'; import '../support/platform.dart' show lkPlatformIsTest, lkPlatformIs, PlatformType; import '../support/region_url_provider.dart'; +import '../support/reusable_completer.dart'; import '../support/websocket.dart'; import '../track/local/local.dart'; import '../track/local/video.dart'; @@ -178,6 +179,15 @@ class Engine extends Disposable with EventsEmittable { Reliability.lossy: false, }; + // Completed whenever something a buffer wait depends on changes: a buffer status, the reconnect + // state or closure. A wait checks its condition again each time it wakes. + final ReusableCompleter _sendStateChanged = ReusableCompleter(); + + void _setBufferStatus(Reliability kind, bool isLow) { + _dcBufferStatus[kind] = isLow; + _sendStateChanged.complete(); + } + List? _enabledPublishCodecs; List? get enabledPublishCodecs => _enabledPublishCodecs; @@ -197,7 +207,12 @@ class Engine extends Disposable with EventsEmittable { maxPacketCount: 1000, // max 1000 packets ); final TTLMap _reliableReceivedState = TTLMap(30000); - bool _isReconnecting = false; + bool _reconnecting = false; + bool get _isReconnecting => _reconnecting; + set _isReconnecting(bool value) { + _reconnecting = value; + _sendStateChanged.complete(); + } Completer? _publisherConnectionCompleter; @@ -422,18 +437,20 @@ class Engine extends Disposable with EventsEmittable { return null; } - Future waitForBufferStatusLow(Reliability kind) async { - // The cached status only changes on a send or on the channel's low-buffer event. A channel - // that a full reconnect replaced fires neither, so the live channel is checked as well. - // A full reconnect creates its empty channel before it finishes, and a send made in between - // is parked for a resume that never comes, so the wait holds until the reconnect is over. - while (!_isClosed && - !isDisposed && - (_isReconnecting || (!_dcBufferStatus[kind]! && isBufferStatusLow(kind) != true))) { - await Future.delayed(const Duration(milliseconds: 10)); - } - if (_isClosed || isDisposed) { - throw UnexpectedConnectionState('Engine disconnected'); + // A full reconnect creates its empty channel before it finishes, and a send made in between + // is parked for a resume that never comes, so the wait also holds until the reconnect is over. + Future waitForBufferStatusLow(Reliability kind) => + _waitUntil(() => !_isReconnecting && (_dcBufferStatus[kind]! || isBufferStatusLow(kind) == true)); + + // Resolves once [isReady] holds, checking it again each time [_sendStateChanged] fires. Fails + // as soon as the engine closes, even when [isReady] already holds. + Future _waitUntil(bool Function() isReady) async { + while (true) { + if (_isClosed || isDisposed) { + throw UnexpectedConnectionState('Engine disconnected'); + } + if (isReady()) return; + await _sendStateChanged.future; } } @@ -553,7 +570,7 @@ class Engine extends Disposable with EventsEmittable { logger.fine('sendDataPacket(label:${channel.label}, sequence:${packet.sequence})'); await channel.send(message); - _dcBufferStatus[reliability] = await channel.getBufferedAmount() <= channel.bufferedAmountLowThreshold!; + _setBufferStatus(reliability, await channel.getBufferedAmount() <= channel.bufferedAmountLowThreshold!); // Align buffer with WebRTC buffer for reliable packets if (reliability == Reliability.reliable) { @@ -867,9 +884,9 @@ class Engine extends Disposable with EventsEmittable { // _onDCStateUpdated(Reliability.lossy, state) _lossyDCPub?.bufferedAmountLowThreshold = 2 * 1024 * 1024; // A new channel starts empty, whatever the one it replaces had buffered. - _dcBufferStatus[Reliability.lossy] = true; + _setBufferStatus(Reliability.lossy, true); _lossyDCPub?.onBufferedAmountLow = (_) { - _dcBufferStatus[Reliability.lossy] = (_lossyDCPub!.bufferedAmount! <= _lossyDCPub!.bufferedAmountLowThreshold!); + _setBufferStatus(Reliability.lossy, _lossyDCPub!.bufferedAmount! <= _lossyDCPub!.bufferedAmountLowThreshold!); }; } catch (err) { logger.severe('[$objectId] createDataChannel() did throw $err'); @@ -891,10 +908,12 @@ class Engine extends Disposable with EventsEmittable { ), ); _reliableDCPub?.bufferedAmountLowThreshold = 2 * 1024 * 1024; - _dcBufferStatus[Reliability.reliable] = true; + _setBufferStatus(Reliability.reliable, true); _reliableDCPub?.onBufferedAmountLow = (_) { - _dcBufferStatus[Reliability.reliable] = - (_reliableDCPub!.bufferedAmount! <= _reliableDCPub!.bufferedAmountLowThreshold!); + _setBufferStatus( + Reliability.reliable, + _reliableDCPub!.bufferedAmount! <= _reliableDCPub!.bufferedAmountLowThreshold!, + ); }; } catch (err) { logger.severe('[$objectId] createDataChannel() did throw $err'); @@ -1682,6 +1701,7 @@ class Engine extends Disposable with EventsEmittable { void markClosed() { if (_isClosed) return; _isClosed = true; + _sendStateChanged.complete(); events.emit(EngineClosingEvent()); } diff --git a/test/core/buffer_status_wait_test.dart b/test/core/buffer_status_wait_test.dart index ee3c64bc8..8defae964 100644 --- a/test/core/buffer_status_wait_test.dart +++ b/test/core/buffer_status_wait_test.dart @@ -41,6 +41,22 @@ void main() { } } + /// Starts a full reconnect and answers its join, leaving it to wait for the subscriber offer. + /// The new, empty publisher channels exist from here on, but the reconnect has not finished. + Future startFullReconnect() async { + final ws = container.wsConnector; + final previousHandlers = ws.handlers; + container.engine.fullReconnectOnNext = true; + ws.onDispose(); + for (var i = 0; i < 200 && identical(ws.handlers, previousHandlers); i++) { + await Future.delayed(const Duration(milliseconds: 10)); + } + await Future.delayed(const Duration(milliseconds: 1)); + ws.onData(joinResponse.writeToBuffer()); + } + + void finishFullReconnect() => container.wsConnector.onData(offerResponse.writeToBuffer()); + /// Fills the reliable channel and sends once, so the engine's cached status also says it is full. Future fillReliableChannel() async { // The mock loops published data back into this room as if a remote sent it, which the room @@ -53,17 +69,35 @@ void main() { expect(container.engine.isBufferStatusLow(Reliability.reliable), isFalse); } - test('a wait ends once the channel is empty, even without a low-buffer event', () async { + test('a wait ends when a full reconnect replaces the full channel', () async { await fillReliableChannel(); final wait = container.engine.waitForBufferStatusLow(Reliability.reliable); - // An empty channel that never reports the drain, as when a full reconnect replaces the - // channel the wait started on. - setBufferedAmount(null); + // The replaced channel never reports a drain, only the new one is empty. + await startFullReconnect(); + finishFullReconnect(); await wait.timeout(const Duration(seconds: 1)); }); + test('a wait wakes as soon as the channel reports a drain', () async { + await fillReliableChannel(); + var done = false; + final wait = container.engine.waitForBufferStatusLow(Reliability.reliable).whenComplete(() => done = true); + await pumpEventQueue(); + expect(done, isFalse); + + setBufferedAmount(null); + for (final dc in mockDataChannels) { + dc.onBufferedAmountLow?.call(0); + } + + // Only zero-length timers run here, so a wait that polls on a timer would not have woken yet. + await pumpEventQueue(); + expect(done, isTrue); + await wait; + }); + test('a wait does not leave an engine listener behind', () async { final before = container.engine.events.listeners.length; await fillReliableChannel(); @@ -90,7 +124,6 @@ void main() { test('a wait holds until a full reconnect finishes', () async { final engine = container.engine; - final ws = container.wsConnector; await fillReliableChannel(); final marker = lk_models.DataPacket( @@ -110,21 +143,12 @@ void main() { .waitForBufferStatusLow(Reliability.reliable) .then((_) => engine.sendDataPacket(marker, reliability: Reliability.reliable)); - final previousHandlers = ws.handlers; - engine.fullReconnectOnNext = true; - ws.onDispose(); - for (var i = 0; i < 200 && identical(ws.handlers, previousHandlers); i++) { - await Future.delayed(const Duration(milliseconds: 10)); - } - await Future.delayed(const Duration(milliseconds: 1)); - ws.onData(joinResponse.writeToBuffer()); - - // The new channel exists and is empty, but the reconnect waits for the subscriber - // transport. A real one takes longer than the wait's poll interval. + // The new channel exists and is empty, but the reconnect waits for the subscriber transport. + await startFullReconnect(); await Future.delayed(const Duration(milliseconds: 300)); expect(markerSent, isFalse, reason: 'the write went out before the reconnect finished'); - ws.onData(offerResponse.writeToBuffer()); + finishFullReconnect(); await write.timeout(const Duration(seconds: 2)); expect(markerSent, isTrue, reason: 'the write never reached the new channel'); }); From 2cb361d8dd79f940c98963d7fecebb7dc0ef2ca4 Mon Sep 17 00:00:00 2001 From: Hiroshi Horie <548776+hiroshihorie@users.noreply.github.com> Date: Fri, 6 Nov 2026 15:23:04 +0800 Subject: [PATCH 8/8] Scope the changeset to data stream writes --- .changes/data-channel-buffer-wait | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changes/data-channel-buffer-wait b/.changes/data-channel-buffer-wait index aab8dd362..d0388efaf 100644 --- a/.changes/data-channel-buffer-wait +++ b/.changes/data-channel-buffer-wait @@ -1 +1 @@ -patch type="fixed" "Fix data channel sends that could hang after a full reconnect, keep waiting after the engine closed, or leak a listener while waiting for the send buffer to drain" +patch type="fixed" "Fix data stream writes that could hang after a full reconnect, carried on after the room disconnected, or leaked a listener while waiting for the send buffer to drain. Writes now also wait for a reconnect to finish"