Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RTCDataChannel.send during 'closing' state #1827

Closed
lgrahl opened this issue Apr 5, 2018 · 15 comments · Fixed by #2223
Closed

RTCDataChannel.send during 'closing' state #1827

lgrahl opened this issue Apr 5, 2018 · 15 comments · Fixed by #2223
Assignees
Labels
Needs Test Needs a WPT test PR exists

Comments

@lgrahl
Copy link
Contributor

lgrahl commented Apr 5, 2018

Let's look at the following scenario: Peer A and B have an open data channel. If peer A closes the channel, peer B's channel will go into the closing state until all pending messages of both peers have been sent. If peer B calls the .send method, it will throw an exception since the channel is not in the open state any more. But peer B has no way of knowing that unless it polls .readyState.

Do we need to address that with an... (I'm reluctant to say that) event?

@aboba
Copy link
Contributor

aboba commented Apr 19, 2018

Can you submit a PR?

@aboba aboba added the Needs PR Discussion has converged - Pull Request needed label Apr 19, 2018
@lgrahl
Copy link
Contributor Author

lgrahl commented Apr 22, 2018

As an alternative: What if we simply say to discard the message passed to the .send call in case .readyState is closing? Edit: Mh, no, on second thought I don't like that at all.

@lgrahl
Copy link
Contributor Author

lgrahl commented Apr 22, 2018

Adding an onclosing event would be overkill in my opinion. I would like to hear more opinions (and hopefully suggestions) before creating a PR.

@aboba
Copy link
Contributor

aboba commented Apr 27, 2018

@lgrahl Looking at RFC 6525 and Section 6.7 of draft-ietf-rtcweb-data-channel, one way to handle this would be to have receipt of an incoming stream reset cause .readyState to transition to closed. The receipt of an incoming stream reset causes an outgoing stream reset to be sent. While after sending an outgoing stream reset the peer could transition .readyState to closing and await receipt of a re-configuration response with a result of success prior to transitioning .readyState to closed, after sending the outgoing stream reset, data can neither be sent nor received so closed might be more appropriate.

"6.7. Closing a Data Channel

Closing of a data channel MUST be signaled by resetting the
corresponding outgoing streams [RFC6525]. This means that if one
side decides to close the data channel, it resets the corresponding
outgoing stream. When the peer sees that an incoming stream was
reset, it also resets its corresponding outgoing stream. Once this
is completed, the data channel is closed. Resetting a stream sets
the Stream Sequence Numbers (SSNs) of the stream back to 'zero' with
a corresponding notification to the application layer that the reset
has been performed. Streams are available for reuse after a reset
has been performed.

[RFC6525] also guarantees that all the messages are delivered (or
abandoned) before the stream is reset."

@lgrahl
Copy link
Contributor Author

lgrahl commented Apr 28, 2018

That would probably resolve the issue with .send but open another one: The close event is an indication that a stream ID can be reused. However, if we set the closed state earlier, the application doesn't know when an ID can be reused.

@jhaenchen
Copy link

jhaenchen commented Sep 10, 2018

@aboba or @lgrahl Excuse me if I'm misinterpreting this, but how would this procedure work for a hard-quit peer, i.e. a browser peer that has been forced to quit and so does not send any reset or anything? I am still stuck in a closing state in this scenario and thought it might be relevant here.

To elaborate:

I have two connected peers, they share a RTCPeerConnection and a DataChannel. The other peer then force-quits their browser. I receive oniceconnectionstatechange with peerConnection.iceConnectionState = 'failed'. Because of this event, I want to clean up the RTCDataChannel, so I call channel.close() and so dataChannel.readyState = 'closing'. Here's where the issue starts: presumably because the RTCPeerConnection that supports the data channel is no longer connected, the reset signal and any queuing operations will fail and onclose is not fired. As I type this I wonder if it's more of a chromium bug or if the specification does not provide for this scenario.

@lgrahl
Copy link
Contributor Author

lgrahl commented Sep 10, 2018

@jhaenchen There's no need for cleanup in that case. I was going to write that:

When the ICE connection state goes into failed, all data channels should be in the closed state immediately. If they aren't, it's a bug in the implementation.

But I haven't found a procedure in the spec that defines what should happen once the peer connection state goes into failed. @aboba, @jan-ivar, anyone? 😅 But to prevent becoming sidetracked in this issue, perhaps one of you could open a new issue when responding.

Anyhow, even if the procedure is missing, there is no need to do cleanup and the data channels should be in the closed state. Whether or not the close event should fire is up for debate. However, I would argue that the event should fire because it is triggered by the remote side.

@aboba aboba assigned alvestrand and unassigned lgrahl Oct 6, 2018
@aboba aboba added the TPAC 2018 label Oct 6, 2018
@aboba aboba assigned lgrahl, jan-ivar and alvestrand and unassigned alvestrand, lgrahl and jan-ivar Oct 6, 2018
@aboba
Copy link
Contributor

aboba commented Oct 21, 2018

@lgrahl Could we fire the onclosed event when readyState transitions to "closing"?

@lgrahl
Copy link
Contributor Author

lgrahl commented Oct 21, 2018

We discussed that before, see #1827 (comment) (but I also forgot that we had this discussion 😅). It is unknown whether there is any application relying on that mechanism.

Another suboptimal alternative would be to say "well, .send can throw if the remote is closing, deal with it".

The least breaking would be to add an onclosing event but... I'm really reluctant to add a new event for a corner case that 99% of applications don't care about.

@jhaenchen
Copy link

I use the onclose event to discern if I should reinitiate a connection. If a connection fails and the onclose event doesn’t fire, the only way I can tell is to monitor the ice connection state for a particular state which seems more brittle than relying on an event.

And it seems that onclose may not fire for user-initiated events either? I’m struggling to understand how best to utilize this event handler.

@jan-ivar
Copy link
Member

@jhaenchen are you still basing observations on Chrome? Do you get different results in Firefox? This sounds like #1020 (comment) again.

@jan-ivar
Copy link
Member

onclosing seems like the simplest solution to me.

@jhaenchen
Copy link

@jan-ivar that does seem like the bug I’m experiencing. Didn’t see the comment you referenced either, thanks for bringing that to my attention!

@aboba
Copy link
Contributor

aboba commented Oct 25, 2018

Outcome of TPAC was to add "onclosing".

alvestrand added a commit that referenced this issue Jul 8, 2019
As per October 2018 TPAC decision.
Fixes #1827
@alvestrand alvestrand added Needs Test Needs a WPT test PR exists and removed Needs PR Discussion has converged - Pull Request needed Ready for PR labels Jul 8, 2019
@monktan89
Copy link

m

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Test Needs a WPT test PR exists
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants