Webrtc-pc: Is close() supposed to fire state change events? Connection state change set asynchronously?

[henbos]

Supposedly, calling close() implies...

• signaling state changes to "closed", this is explicit but _no signalingstatechange event is fired_.
• ICE connection state changes to "closed", this is not explicit but destroying the ICE agent is.
• connection state changes to "closed", this is not explicit but destroying the ICE agent is.

Did we simply forget to set the ICE connection state and connection state to "closed" in the spec, or is this supposed to be covered by destroying the ICE agent?

Without setting the ICE connection state and the connection state explicitly it is unclear if this is supposed to happen synchronously or asynchronously and whether or not events should fire. For example, update ICE connection state is invoked _asynchronously_ when the ICE agent indicates the state has changed and it also fires an event. But it is not clear that these steps should be executed when the ICE agent is destroyed.

Possible spec bugs:

• ICE connection state and connection state are set asynchronously, but signaling state is set synchronously.
• ICE connection state and connection state changing fires events, but signaling state changing does not.

I would think that invoking a synchronous API should set all three of these states synchronously and not fire any events.

Otherwise if it is intended that some states are updated synchronously and some are updated asynchronously it makes sense that the asynchronous changes does still fire events, but it got me a bit confused.

[jan-ivar]

Looks like we decided this before: https://github.com/w3c/webrtc-pc/issues/1799#issuecomment-372490350

[jan-ivar]

signaling state is set synchronously.

Signaling state is set in SLD/SRD and tracks the state of those methods essentially. We fire lots of events synchronously in those methods before the promise resolves, so I think we need to continue doing so to maintain invariants. I filed https://github.com/w3c/webrtc-pc/issues/1885 to adjust the ordering slightly.

The other two are ICE agent dependent, so having them be asynchronous makes sense to me.

it is not clear that these steps should be executed when the ICE agent is destroyed.

I interpret the words "destroy" and "abrupt" in _"Destroy connection's ICE Agent, abruptly ending any active ICE processing and releasing any relevant resources"_, to mean we should explicitly not perform any ordered tear-down algorithm or fire any events.

[jan-ivar]

I would think that invoking a synchronous API should set all three of these states synchronously and not fire any events.

Agree. pc.close already says:

• _"Set the [[IceTransportState]] slot of each of connection's RTCIceTransports to "closed"."_

...which sets the internal slot directly, side-stepping the algorithm that normally sets it¹, as well as the other states. So it looks like we're missing:

• Set connection's ICE connection state to "closed".
• Set connection's connection state to "closed".

^{1. Search for "When the ICE Agent indicates that the RTCIceTransportState for an RTCIceTransport has changed"}

[henbos]

Thanks @jan-ivar !

[jan-ivar]

I think this closes the main issue raised. I guess please reopen or open individual issues for any remaining issues.

[eskimor]

Why would one change a property, but not fire the property_on_change event ... I really don't get that. It is especially annoying, as it does not even happen if the remote side calls close(). I relied on that to detect a proper "hang up" as opposed to a connection loss.

I guess it is pointless to complain now, doing it nonetheless .. and finding a workaround .

[lgrahl]

You're not alone with that perception. I find it extremely hard to wrap my head around the logic to "not fire when you have caused the event" because then I always wonder what is supposed to happen to all the other closely related moving parts. Where does the scope of "having caused the event" start and where does it end? If I close a peer connection, should data channel close events still fire? Should the state event on the SCTP transport fire? As someone who worked a ton with this stuff, I have encountered multiple game-breaking bugs in my own applications due to browsers changing the logic where I don't even know whether it's a bug fix or a new bug. Really, I am lost on that one. Personally, I would find it a lot simpler to just always fire and make no exceptions. Otherwise, we should have an event firing matrix.

Sorry for the rant. :heart:

[henbos]

close() causes signalingState, iceConnectionState and connectionState to be closed. Whether or not it makes sense to fire an event caused by an explicit synchronous API call (as oppose to the other usages of the listeners: callbacks from asynchronous operations) can be debated, but if the old behavior is easy to shim: when invoking close(), also invoke these event handlers.

It is especially annoying, as it does not even happen if the remote side calls close().

The remote endpoint closing is irrelevant to this issue. The only one than can cause your local PC to close is you explicitly calling the synchronous API close().

(There may currently be a bug in Chrome under investigation about RTCP BYE messages not being sent in some cases but that is not what this issue is about.)

[henbos]

(There may currently be a bug in Chrome under investigation about RTCP BYE messages not being sent in some cases but that is not what this issue is about.)

Looks like suspending the page can cause the pc to get closed, this is not defined in the spec, but trying to make Chrome compliant to this may have caused this regression.

[eskimor]

The remote endpoint closing is irrelevant to this issue. The only one than can cause your local PC to close is you explicitly calling the synchronous API close().

That's at least in current Chrome not true: When I close on the remote end - the local end state changes to close - just without an event happening.

But even locally, if I have a property and a property_on_change event, I expect the event to fire when the property changes, that's the only sensible implementation imho. Any state mirroring logic falls apart otherwise - e.g. an UI displaying the current state - will never display the state "closed".

Users are able to fix that (apart from the remote issue), by wrapping the API ... yes, but why implement an API that needs to be fixed by users? If it is important to distinguish between "user closed" and "closed for some other reason" - we should simply have the state reflect that distinction.

[eskimor]

Anyhow, I am a bit (two years) late to the party. Quick fix for me, is polling the state. Next project will just implement a data channel or something for signalling close events.

[henbos]

Are you sure? I can't make onsignalingstatechange due to remote pc closing, I wrote a codepen here:
https://codepen.io/henbos/pen/rNVJmNw?editors=1010
See any problem with it?

In any case, if close can happen for asynchronous reason, then the way the spec handles event suggests it would have the event fire for this reason as well. But as it happens, AFAIK any reason other than close() being called that causes close would be non-standard behavior.

[eskimor]

@henbos no, you are right - I tested in an application that actually has a side channel. So, there is just no signalling telling me that the remote end hung up? Thanks for the nice codepen! :-)

[fippo]

Signaling is the applications job. There is a dtls close alert but its not sent over a reliable channel (and resends might not happen if the browser is closed etc)

[jan-ivar]

So, there is just no signalling telling me that the remote end hung up?

@eskimor There is. Either use a datachannel:

const dc1 = pc1.createDataChannel("dummy", {negotiated: true, id: 0});
const dc2 = pc2.createDataChannel("dummy", {negotiated: true, id: 0});
await Promise.all([dc1, dc2].map(dc => new Promise(r => dc.onopen = r)));
pc2.close();
dc1.onclose = () => console.log("close fired"); // close fired

Works in Firefox and Chrome 82 (probably 80 as well, addIceCandidate errors look unrelated).
But not Safari. @youennf?

Had data channels always been included by default in WebRTC, then we could have done a simpler API for this, but they're not.

Or you can also use the newer dtls transport object:

const {sender} = pc1.addTransceiver("audio");
await new Promise(r => pc1.onsignalingstatechange = r);
const {transport} = sender;
await new Promise(r => transport.onstatechange = () => transport.state == "connected"
                                                       && r());
pc2.close();
transport.onstatechange = () => console.log(transport.state); // close

...which detects when the dtls connection drops, but that still only works in Chrome atm.

[fippo]

instead of the dtlsTransport this should also be visible on pc.connectionState (in chrome)

[eskimor]

I see, so the close event for data channels works as I would have expected it to work for PeerConnection. I guess I need to re-study the API a bit, now that things have stabilized. Thanks a lot for the insights!

[jan-ivar]

instead of the dtlsTransport this should also be visible on pc.connectionState (in chrome)

Actually no. The spec says of closed: _"The RTCPeerConnection object's [[IsClosed]] slot is true."_

...which means it does _not_ detect remote close. Pity. @henbos was this intentional?

[jan-ivar]

I'm going to answer my own question: I think it is intentional, because you may renegotiate any number of (future) transports (or data channels for that matter) under the umbrella of the RTCPeerConnection negotiation factory.

Thus there is no way to truly know when a remote party has called pc.close(), because that's not a network question.

[henbos]

There are things observable from remote close (RTCP BYE causing track.onmute - though not implemented in Chrome yet; and as discussed RTCDataChannel.onclose and RTCDtlsTransportState.onstatechange). But these events could also be triggered for other reasons - there is no event for the JavaScript notion of "I invoked close()", that is something the application would have to invent. And like @jan-ivar says, if any of these events fires that does not guarantee that the PC does not continue to get used in the future.

[henbos]

I filed #2489 for follow-up discussions