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

Unclear whether XxxTransport is really closed or not #338

Closed
ibc opened this Issue Jan 9, 2016 · 8 comments

Comments

Projects
None yet
3 participants
@ibc
Copy link
Contributor

ibc commented Jan 9, 2016

In WebRTC 1.0 calling peerconnection.close() is required even if it fires a "failed" "oniceconnectionstatechange" event, otherwise it is not freed.

In ORTC, and with a recent change, a RTCDtlsTransport may fire a "closed" ondtlsstatechange event (without the user having called stop() on it).

Also, a RTCIceTransport may fire "failed" or "closed" onicestatechange...

So, when should the user assume that a RTCDtlsTransport and a RTCIceTransport has been definitely closed (and "freed")?

If for example the remote peer sends me a DTLS close alert and my RTCDtlsTransport fires "closed" ondtlsstatechange, should I call stop() on it or not?

@ibc

This comment has been minimized.

Copy link
Contributor

ibc commented Jan 14, 2016

I would extend this question to all the object within the ORTC spec. The draft should clearly define the lifetime of them and clarify which method and/or events free the object.

@aboba

This comment has been minimized.

Copy link
Contributor

aboba commented Feb 29, 2016

If dtlsTransport.state is already "closed" then calling stop() will not change the value of state. The dtlsTransport will not be garbage collected until references are removed, so calling stop() has no effect and is not necessary.

@aboba aboba added the question label Feb 29, 2016

@ibc

This comment has been minimized.

Copy link
Contributor

ibc commented Feb 29, 2016

Clear. However, and given that RTCPeerConnection is not GC'd until stop() is called, I suggest adding the above information into the ORTC draft.

aboba added a commit that referenced this issue Feb 29, 2016

@aboba aboba added PR exists and removed question labels Feb 29, 2016

@robin-raymond

This comment has been minimized.

Copy link
Contributor

robin-raymond commented Mar 1, 2016

I think there is confusion about an object being able to garbage collect its internal state vs when an object itself can be GC'ed. In ORTC, an object's internal state can be GC'ed when it goes into the final state (eg. "closed"). Each ORTC object itself can be GCed when no further reference to the object exists. The caveat is that some ORTC objects keep a reference to other ORTC objects. For example, the programmer holding a reference to the DataChannel indirectly also holds a reference to the SctpTransport, DtlsTransport, IceTransport and IceGatherer. So which are we trying to clarify here as I think the documentation for this is different depending on the context.

@robin-raymond

This comment has been minimized.

Copy link
Contributor

robin-raymond commented Mar 1, 2016

Further comment: The pull request #400 is accurate but is missing some important information. We need to define which ORTC objects internally keep references to other ORTC objects to the GC is very clear.

@robin-raymond robin-raymond removed the PR exists label Mar 1, 2016

@ibc

This comment has been minimized.

Copy link
Contributor

ibc commented Mar 1, 2016

I think there is confusion about an object being able to garbage collect its internal state vs when an object itself can be GC'ed. In ORTC, an object's internal state can be GC'ed when it goes into the final state (eg. "closed"). Each ORTC object itself can be GCed when no further reference to the object exists.

Yes of course, that's how any JS object works. My concerns were just about the need or not to call close()/stop() on ORTC objects. As said above, in WebRTC 1.0 RTCPeerConnection.stop() MUST be called to allow GC even if it has no references and somehow failed (ICE or DTLS failed, remote DTLS close, etc).

@aboba aboba added PR exists and removed PR exists labels Mar 21, 2016

@aboba

This comment has been minimized.

Copy link
Contributor

aboba commented Apr 2, 2016

@robin-raymond What do you think is needed to resolve this issue?

@robin-raymond

This comment has been minimized.

Copy link
Contributor

robin-raymond commented Apr 7, 2016

I believe this was resolved in #400 . Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment