Skip to content

Commit 59104b7

Browse files
committed
fix: backport abandoned data stream writer drop from node-sdks a6f8a35
1 parent d21086f commit 59104b7

4 files changed

Lines changed: 116 additions & 1 deletion

File tree

livekit-rtc/livekit/rtc/data_stream.py

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -378,6 +378,21 @@ def __init__(
378378
def _provisional_timestamp(self) -> int:
379379
return int(datetime.datetime.now().timestamp() * 1000)
380380

381+
def _register_open(self) -> None:
382+
"""Records the freshly opened writer so that, if it is never closed,
383+
the room can drop its native handle on disconnect."""
384+
assert self._writer_handle is not None
385+
self._local_participant._open_stream_writers.add(self._writer_handle)
386+
387+
def _unregister_open(self) -> None:
388+
"""Forgets the writer because a close request has been issued for it.
389+
390+
The close consumes the handle on the native side, so it must no longer
391+
be dropped by the disconnect cleanup.
392+
"""
393+
if self._writer_handle is not None:
394+
self._local_participant._open_stream_writers.discard(self._writer_handle)
395+
381396
async def _wait_for_callback(
382397
self, req: proto_ffi.FfiRequest, callback_field: str, response_field: str
383398
) -> proto_ffi.FfiEvent:
@@ -403,6 +418,10 @@ async def aclose(
403418
if self._writer_handle is None:
404419
raise RuntimeError("Stream is not open")
405420
self._closed = True
421+
# unregister before sending: the request consumes the handle natively,
422+
# so it must not be dropped again by the disconnect cleanup even if the
423+
# close itself reports an error
424+
self._unregister_open()
406425
await self._send_close(reason=reason, attributes=attributes)
407426

408427
async def _send_close(self, *, reason: str, attributes: Optional[Dict[str, str]]) -> None:
@@ -461,6 +480,7 @@ async def _send_header(self) -> None:
461480
cb = await self._wait_for_callback(req, "text_stream_open", "text_stream_open")
462481
owned = cb.text_stream_open.writer
463482
self._writer_handle = owned.handle.id
483+
self._register_open()
464484
self._info = _text_stream_info_from_proto(owned.info)
465485
if self._info.size is None and self._total_size is not None:
466486
# total_size is not transmitted for text streams; keep it local
@@ -546,6 +566,7 @@ async def _send_header(self) -> None:
546566
cb = await self._wait_for_callback(req, "byte_stream_open", "byte_stream_open")
547567
owned = cb.byte_stream_open.writer
548568
self._writer_handle = owned.handle.id
569+
self._register_open()
549570
self._info = _byte_stream_info_from_proto(owned.info)
550571

551572
async def write(self, data: bytes) -> None:

livekit-rtc/livekit/rtc/participant.py

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -246,6 +246,14 @@ def __init__(
246246
self._room_queue = room_queue
247247
self._track_publications: dict[str, LocalTrackPublication] = {}
248248
self._rpc_handlers: Dict[str, RpcHandler] = {}
249+
# Handle ids of data stream writers that have been opened but not yet
250+
# closed. The FFI close request consumes the handle (take_handle), so
251+
# an entry is removed as soon as a close is sent for it; whatever is
252+
# left when the room disconnects is dropped by
253+
# _dispose_open_stream_writers(). Only the ids are kept — wrapping them
254+
# in FfiHandle would make GC drop a handle the close request already
255+
# consumed.
256+
self._open_stream_writers: set[int] = set()
249257

250258
@property
251259
def track_publications(self) -> Mapping[str, LocalTrackPublication]:
@@ -254,6 +262,22 @@ def track_publications(self) -> Mapping[str, LocalTrackPublication]:
254262
"""
255263
return self._track_publications
256264

265+
def _dispose_open_stream_writers(self) -> None:
266+
"""Drops the native writers of any data streams that were opened but
267+
never closed — a writer the caller abandoned, or one left behind by a
268+
failed write. Their handles would otherwise be held by the FFI server
269+
for the lifetime of the process.
270+
271+
The room is already gone at this point, so the handles are dropped
272+
directly rather than closed: there is no end-of-stream left to deliver.
273+
"""
274+
for writer_handle in self._open_stream_writers:
275+
try:
276+
FfiHandle(writer_handle).dispose()
277+
except Exception:
278+
logger.exception("failed to dispose data stream writer handle")
279+
self._open_stream_writers.clear()
280+
257281
async def publish_data(
258282
self,
259283
payload: Union[bytes, str],

livekit-rtc/livekit/rtc/room.py

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -686,6 +686,7 @@ async def disconnect(
686686

687687
await self._drain_rpc_invocation_tasks()
688688
self._error_stream_readers()
689+
self._dispose_open_stream_writers()
689690

690691
req = proto_ffi.FfiRequest()
691692
req.disconnect.room_handle = self._ffi_handle.handle # type: ignore
@@ -735,6 +736,7 @@ async def _listen_task(self) -> None:
735736
# Clean up any pending RPC invocation tasks
736737
await self._drain_rpc_invocation_tasks()
737738
self._error_stream_readers()
739+
self._dispose_open_stream_writers()
738740

739741
def _on_rpc_method_invocation(self, rpc_invocation: RpcMethodInvocationEvent) -> None:
740742
if self._local_participant is None:
@@ -1195,6 +1197,16 @@ def on_close() -> None:
11951197
self._byte_stream_readers[handle_id] = byte_reader
11961198
byte_stream_handler(byte_reader, opened.participant_identity)
11971199

1200+
def _dispose_open_stream_writers(self) -> None:
1201+
"""Drops the native writers of data streams left open at disconnect.
1202+
1203+
A writer releases its handle only when it is closed, so one the
1204+
application abandoned — or left behind after a failed write — would
1205+
otherwise be held by the FFI server for the life of the process.
1206+
"""
1207+
if self._local_participant is not None:
1208+
self._local_participant._dispose_open_stream_writers()
1209+
11981210
def _error_stream_readers(self) -> None:
11991211
"""Wakes up any pending stream reads with a StreamError on disconnect.
12001212

tests/rtc/test_e2e_data_streams.py

Lines changed: 59 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@
2626
import pytest
2727

2828
from livekit import api, rtc
29-
from livekit.rtc._ffi_client import FfiClient
29+
from livekit.rtc._ffi_client import FfiClient, FfiHandle
3030

3131

3232
def skip_if_no_credentials() -> Any:
@@ -73,6 +73,21 @@ def reader_is_subscribed(reader: Any) -> bool:
7373
return any(q is queue for q, _, _ in FfiClient.instance.queue._subscribers)
7474

7575

76+
def ffi_handle_is_dropped(handle_id: int) -> bool:
77+
"""True when the FFI server no longer holds ``handle_id``.
78+
79+
Probes by trying to drop it: ``FfiHandle.dispose()`` asserts that the
80+
native drop succeeded, so a handle that is already gone raises. Destructive
81+
when the handle *is* still held — it gets dropped — so only call this once,
82+
at the end of a test.
83+
"""
84+
try:
85+
FfiHandle(handle_id).dispose()
86+
except AssertionError:
87+
return True
88+
return False
89+
90+
7691
async def wait_until_unsubscribed(reader: Any, timeout: float = 5.0) -> None:
7792
deadline = asyncio.get_event_loop().time() + timeout
7893
while asyncio.get_event_loop().time() < deadline:
@@ -601,6 +616,49 @@ async def read() -> None:
601616
await asyncio.gather(receiver.disconnect(), sender.disconnect())
602617

603618

619+
@pytest.mark.asyncio
620+
@skip_if_no_credentials() # type: ignore[untyped-decorator]
621+
async def test_writer_handle_untracked_after_close() -> None:
622+
"""Closing a writer consumes its handle natively, so it must drop out of
623+
the open-writer set and not be dropped again at disconnect."""
624+
receiver, sender = await connect_rooms(unique_room_name("ds-writer-close"))
625+
try:
626+
local = sender.local_participant
627+
writer = await local.stream_text(topic="writer-close-topic")
628+
handle = writer._writer_handle
629+
assert handle is not None
630+
assert handle in local._open_stream_writers
631+
632+
await writer.write("data")
633+
await writer.aclose()
634+
635+
assert handle not in local._open_stream_writers
636+
finally:
637+
await asyncio.gather(receiver.disconnect(), sender.disconnect())
638+
639+
640+
@pytest.mark.asyncio
641+
@skip_if_no_credentials() # type: ignore[untyped-decorator]
642+
async def test_abandoned_writer_disposed_on_disconnect() -> None:
643+
"""A writer opened and never closed holds its native writer in the FFI
644+
handle table; disconnecting must drop it."""
645+
receiver, sender = await connect_rooms(unique_room_name("ds-writer-abandon"))
646+
try:
647+
writer = await sender.local_participant.stream_text(topic="writer-abandon-topic")
648+
handle = writer._writer_handle
649+
assert handle is not None
650+
651+
await writer.write("data") # abandoned: aclose() is never called
652+
653+
await sender.disconnect()
654+
655+
# asserted against the FFI handle table rather than the SDK's
656+
# bookkeeping, so this fails on an actually-leaked native writer
657+
assert ffi_handle_is_dropped(handle)
658+
finally:
659+
await receiver.disconnect()
660+
661+
604662
@pytest.mark.asyncio
605663
@skip_if_no_credentials() # type: ignore[untyped-decorator]
606664
async def test_send_file_round_trip(tmp_path: Any) -> None:

0 commit comments

Comments
 (0)