In-band negotiated channel should be open when dispatching

[lgrahl]

These changes move the data channel that is about to be announced into the open state prior to firing the datachannel event. (Also explicitly announces the data channel as open now.)

Resolves #1761

I can write a WPT test for this once merged.

---

Preview | Diff

[adam-be]

We should cut up the long line before merging.

As you mention, we need a test for this. Let's see if the chairs are satisfied with the offered Promise<test>. :)

[lgrahl]

Let me rephrase: I can write a WPT test for this once approved (didn't want to mess with web-platform-tests/wpt#10468 before approval).

[taylor-b]

Looks good to me. By the way, here's a fiddle that tests the behavior of the browser: https://jsfiddle.net/n6mgoxn2/

Found that Chrome:

• Initially sets state to open
• Allows you to send in the event handler
• Fires onopen after ondatachannel

And Firefox:

• Initially sets state to connecting
• Doesn't allow you to send in the event handler
• Fires onopen after ondatachannel

So although I'm not especially fond of the hybrid approach, it does seem like the safest bet to ensure existing code continues to work.

[jan-ivar]

Normally, I'd worry about running browser code after calling content JS - we normally fire events at the end - but apparently announce the data channel as open queues a separate task, so that seems fine.

But why this test? Is it to avoid firing the open event if JS does

pc.ondatachannel = ({channel}) => channel.close();
pc.onopen = () => { /* never fires */ };

?

If so, it won't catch:

pc.ondatachannel = async ({channel}) => { await 0; channel.close(); };
pc.onopen = ({channel}) => console.log(channel.readyState); // open, and remains so

If I read the spec right, this would appear to leave channel.readyState as "open", even though the channel is closed. That seems wrong. We didn't specify it before, but if we're going to, then I think we want it to be right.

Should we maybe check existing state in the queued task? We check pc.[[IsClosed]] but not [[ReadyState]].

Or would it be better to not set state at all, since we've opened it already?

[jan-ivar]

and wordwrap.

[lgrahl]

But why this test?

The channel could have been closed by the datachannel event handler in the meantime and it would be pointless to fire that event if the actual [[ReadyState]] has already changed. It could break state machines of the application and of course would be confusing when logged (I logged my .close() call but now it raised the open event - wtf?).

Yes, it would not catch the latter example but I don't see a problem with that since open will fire in between await 0 (really, this is allowed in JS? This language... 🔥) and channel.close(). However, that is an entirely different situation. Every application dev needs to be aware that race conditions can occur between yields if not placed carefully.

If I read the spec right, this would appear to leave channel.readyState as "open", even though the channel is closed.

I don't read that out of the spec. Can you elaborate?

[jan-ivar]

@lgrahl Sorry for dropping the ball here. Thanks for confirming intent.

I don't see a problem with that since open will fire in between await 0

Actually, it won't. await 0 (or Promise.resolve().then(() => channel.close()) if you prefer, this is not specific to await syntax), will execute on a micro-task, immediately after the current task, before the next queued task.

I think we need to modify announce the data channel as open to check that readyState is not "closed" or "closing", before setting it to "open". This seems like a good idea anyway in a queued task.

[lgrahl]

Alright, so this is what my naive mind has assumed how it works (referring to the second code example in #1851 (comment)):

1. The datachannel event fires which runs some synchronous code immediately - the event handler. This event handler enqueues a new task to run the code after await 0.
2. Since the synchronous code could have closed the channel in the meantime, the check is required.
3. The scheduled channel.close() runs after all other tasks enqueued before this one ran.

Please correct me if it works differently. And sorry for the brief detour. :)

Edit: Mhh, I guess your point is that interpreting the spec strictly, announcing the channel as open enqueues a new task and that runs after channel.close(). Good catch!

---

However, your suggestion to move that code into the open announcement sounds good to me. Only the description announce the data channel as open is a bit awkward if it under certain circumstances does not do so. Suggestions welcome. Edit 2: But then again... it's fine. I'll do the change tomorrow.

[jan-ivar]

announcing the channel as open enqueues a new task and that runs after

Exactly.

announce the data channel as open is a bit awkward if it under certain circumstances does not do so

That's already the case if [[IsClosed]] is true, so I wouldn't worry about it.

[lgrahl]

Added tests for this in lgrahl/web-platform-tests@1de5a54.

@taylor-b Supplement from these tests: Chrome still fires the open event when closing the channel inside the datachannel event handler. Firefox does not.

[jan-ivar]

@lgrahl are there more changes coming to address my comments wrt my feedback on checking readyState in the queued task?

[lgrahl]

@jan-ivar Yes, I plan to address your comments as discussed. Just got distracted by other stuff. :)

[jan-ivar]

lgtm