Closing peer connection threads

[pablito25sp]

When creating a new PeerConnection without a PeerConnectionFactory, network, signaling and worker threads are created.
After closing the peer connection, Handler will close those threads if they exist after receiving the OnSignalingChange for "close" state.

[ibc]

I like it. @jmillan?

[jmillan]

Thanks @pablito25sp!

[pablito25sp]

You're welcome! Great work of all of you 👏🏻

[jmillan]

@pablito25sp,

The tests hang up for ever after this change.

If I kill the process it seems that OnTransportClose was never fired.

Please take a look at it.

NOTE: The test binary resides in build/test/ directory.

[pablito25sp]

Ok.. Will do. I did not run the test obviously, sorry about that.
Anyway it's something weird because on my implementation, callbacks are correctly triggered.
I will let you know.

[pablito25sp]

Hey guys, I'm not being able to run tests on my laptop (an M1).

And about what I've said before about the callbacks being triggered, I was not accurate. I'm triggering callbacks on my implementation when receiving the OnConnectionStateChange.

I've found some similar issues here: https://stackoverflow.com/questions/51275714/webrtc-peerconnectionfactory-graceful-shutdown

My question is, would be appropriate to move producer->TransportClosed(); out of SendTransport::Close() (same for RecvTransport::Close()) in Transport.cpp? And effectively call those callbacks when Handler receives (and pass to the Transport) the closed state?

I can update the PR with this if it makes sense for you.

[jmillan]

Hi @pablito25sp,

Deleting the threads on PeerConnection destructor is fine. We are creating them on construction, and doing the oposite on destruction will remove any leak and be consistent.

In your approach it seems that PeerConnection::Close() gets blocked if the signaling thread has been removed before.

[jmillan]

Could you try the threads removal on PeerConnection destructor?

[jmillan]

I've reverted the commit from v3 for now.

[jmillan]

Regarding to the use-after-free, yes, we need to run valgrind on the the tests. Thanks for the reminder!

[pablito25sp]

Could you try the threads removal on PeerConnection destructor?

Will do. Thanks for the hint.

[pablito25sp]

Opened a new one: #122
Instead of writing a destructor, I've just assigned nullptr to the threads when constructor is called without PeerConnection::Options. Looks clearer to me.