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

Rewrite the createOffer algorithm to eliminate race conditions. #874

Closed

Conversation

taylor-b
Copy link
Contributor

First step in addressing #782. Though the resulting algorithm is pretty
ugly and could probably use some restructuring.

First step in addressing w3c#782. Though the resulting algorithm is pretty
ugly and could probably use some restructuring.
@taylor-b
Copy link
Contributor Author

taylor-b commented Oct 19, 2016

@jan-ivar, can you take a look? A couple things to note:

This only covers createOffer so far. I'll handle the other methods (createAnswer, SLD, SRD) later, but first I want to make sure I'm on the right path.

And as mentioned in the description, the algorithm ends up pretty ugly. It's essentially 4 levels of nesting, with GOTOs. If it was source code, I wouldn't "lgtm" it myself. Do you think it would be appropriate to restructure it, creating multiple "steps to do X", "steps to do Y" etc. subsections? Something like this:

To obtain an identity assertion before running steps s, given promise p:

  1. Let idpPromise be the promise returned by the IdP proxy.
  2. Upon fulfillment of idpPromise, run s, given p.
  3. Upon rejection of idpPromise, reject p.

To prepare to create an offer given promise p:

  1. In parallel:
    1. Do parallel stuff.
    2. Queue a task to perform the final steps to create an offer given p.

The final steps to create an offer given promise p are as follows:

  1. If IdP has changed, obtain an identity assertion before preparing to create an offer given p and abort these steps.
  2. If PC state has changed, prepare to create an offer given p again and abort these steps.
  3. Make offer and resolve p with it.

I've noticed the "web APIs" section of the HTML spec (for example) does this kind of thing all the time. It very rarely has more than 2 levels of nested tasks.

@adam-be
Copy link
Member

adam-be commented Oct 19, 2016

I'm not sure how the PR text works with the two returns of p (both step 4.).

I think we should give the approach with subsections a try. I'm not sure where the execution in the above description starts and what s is.

@taylor-b
Copy link
Contributor Author

That was something I noticed, but I thought maybe the extra return wouldn't hurt anything. In any case, I'll work on the alternate flavor of this PR.

Also, I got to thinking, maybe the identity assertion race condition (calling setIdentityProvider after createOffer) should be handled in some other way? Using two identity providers would be terrible user experience anyway (I assume) so this is something that should never happen. I'll try moving "begin identity assertion request process" to the synchronous section for this PR.

This simplifies the algorithm a bit, and ignores the case of:

`pc.createOffer()`
`setIdentityProvider(idp)`

Only the identity provider set at the time `createOffer` was called will
be used. And the identity assertion request process will begin as soon
as `createOffer` is called, rather than when the operation is taken off
the queue.
@taylor-b
Copy link
Contributor Author

See alternate PR above. I think the identity provider part still needs to be formalized better, but that can probably happen separately.

@adam-be
Copy link
Member

adam-be commented Oct 20, 2016

I think we can close this PR in favor of #875.

@stefhak
Copy link
Contributor

stefhak commented Oct 20, 2016

Closing as it is superseded by #875 .

@stefhak stefhak closed this Oct 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants