Fixed RTCEngine.addTrack leaking pendingTrackResolvers on cancellation#920
Open
adrian-niculescu wants to merge 1 commit intolivekit:mainfrom
Open
Fixed RTCEngine.addTrack leaking pendingTrackResolvers on cancellation#920adrian-niculescu wants to merge 1 commit intolivekit:mainfrom
adrian-niculescu wants to merge 1 commit intolivekit:mainfrom
Conversation
When the AddTrack request timed out via withDeadline (20s) or its caller coroutine was cancelled, the cid stayed in pendingTrackResolvers because suspendCancellableCoroutine had no invokeOnCancellation cleanup. A retry of the same track instance (same cid) hit the duplicate guard and threw DuplicateTrackException, blocking republish until the connection closed, the signal reconnected, or the server eventually replied for that cid. Added cont.invokeOnCancellation that removes the entry under the existing synchronized monitor, with an identity check so a stale handler cannot evict a freshly-registered resolver bound to the same cid. Added two regression tests in RTCEngineMockE2ETest covering the timeout and caller-cancellation paths.
🦋 Changeset detectedLatest commit: 8d335b3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
RTCEngine.addTrackregisters the publish continuation inpendingTrackResolvers[cid]but never cleans it up if the 20swithDeadlinefires or the calling coroutine is cancelled — thesuspendCancellableCoroutineblock has noinvokeOnCancellationhandler. As a result, retrying the sameLocalAudioTrack/LocalVideoTrack(sameMediaStreamTrack.id()→ samecid) hits the duplicate guard atRTCEngine.kt:395and throwsDuplicateTrackException, blocking republish until any of:cid(clears the entry viaonLocalTrackPublished),onClose→abortPendingPublishTracks), orFor lost AddTrack requests or dropped responses on a healthy signal, the mic/camera publish stays poisoned indefinitely.
Fix
Register a
cont.invokeOnCancellationinside the samesynchronized(pendingTrackResolvers)block that does the insert. The handler removes the entry only if it still references the same continuation (pendingTrackResolvers[cid] === cont) so a stale handler from a cancelled publish cannot evict a freshly-registered resolver bound to the samecid— necessary because the duplicate guard would still let the retry register before the previous handler runs in concurrent scenarios.Test plan
addTrackTimeoutDoesNotPoisonRetryandaddTrackCallerCancellationDoesNotPoisonRetryinRTCEngineMockE2ETest. Both unregister the mock auto-responder so the first publish actually times out / stays in flight, then assert that a same-cidretry resolves cleanly when the server eventually replies.Track with same ID local_cid has already been published!— confirming they catch the original bug.livekit-android-test:testDebugUnitTestsuite passes (242 tests)../gradlew spotlessApplyclean.