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

@jan-ivar Thank you for your explanation. Now I understand much better on why the enqueue operation steps are described the way they are.

The way I see it is that the main difference the patch in  #1397 make is that the synchronous part of the operation also execute asynchronously if the operation queue length is 1. 

Currently in the spec, there are only two places where operations have synchronous part:

- `createAnswer` step 4.1
- `addIceCandidate` step 4

Both of the synchronous parts only concern with returning rejected promise synchronously in case that operation queue length is 1. As a result, the only observable side effect for the special case of length 1 is:

1. When `close()` is called in next synchronous step, promise may or may not resolve depending on operation queue length.
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. In Chrome inspector one can click an arrow cursor to expand objects including promises, and the expanded attributes show the latest state of the promise, which in this case should be "rejected" even if the initial state is "pending".  This seems more like a compensation for other browsers like Firefox that do not have the interactive feature to quickly inspect latest attributes of objects. 

But Chrome and Firefox also show rejected promises returned from failed cases like `createAnswer()` as pending promise, so I am more confused of which browser that debugging technique is being used. Note that the use case is also limited to implementation-specific debugging because it is impossible to query the current state of promises in JavaScript, which also make this untestable.

Let me try to understand your points with some pseudo-tests here. Based on current behavior, enqueue would execute operation synchronously if operation queue length is 1. So:

```javascript
// This should fulfill with answer
pc.setRemoteDescription(offer);
pc.createAnswer().then(() => console.log('pass'))

// This should reject
// Note that this test fail in current browsers
const promise = pc.createAnswer();
pc.close();
promise.catch(() => console.log('pass'));

// This should never resolve
pc.setRemoteDescription(offer); // increase queue length
const promise = pc.createAnswer();
pc.close();
promise.catch(() => console.log('fail'));

// This should also never resolve
pc.addIceCandidate(...); // rejects
const promise = pc.createAnswer();
pc.close();
promise.catch(() => console.log('fail'));

// This should show rejected promise in console
console.log('rejected promise:', pc.createAnswer());

// This should show pending promise in console
// that later have an updated rejected state
pc.createAnswer(); // rejects
console.log('pending promise:', pc.createAnswer());

// 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);
```

I think we can solve this with a test driven approach. I am away next week. After I'm back I plan to write test cases that match the intended behavior in the spec.

-- 
GitHub Notification of comment by soareschen
Please view or discuss this issue at https://github.com/w3c/webrtc-pc/issues/1218#issuecomment-310615500 using your GitHub account

Received on Friday, 23 June 2017 09:15:08 UTC