Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Setting local/remote descriptions in succession without waiting for the promises to resolve #1569

Closed
henbos opened this issue Sep 1, 2017 · 2 comments

Comments

@henbos
Copy link
Contributor

henbos commented Sep 1, 2017

The setting of an RTCSessionDescription says to "In parallel, start the process to apply description as described in ..." which does things based on the PC's state. This, along with other function calls or tasks queued that get and set the PC and other objects' states, would seem unsafe. Is there a lock implied, and does "in parallel" mean "queue a task to do in parallel"?

Doing multiple SRDs in succession without waiting for promises to resolve might have indeterminate effects due to races - even if we protect the PC's state with locks such that if a task is executed in any context all of it is executed before any other task is executed:

pc.ontrack = (e) => { console.log(e.streams[0].getTracks().length); }
pc.setRemoteDescription() such that stream x with track y is added
pc.setRemoteDescription() such that stream x with track y is removed

The spec says that when the process to apply the description is finished a task is queued to do things, including queuing tasks to fire ontrack events. Note the "queue a task to queue a task".

This means that even with locks protecting the PC's state the order of firing the first ontrack event and executing the task after the second SRD process finished is racey. In the first ontrack event, stream x could contain track y or it could be empty.

@taylor-b
Copy link
Contributor

taylor-b commented Sep 5, 2017

I think you may be missing the fact that createOffer, createAnswer, setLocalDescription and setRemoteDescription all use an "operations queue" that forces the algorithms to be executed serially. "Enqueue an operation" means "use operations queue" whereas "queue a task" doesn't. This isn't very clear; maybe we should file another issue to use terminology that's more differentiable?

Anyway, with the operations queue in mind, is this race still possible? I don't think so. After PR #1519 is merged, the "track" event is guaranteed to fire before the first SRD promise resolves. And the second SRD is guaranteed to only start after the first SRD promise resolves.

Note that there's still this issue, which is that some things happen "in parallel" that aren't allowed to: #1177

@henbos
Copy link
Contributor Author

henbos commented Sep 12, 2017

With "operations queue" and #1519 merged we don't have undefined parallel operations order or "queue an operation to queue a task". I think queuing on one of the queues to queue on the other could yield races but this should be safe now.

"SRD2" will always be executed after "SRD1" and "ontrack1" will never be affected by "SRD2" because it is now fired as part of the "SRD1" steps (not queued). Sweet, closing this issue!

@henbos henbos closed this as completed Sep 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants