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

Negotiation methods are vestigial, racy with a pushy SFU. #2221

Closed
jan-ivar opened this issue Jul 1, 2019 · 1 comment
Closed

Negotiation methods are vestigial, racy with a pushy SFU. #2221

jan-ivar opened this issue Jul 1, 2019 · 1 comment
Assignees

Comments

@jan-ivar
Copy link
Member

jan-ivar commented Jul 1, 2019

Not covered in "Perfect negotiation in WebRTC", is dealing with an SFU. Fippo explained to me that SFUs can be “pushy”: They’ll send an offer, followed immediately by a second “better” offer. One strategy is the “FIFO peer”:

io.onmessage = async ({data: {description, candidate}}) => {
  if (description) {                              // FIFO peer:
    await Promise.all([                           // ←- Avoids race!
      pc.setRemoteDescription(description),       // ←- Always an “offer”. Fixed roles
      pc.createAnswer(),                          // ←- 
      pc.setLocalDescription({type: "answer"})    // ←- Unusual, but works today
    ]);                                           // ←- 
    io.send({description: pc.localDescription});

A second offer may come in while we’re busy responding to the first offer. As a workaround we use Promise.all to front-load the peer connection’s queue with all our methods to get back to "stable" before any other methods get a go!

Even with #2165 fixed, we’re not able to fully get rid of Promise.all here. We’ll still need:

io.onmessage = async ({data: {description, candidate}}) => {
  if (description) {
    await Promise.all([                          // ←- Still needed
      pc.setRemoteDescription(description),
      pc.setLocalDescription()
    ]);
    io.send({description: pc.localDescription});

The SFU FIFO case shows SLD() & SRD() are racy and a bit outdated: from an earlier time when SDP mangling between createOffer/Answer to SLD was allowed (it is now forbidden), and when rejecting m-lines in the answer was common. I propose we give people a simpler and safer alternative that’s race-free and glare-proof:

  io.onmessage = async ({data: {description, candidate}}) => {
    if (description) {                                      // FIFO peer:
      await pc.setRemoteAndLocalDescriptions(description);  // ←- Always an “offer”.
      io.send({description: pc.localDescription});          // because of fixed roles
    } else if (candidate) {
      await pc.addIceCandidate(candidate);
    
  };

Proposal: pc.setRemoteAndLocalDescriptions(description) works like regular SRD, plus, if description is an offer, generates and sets a local answer before resolving. This would be behavior-neutral with the Promise.all case. I.e. if SRD succeeds, but SLD fails, we won’t roll back on failure.

@jan-ivar
Copy link
Member Author

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

No branches or pull requests

1 participant