RTCPeerConnection object not clearing after participant leaves about rtc-quickconnect HOT 9 OPEN

@warrenjmcdonald @nathanoehlman
I managed to reproduce this using the latest quickconnect version with the latest rtc-switchboard code.
Just did on console :

1. node server => on rtc-switchboard //no changes here, localhost:3000

2. live-server (http web server) => on quickconnect/examples /bundle-demo.html

3. Added this code to the bundle-demo.html javascript
   https://github.com/rtc-io/rtc-quickconnect#example-usage-using-captured-media

Having two tabs open, upon refresh of client A, checking chrome://webrtc-internals/ there is always an RTCPeerConnection object hanging on client B belonging to client A.

This does not happen in much older versions of quickconnect which we use on production, I can see there how the peer connections get cleared.

from rtc-quickconnect.

@nathanoehlman we have investigated this quite thoroughly today to see what is causing the difference in behaviour in latest quickconnect and rtc-tools, to the older quickconnect and rtc-tools we have been using until recently.

We have eliminated PC signallingState and iceConnectionState
We feel that we have reproduced the correct clean up sequence.
We have tried manually removing the associated mediastreams

In the end we still have no idea what is causing the PC to not be properly cleaned up.

We have double checked the older quickconnect behaviour in the same browser version and it is cleaning up such that the PC objects are properly removed.

We are not sure where to look next but assume there is some reference to the object still in place which is preventing the cleanup such as an event handler etc.

from rtc-quickconnect.

There is behaviour that is intended to handle a graceful call exit here:

https://github.com/rtc-io/rtc-quickconnect/blob/master/index.js#L916

And and from the party that is leaving:

https://github.com/rtc-io/rtc-quickconnect/blob/master/index.js#L589
https://github.com/rtc-io/rtc-signaller/blob/master/index.js#L231

Similar to your suggestion, the leaving party sends a /leave message which should be received by the message:leave handler on the remote end. Which should result in calls.end, which should clean up the connection via rtc-tools:

https://github.com/rtc-io/rtc-quickconnect/blob/master/lib/calls.js#L110
https://github.com/rtc-io/rtc-quickconnect/blob/master/lib/calls.js#L157

In the case of an ungraceful exit, in the worst case the connection should be cleaned up by the heartbeat failing, unless it has been disabled.

https://github.com/rtc-io/rtc-quickconnect/blob/master/lib/calls.js#L194
https://github.com/rtc-io/rtc-quickconnect/blob/master/lib/heartbeat.js#L29

The current rtc-switchboard may generate a leave message when a peer disconnects, I'm not exactly sure atm.

Do you see the /leave message when .close() is called? Do you see the heartbeat failing if the remote peer just disappears?

from rtc-quickconnect.

@briely
We use rtc-signaller-socket-io. The line below never gets triggered:
https://github.com/rtc-io/rtc-signaller/blob/master/index.js#L231

from rtc-quickconnect.

That makes sense. Perhaps the /leave behaviour should be moved into rtc-signal.

from rtc-quickconnect.

Just confirming: you've disabled the heartbeat behaviour?

from rtc-quickconnect.

@magestican Just trying to replicate your example to reconstruct the behaviour you are seeing with chrome://webrtc-internals/. The behaviour you describe is as I would expect - webrtc-internals seems to display information about PeerConnections that were created in a browser instance up until the point of the instance being refreshed. As case in point is executing the following code, which is the simplest form of creating a connection, closing it, and removing it's references to make it garbage collectable:

var pc = new webkitRTCPeerConnection({ iceServers: []});
pc.close();
pc = undefined;

webrtc-internals will continue to show the details for this peer connection up until the peer connection is closed/refreshed.

Are you saying that this behaviour does not occur with older versions of quickconnect? If so, I have no idea how that'd be occurring. Looking for issues against Chrome shows https://bugs.chromium.org/p/chromium/issues/detail?id=356605, which seems to indicate that PeerConnections used to not be garbage collected due to being tied to the window object, but that should have been fixed a while ago. The bug itself seems to indicate that the webrtc-internals behaviour is as expected, but again, not sure how well it ties in.

from rtc-quickconnect.

@magestican @warrenjmcdonald @betimer Yep, using the rtc-signaller-socket.io isn't currently sending the /leave message that rtc-signaller does. As per the suggestion from @briely, I'll update rtc-signal/signaller to do this instead, then update rtc-signaller and rtc-signaller-socket.io to use this as the leave behaviour for both.

from rtc-quickconnect.

The other thing I should mention is that quickconnect does have an additional failsafe mechanism for removing peer connections that are no longer in operation.

Say for instance a peer has closed (due to a browser close, or unexpected disconnect). Each quickconnect peer connection is created with a monitor (https://github.com/rtc-io/rtc-tools/blob/master/monitor.js) that tracks the signaling and ICE connection state change events to ensure that a connection is still valid. In the event of a connection between two peers (A + B) where B is closed unexpectedly, the following should occur:

1. A will continue to believe the PeerConnection is open.
2. After a while, the browser will detect that the PeerConnection to B has failed, and the iceConnectionState on the connection held by A will transition to either failed, or closed.
3. The monitor will pick this up, and emit a closed event.
4. The closed event causes the logic at https://github.com/rtc-io/rtc-quickconnect/blob/master/index.js#L288 to be execute, causing the cleanup of the connection that A held.

With the ignoreDisconnect property set to true, this failsafe logic still seems to work correctly.

from rtc-quickconnect.