Fix broken RTCPeerConnection's operations array to work with promises.

[jan-ivar]

I'm resubmitting this PR for consideration as a solution to #299, and I respectfully request that people take another look, as I don't see any competing PRs to solve this issue.

[alvestrand]

@ekr @adam-be @pthatcherg can you comment?

[ekr]

This doesn't seem to me to be an improvement.

[jan-ivar]

@ekr are you saying this does not fix #299?

[ekr]

#299 does not contain a complete description.

[jan-ivar]

I've updated #299 with a better summary.

[pthatcherg]

Mentioning addIceCandidate seems like a good thing.

[jan-ivar]

Btw, here it is in code:

_runOperation: function(operation) {
  this._throwIfClosed();
  let p = (!this._operationsLength)? operation() : this._operations.then(operation);
  this._operationsLength++;
  let decrement = () => this._operationsLength--;
  this._operations = p.then(decrement, decrement);
  return p;
},

[dontcallmedom]

I like the rewrite; it looks more accurate and precise than the current text.

[pthatcherg]

OK, I've looked into this more deeply and I think it's trying to do 3 different things, 2 of which are good, 1 of which is not good.

1. Update to deal with promises and closed signaling state (good).
   2 Some general cleanup, like referring to addIceCandidate (good).
2. Defining a queue of operations as a promise chain + queue length (not good).

To keep the first 2, but not the 3rd, I created https://github.com/w3c/webrtc-pc/pull/314/files.

I still think #3 is ugly, and I still don't see what it buys us. As far as I can tell, it's only there to try and address this use case:

var senderX = pc.addTrack(trackX);
pc.createOffer();
var senderY = pc.addTrack(trackY);

But I don't see how this PR fixes that use case. The fundamental problem with that use case is that addTrack isn't in the queue with the rest of the operations, not that the queue is defined in terms of a queue or a promise chain.

[pthatcherg]

Yikes, somehow my text got garbled. The three were supposed to be:

• Update to deal with promises and closed signaling state (good).
• Some general cleanup, like referring to addIceCandidate (good).
• Defining a queue of operations as a promise chain + queue length (not good).

[pthatcherg]

By the way, I think in this case:

var senderX = pc.addTrack(trackX);
pc.createOffer();
var senderY = pc.addTrack(trackY);

The answer should be that the offer has the first track but not the second. Previously, you said that the "promise chain" approach would make it be the opposite, where the offer would include both. I think that would be broken.

[jan-ivar]

I still think #3 is ugly, and I still don't see what it buys us.

It solves #299.

As far as I can tell, it's only there to try and address this use case:

var senderX = pc.addTrack(trackX);
pc.createOffer();
var senderY = pc.addTrack(trackY);

Not so. Compare to #222 to see what was added to solve that use-case.

But I don't see how this PR fixes that use case.

It solves it by executing operation synchronously when chainLength is 0.

The fundamental problem with that use case is that addTrack isn't in the queue with the rest of the operations, not that the queue is defined in terms of a queue or a promise chain.

That sounds like another solution rather than a fundamental statement (addTrack is not the lone sync api). The working group spent all sprint to get to agreement that this was the desired behavior, so please lets not reopen that can of worms.

[jan-ivar]

s/sprint/spring/

[pthatcherg]

On Wed, Sep 23, 2015 at 6:36 AM, jan-ivar [email protected] wrote:

I still think #3 #3 is ugly, and I
still don't see what it buys us.

It solves #299 #299.

#299 isn't very clear, but it appears to say "current text doesn't address
promises" and "current text doesn't define 'execute asynchronously'".

I wrote the is PR: #314 to address
both of these while keeping it closer to the current text of "queue of
operations". In other words I don't think "queue of operations" is needed
to address #299.

As far as I can tell, it's only there to try and address this use case:

var senderX = pc.addTrack(trackX);
pc.createOffer();
var senderY = pc.addTrack(trackY);

Not so. Compare to #222 #222 to
see what was added to solve that use-case.

Can you please make it more clear? I'm having a heard time reading the
difference between those two PRs and figure out how it address this case.

But I don't see how this PR fixes that use case.

It solves it by executing operation synchronously when chainLength is
0.

​What do you mean it executes it synchronously? ​The PeerConnection isn't
going to wait for the promise to fulfilled, is it?

I still don't see how the behavior of PR 307 is different than PR 314.
I'll ask again: can you please provide a case where it's different?
Because if it's not then I'd prefer PR 314 because it's much more simple.

The fundamental problem with that use case is that addTrack isn't in the
queue with the rest of the operations, not that the queue is defined in
terms of a queue or a promise chain.

That sounds like another solution rather than a fundamental statement (
addTrack is not the lone sync api). The working group spent all sprint to
get to agreement that this was the desired behavior, so please lets not
reopen that can of worms.

​Sorry, I may have missed the discussion. Can you summarize what the
agreement was?​

—
Reply to this email directly or view it on GitHub
#307 (comment).

[jan-ivar]

#299 isn't very clear, but it appears to say "current text doesn't address promises" and "current text doesn't define 'execute asynchronously'".

It actually says: "the old text doesn't return any promises is the problem, or allow the synchronous parts of the operations to run, and they need to run synchronously, since they're the ones creating the promises."

Please let me know what is not clear about that.

[pthatcherg]

Well, that's the 3rd comment down after a reference to a giant comment on a
PR, followed by a vague TL;DR. I didn't get past the giant comment and
the vague TL;DR. Your clear statement was lost in the noise, I'm afraid.
But I think I may understand now.

On Wed, Sep 23, 2015 at 10:20 AM, jan-ivar [email protected] wrote:

#299 #299 isn't very clear, but
it appears to say "current text doesn't address promises" and "current text
doesn't define 'execute asynchronously'".

It actually says: "the old text doesn't return any promises is the
problem, or allow the synchronous parts of the operations to run, and they
need to run synchronously, since they're the ones creating the promises."

Please let me know what is not clear about that.

—
Reply to this email directly or view it on GitHub
#307 (comment).

[jan-ivar]

​What do you mean it executes it synchronously?

Each of the five _operation_s: createOffer, setLocalDescription, createAnswer, setRemoteDescription, and addIceCandidate, are functions that come with processing models.

Look at setLocalDescription:

1. Let p be a new promise.
2. If this RTCPeerConnection object's signaling state is closed, the user agent MUST reject p with InvalidStateError, and jump to the step labeled Return.
3. If a local description contains a different set of ICE credentials, then the ICE Agent MUST trigger an ICE restart. When ICE restarts, the gathering state will be changed back to "gathering", if it was not already gathering. If the RTCPeerConnection ice connection state was "completed", it will be changed back to "connected".
4. The user agent MUST start the process to apply the RTCSessionDescription argument.
5. Return: Return p.

(I'm leaving out the remaining steps which are all about what step 4 launched in parallel).

Before it returns, this function:

1. Creates a new promise p.
2. Rejects and returns p if the pc is closed.
3. Starts stuff in parallel that will resolve or reject p later.
4. Returns p.

p is important, as without p you have no handle on when the asynchronous part set up by setLocalDescription completes.

Without calling these steps you have no p. Furthermore, in the typical call (where nothing is queued) the steps must be called synchronously, by which I mean that the steps must have executed before the call from content JS returns, and p must be returned to content JS.

That's what I mean by synchronous.

[jan-ivar]

But I think I may understand now.

Great. I'm striving to get better at boiling stuff down. I see you made additional comments elsewhere, so let me catch up.

[jan-ivar]

Addressed feedback from @adam-be in #314 (comment)

[jan-ivar]

Simplified for fair comparison with #314.

[stefhak]

Closing as the discussion seems to have moved to #314 and in addition seems to have converged over there.