W3C home > Mailing lists > Public > public-webrtc-logs@w3.org > June 2017

Re: [webrtc-pc] Race condition in enqueue an operation

From: jan-ivar via GitHub <sysbot+gh@w3.org>
Date: Fri, 23 Jun 2017 16:38:46 +0000
To: public-webrtc-logs@w3.org
Message-ID: <issue_comment.created-310714399-1498235925-sysbot+gh@w3.org>
@soareschen Thanks for finding the cases where this matters: not validation errors, but the state errors you mention (The spec has improved in the years since last time this came up, moving validation outside of the enqueuing. We can't do the same for state checks unfortunately).

> the only observable side effect for the special case of length 1 is

As designed, length 1 is the *typical* case, not a special case. This is central to my point.

> 1. When close() is called in next synchronous step, promise may or may not resolve depending on operation queue length.

Again, it neither resolves nor rejects after `close()`. Look at [enqueue](http://w3c.github.io/webrtc-pc/#enqueue-an-operation):

 * *"7. Upon fulfillment or rejection of the promise returned by the operation, run the following:*
   * *1. If connection's [[isClosed]] slot is true, abort these steps."*

> 2. When immediately printing returned promise to console, the promise is shown as "rejected" or "pending" depending on operation queue length.
> 
> I find the second use case of console debugging to be weird.

The queue is inherently weird, so this is correct behavior, a clue to the weirdness that just happened. Avoid the queue at peril, and there's nothing weird here. I'm averse to polluting the typical case with the weirdness of the atypical legacy case merely for consistency.

Your interactions with Firefox don't match mine. Breakpoint after `createAnswer()` in [this fiddle](https://fiddle.jshell.net/jib1/fsoL1rab/show/):
```js
▼ p: Promise { "rejected" }
    <state>: "rejected"
  ▶ <reason>: InvalidStateError
  ▶ __proto__: PromiseProto
```

> I think we can solve this with a test driven approach.

If you're suggesting the spec should change based on test results, then I find that backwards. Tests exist to file bugs against browsers, not the other way around. Implementations have too many bugs atm. In any case, I don't think a legacy accommodation should wag the dog.

In any case, this test is wrong because it should never resolve nor reject according to the spec:
```js
// This should reject
// Note that this test fail in current browsers
const promise = pc.createAnswer();
pc.close();
promise.catch(() => console.log('pass'));
```

These two tests are wrong because https://github.com/w3c/webrtc-pc/pull/875 says they should be included:
```js
// This should not have added track in offer
// because createOffer only have async part of operation
const promise = pc.createOffer();
pc.addTransceiver('audio');
promise.then(offer => console.log('offer should not have track:', offer);

// This should also not have added track in offer
pc.setLocalDescription(oldOffer); // increase queue length
const promise = pc.createOffer();
pc.addTransceiver('audio');
promise.then(offer => console.log('offer should not have track:', offer);
```

-- 
GitHub Notification of comment by jan-ivar
Please view or discuss this issue at https://github.com/w3c/webrtc-pc/issues/1218#issuecomment-310714399 using your GitHub account
Received on Friday, 23 June 2017 16:38:53 UTC

This archive was generated by hypermail 2.3.1 : Tuesday, 4 June 2019 15:32:44 UTC