Conversation
This logic was not being called on stream abort, which meant that you would unintentionally "leak" data stream subscriptions by aborting
…ined cases When a participant disconnects, this should work fine if the subscription has already been aborted
…as unsuccessfully
🦋 Changeset detectedLatest commit: 49748e0 The changes in this PR will be included in the next version bump. 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 |
size-limit report 📦
|
…onStream subscribeRequest is no longer called directly within RemoteDataTrack, so the tests now match more closely how logically this works now
| } | ||
| const currentDescriptor = this.descriptors.get(sid); | ||
| if (currentDescriptor?.subscription.type === 'active') { | ||
| currentDescriptor.subscription.streamControllers.delete(streamController); |
There was a problem hiding this comment.
I think cleanup() is also trying to clean up the streamController, should we move the code to one place instead?
like having cleanup() does the cleaning, and onAbort() handles the signaling and cleanup
There was a problem hiding this comment.
Ah, good point - this is I think duplicative. I will remove this and let the copy in cleanup handle deleting the stream controller from the map instead. 👍
| currentDescriptor.subscription.streamControllers.delete(streamController); | ||
| } | ||
|
|
||
| streamController.error(DataTrackSubscribeError.cancelled()); |
There was a problem hiding this comment.
onAbort() is called after abort(), does streamController guarantee that it is not closed yet ?
and will it throw if streamController is closed already ?
There was a problem hiding this comment.
I think it should be ok, the mdn docs for the error method explicitly say that it can be called multiple times.
3 participants
A customer surfaced some issues with the
signalparameter passed to data track subscriptions - when this was passed and aborted during certain parts of the subscription / participant connection lifecycle outside the happy path, it was possible that things could get into an inconsistent state:Aborting after participant disconnect - the stream would throw a
Cannot close an errored readable streamerror in this case because the manager tried to clean up a subscription that the user had already manually aborted. So, make sure the stream controller gets removed from the list so it doesn't get double-closed.The data track is unpublished during the brief window of time between the sfu subscription being established and the readable stream finishing being setup. If this happened, the readable stream would hang forever isntead of explicitly aborting.
Abort signal listener not being cleaned up properly in all cases - if the manager closes the stream (the stream doesn't get into an error state, it is explicitly closed), then this wouldn't clean up the abort signal listener, so if a user triggered the abort signal later, it would cause an error to be throw.
These cases now all have explicit tests that test a few variations to ensure that this doesn't regress.