Add initialization of DataChannelId on connection #2222

alvestrand · 2019-07-08T07:23:03Z

Note that this initializes DataChannelId on connecting
the SctpTransport, not when the DTLS role is determined.

aboba

lines 9151-52: The DTLS role is not determined as part of the SCTP association setup process. So I would suggest deleting the reference to the DTLS role in line 9150 (the DTLS role will have been determined, so it isn't relevant).

henbos · 2019-07-11T14:35:59Z

webrtc.html

+                    to a value generated by the
+                    user agent, according to [[!RTCWEB-DATA-PROTOCOL]].
+                    If no available ID could be generated,
+                    <a>throw</a> an <code>OperationError</code> exception.


You can't throw an exception here, we're in a queued task, we're not in a function

We should fail like we do below?

Agree. Actually, these two steps can be merged.

Edit: This was a reply to @henbos' comment.

henbos · 2019-07-11T14:48:46Z

webrtc.html

+                  <p>If <var>channel</var>'s <a>[[\DataChannelId]]</a> slot is
+                    <code>null</code>, initialize <a>[[\DataChannelId]]</a>
+                    to a value generated by the
+                    user agent, according to [[!RTCWEB-DATA-PROTOCOL]].


It's important that the underlying data transport's ID matches this label. I think we either need to say that this generation took place as part of the underlying connection procedure and is surfaced here, or we have to generate the ID here and then "in parallel" update the underlying data transport to match this ID.

I can't quite follow that. Can you elaborate? I think it's okay to not say anything here as the referenced IETF document explains the procedure well.

Edit: This was a reply to @henbos' comment.

I took "generated by the user agent" to be so obvioiusly the underlying data transport's ID that it didn't need to be called out.

Clarification:

Each RTCDataChannel has an associated underlying data transport.
Whenever something happens to the underlying transport, like connecting or closing which could happen in parallel on a network thread, we queue a task to update the RTCDataChannel to match. We need to be careful about keeping these two objects (the JS version and the actual SCTP implementation) in sync.

For example, when creating an RTCDataChannel with createDataChannel(), there's a step to "Create channel's associated underlying data transport and configure it according to the relevant properties of channel." meaning we pass down the ID to the underlying transport. Likewise when a data channel is created by the underlying sctp transport, we queue a task to to create an RTCDataChannel to match the configuration received from the underlying data transport.

So if we modify the [DataChannelId] internal slot, we need to inform the underlying data transport that this is the ID that it should use.

Do we need to add "In parallel, update the underlying data transport's id"?

Or better: Can we say "If [DataChannelId] is null, update it to match the ID received from the underlying sctp data channel that was generated as specified in [RTCWEB-DATA-PROTOCOL]"? This implies that we already did generate it in parallel, as we should have?

I took "generated by the user agent" to be so obvioiusly the underlying data transport's ID that it didn't need to be called out.

The concern isn't so much that they mismatch since that clearly is the intent but that it is unclear if the ID is generated before or after the queuing of a task.

The id is not a modifiable property. It's a settable property. Once it's set to a non-null value, it can't be changed. So we don't have to deal with synchronization of changes - either it's set at creation (in JS, communicated to lower layers) or it's set in the protocol machine (in lower layers, communicated to JS). This makes thinking about synchronization somewhat easier.

I changed this to "initialize [[DataChannelId]] to the value generated by the underlying sctp data channel, according to..."

lgrahl

This is phrased as if it only prevents the first of the three cases listed in #1818. See inline comments for suggestions.

lgrahl · 2019-07-11T17:33:31Z

webrtc.html

-                    populated once the DTLS role is determined during the
-                    process of <a data-lt="set the RTCSessionDescription">
-                    setting an <code>RTCSessionDescription</code></a>.
+                    populated during the process of


This references conected but indicates connecting. I'd instead suggest: "[...] it will be populated once the SCTP association has been established."

The section is called "connected procedure". I guess I should call it that.

lgrahl · 2019-07-11T17:38:17Z

webrtc.html

@@ -9287,7 +9287,15 @@ <h4 id="sctp-transport-connected">Connected procedure</h4>
                  <p>Let <var>channel</var> be the <code><a>RTCDataChannel</a></code> object.</p>
                </li>
                <li>
-                  <p>If the <var>channel</var>'s <a>[[\DataChannelId]]</a> slot is greater or
+                  <p>If <var>channel</var>'s <a>[[\DataChannelId]]</a> slot is


When the statechange event is being caught by the application, is it ensured that the ids have been populated? Probably yes since the event is being appended to the event loop?

Not sure. I moved the statechange event to after the ID assignment, but that causes "announce the channel as open" to move before the statechange event. Does that matter?
I don't think so, because the "Announcing a data channel as open" section starts off with "MUST queue a task".

lgrahl · 2019-07-11T17:52:32Z

webrtc.html

+                  <p>If <var>channel</var>'s <a>[[\DataChannelId]]</a> slot is
+                    <code>null</code>, initialize <a>[[\DataChannelId]]</a>
+                    to a value generated by the
+                    user agent, according to [[!RTCWEB-DATA-PROTOCOL]].


I can't quite follow that. Can you elaborate? I think it's okay to not say anything here as the referenced IETF document explains the procedure well.

Edit: This was a reply to @henbos' comment.

lgrahl · 2019-07-11T17:53:05Z

webrtc.html

+                    to a value generated by the
+                    user agent, according to [[!RTCWEB-DATA-PROTOCOL]].
+                    If no available ID could be generated,
+                    <a>throw</a> an <code>OperationError</code> exception.


Agree. Actually, these two steps can be merged.

Edit: This was a reply to @henbos' comment.

henbos · 2019-08-01T13:31:16Z

webrtc.html

+                  <p>If <var>channel</var>'s <a>[[\DataChannelId]]</a> slot is
+                    <code>null</code>, initialize <a>[[\DataChannelId]]</a>
+                    to a value generated by the
+                    user agent, according to [[!RTCWEB-DATA-PROTOCOL]].


Clarification:

Each RTCDataChannel has an associated underlying data transport.
Whenever something happens to the underlying transport, like connecting or closing which could happen in parallel on a network thread, we queue a task to update the RTCDataChannel to match. We need to be careful about keeping these two objects (the JS version and the actual SCTP implementation) in sync.

For example, when creating an RTCDataChannel with createDataChannel(), there's a step to "Create channel's associated underlying data transport and configure it according to the relevant properties of channel." meaning we pass down the ID to the underlying transport. Likewise when a data channel is created by the underlying sctp transport, we queue a task to to create an RTCDataChannel to match the configuration received from the underlying data transport.

So if we modify the [DataChannelId] internal slot, we need to inform the underlying data transport that this is the ID that it should use.

Do we need to add "In parallel, update the underlying data transport's id"?

Or better: Can we say "If [DataChannelId] is null, update it to match the ID received from the underlying sctp data channel that was generated as specified in [RTCWEB-DATA-PROTOCOL]"? This implies that we already did generate it in parallel, as we should have?

henbos · 2019-08-01T13:40:59Z

webrtc.html

+                    <a>throw</a> an <code>OperationError</code> exception.
+                  </p>
+                <li>
+                  <p>If <var>channel</var>'s <a>[[\DataChannelId]]</a> slot is greater or
                  equal to <var>transport</var>'s <a>[[\MaxChannels]]</a> slot,
                  <a data-lt="data-channel-failure"> close the <var>channel</var> due to a failure
                  </a>. Otherwise, <a data-lt="announce the rtcdatachannel as open">announce the


Not new in this PR: We are inside of steps invoking "announce the rtcdatachannel as open" and "close the channel due to failure". The referenced algorithm queues a task. We should refactor this so that we can invoke the algorithms without queuing a task; the queuing of a task is in response to something happening in parallel.

Not addressed in this PR.

alvestrand · 2019-08-15T14:04:32Z

I think it has a test. Will look to check that it actually tests.

Fixes #1818 Note that this initializes DataChannelId on connecting the SctpTransport, not when the DTLS role is determined.

alvestrand · 2019-09-27T11:45:49Z

Please re-review. I changed text.

lgrahl · 2019-09-30T11:37:37Z

webrtc.html

                  <a data-lt="data-channel-failure"> close the <var>channel</var> due to a failure
                  </a>. Otherwise, <a data-lt="announce the rtcdatachannel as open">announce the
                  <var>channel</var> as open</a>.</p>
                </li>
              </ol>
            </li>
+            <li>
+              <p><a>Fire an event</a> named <code><a data-lt="
+              RTCSctpTransport state change">statechange</a></code> at <var>


The event fire order can now be misinterpreted easily. The individual open events fire after the statechange event because the open event is scheduled in yet another task. When we refactor that as suggested by @henbos, we need to ensure not to break that order: statechange, then for each data channel, open.

Adding a note seems reasonable.

alvestrand · 2019-10-03T10:25:14Z

Updated with note (and a drive-by grammar edit).

lgrahl · 2019-10-03T11:31:57Z

LGTM now

henbos

LGTM

aboba reviewed Jul 9, 2019

View reviewed changes

aboba requested review from henbos and jan-ivar July 11, 2019 14:35

henbos reviewed Jul 11, 2019

View reviewed changes

lgrahl reviewed Jul 11, 2019

View reviewed changes

aboba added the Needs submitter action label Jul 18, 2019

henbos added Needs Test Needs a WPT test and removed Needs submitter action labels Jul 18, 2019

henbos reviewed Aug 1, 2019

View reviewed changes

alvestrand added the Needs submitter action label Aug 1, 2019

alvestrand added Editors can integrate and removed Needs Test Needs a WPT test labels Aug 15, 2019

alvestrand and others added 3 commits September 27, 2019 13:17

Add initialization of DataChannelId on connection

7a9651d

Fixes #1818 Note that this initializes DataChannelId on connecting the SctpTransport, not when the DTLS role is determined.

Remove reference to DTLS role.

c792701

Review comments

70dc3e5

alvestrand force-pushed the datachannel-id-assign-bug-1818 branch from 98afd39 to 70dc3e5 Compare September 27, 2019 11:45

lgrahl approved these changes Sep 30, 2019

View reviewed changes

Add explanatory sequencing note

e8d2a4f

henbos approved these changes Oct 3, 2019

View reviewed changes

henbos merged commit 8917c57 into master Oct 3, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add initialization of DataChannelId on connection #2222

Add initialization of DataChannelId on connection #2222

alvestrand commented Jul 8, 2019 •

edited by pr-preview bot

Loading

aboba left a comment •

edited

Loading

henbos Jul 11, 2019

henbos Jul 11, 2019

lgrahl Jul 11, 2019 •

edited

Loading

alvestrand Sep 27, 2019

henbos Jul 11, 2019

lgrahl Jul 11, 2019 •

edited

Loading

alvestrand Aug 1, 2019

henbos Aug 1, 2019 •

edited

Loading

henbos Aug 1, 2019

alvestrand Aug 15, 2019

alvestrand Sep 27, 2019

lgrahl left a comment

lgrahl Jul 11, 2019

alvestrand Sep 27, 2019

lgrahl Jul 11, 2019

alvestrand Sep 27, 2019

lgrahl Jul 11, 2019 •

edited

Loading

lgrahl Jul 11, 2019 •

edited

Loading

henbos Aug 1, 2019 •

edited

Loading

henbos Aug 1, 2019

alvestrand Sep 27, 2019

alvestrand commented Aug 15, 2019

alvestrand commented Sep 27, 2019

lgrahl Sep 30, 2019

alvestrand Oct 3, 2019

alvestrand commented Oct 3, 2019

lgrahl commented Oct 3, 2019

henbos left a comment

Add initialization of DataChannelId on connection #2222

Add initialization of DataChannelId on connection #2222

Conversation

alvestrand commented Jul 8, 2019 • edited by pr-preview bot Loading

aboba left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgrahl Jul 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgrahl Jul 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henbos Aug 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgrahl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgrahl Jul 11, 2019 • edited Loading

Choose a reason for hiding this comment

lgrahl Jul 11, 2019 • edited Loading

Choose a reason for hiding this comment

henbos Aug 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alvestrand commented Aug 15, 2019

alvestrand commented Sep 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alvestrand commented Oct 3, 2019

lgrahl commented Oct 3, 2019

henbos left a comment

Choose a reason for hiding this comment

alvestrand commented Jul 8, 2019 •

edited by pr-preview bot

Loading

aboba left a comment •

edited

Loading

lgrahl Jul 11, 2019 •

edited

Loading

lgrahl Jul 11, 2019 •

edited

Loading

henbos Aug 1, 2019 •

edited

Loading

lgrahl Jul 11, 2019 •

edited

Loading

lgrahl Jul 11, 2019 •

edited

Loading

henbos Aug 1, 2019 •

edited

Loading