Start AudioStream _run task after the FFI handle is assigned#742
Start AudioStream _run task after the FFI handle is assigned#742apoorva-01 wants to merge 1 commit into
Conversation
If owned-stream creation raises, __init__ never assigns _ffi_handle, but the already-scheduled _run task still dereferences it and leaks an uncatchable AttributeError on the livekit logger. Creating the task last leaves no orphan on that path.
|
|
There was a problem hiding this comment.
🚩 Test may not be discovered by root pytest configuration
The root pyproject.toml sets testpaths = ["tests"] which points to the repo-root tests/ directory. The new test file lives in livekit-rtc/tests/, which is a separate directory. Unless pytest is invoked from within livekit-rtc/ or with an explicit path, this test may not be collected by the default pytest invocation from the repo root. Other tests in livekit-rtc/tests/ (e.g., test_audio.py) appear to be integration/E2E tests that may be run separately, so this might be intentional — but worth confirming the CI configuration discovers it.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Good flag, but CI does collect it. The test jobs run pytest tests/ livekit-rtc/tests/ with both paths explicit (tests.yml), which overrides the testpaths = ["tests"] default. It shows up green in this PR's run, e.g. the ubuntu-3.9 job: livekit-rtc/tests/test_audio_stream_init.py ..
AudioStream.__init__kicks off the_runtask before it creates the stream and setsself._ffi_handle. So if stream creation throws (e.g. the track got torn down when the participant dropped mid-setup),__init__raises like it should, but the_runtask is already running and trips over the unsetself._ffi_handlewithAttributeError: 'AudioStream' object has no attribute '_ffi_handle'. It leaks as an unretrieved task exception on thelivekitlogger, so the caller can't catch it.Fix is just to start the task after the handle is set. Nothing awaits in between so the order doesn't matter for correctness, and it lines up with
VideoStream, which already does it this way.Could also default
_ffi_handletoNoneand guard_is_event, but that leaves a_runtask spinning on a predicate that never matches, so I went with the reorder. Happy to switch if you'd rather. Test fails on the old order, passes now.closes #729