rm unneeded function, better cleanup#195
Conversation
stephen-derosa
requested review from
alan-george-lk and
xianshijing-lk
as code owners
There was a problem hiding this comment.
Pull request overview
This PR refines SubscriptionThreadDispatcher’s reader-thread lifecycle handling to avoid redundant restarts and to ensure data reader threads reliably tear down (especially when a data-track subscription is still in flight), aligning with the SDK’s threading model by preventing teardown hangs.
Changes:
- Add cancellation signaling for active data readers and ensure cancellation is set before closing/joining during unpublish and teardown.
- Introduce a self-cleanup path (
eraseDataReaderIfCurrent) so data reader threads can safely remove stale slots when they exit. - Skip redundant audio/video/data reader restarts when the same track SID is already active.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/tests/unit/test_subscription_thread_dispatcher.cpp | Adds unit tests covering data reader cancellation semantics and self-erase behavior. |
| src/subscription_thread_dispatcher.cpp | Implements cancellation-before-close, self-erase cleanup, and redundant reader-start suppression. |
| include/livekit/subscription_thread_dispatcher.h | Adds per-reader state (track_sid, cancelled) and declares the new cleanup helper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| auto existing = active_readers_.find(key); | ||
| if (existing != active_readers_.end() && existing->second.track_sid == track->sid()) { | ||
| LK_LOG_DEBUG( | ||
| "Skipping audio reader start for participant={} track_name={} because a " | ||
| "reader for sid={} is already active", | ||
| key.participant_identity, key.track_name, track->sid()); | ||
| return {}; | ||
| } |
There was a problem hiding this comment.
🔍 SID-based dedup silently prevents replaced callbacks from taking effect on active readers
The new early-return at src/subscription_thread_dispatcher.cpp:408-415 (audio) and src/subscription_thread_dispatcher.cpp:476-483 (video) skips restarting a reader when the incoming track has the same SID as the already-active reader. This is correct for suppressing redundant subscription events, but it introduces a subtle behavioral change: if a user calls setOnAudioFrameCallback to replace the callback while a reader is active, and then handleTrackSubscribed fires again with the same track SID (e.g. during a server reconnect), the reader continues using the old callback. Before this PR, startAudioReaderLocked would always extract the old reader and start a new one, picking up the replacement callback from audio_callbacks_. In practice this is unlikely to matter because handleTrackSubscribed with the same SID without an intervening handleTrackUnsubscribed is rare, but it is a semantic contract change on the startAudioReaderLocked/startVideoReaderLocked interface.
Was this helpful? React with 👍 or 👎 to provide feedback.
No description provided.