Leak in Worker::mapChannelRequestHandlers and other maps

[ibc]

Bug Report

Your environment

• mediasoup version: 3.10.5

Issue 1 description

By looking at where we call OnChannelRequestHandlerRemoved() I'm pretty sure that we are leaking an entry in Worker::mapChannelRequestHandlers when we close a Router via Node router.close() method and such a Router contains RtpObservers.

this->listener->OnChannelRequestHandlerRemoved(rtpObserver->id); is just called in Router when receiving a Channel::ChannelRequest::MethodId::ROUTER_CLOSE_RTP_OBSERVER request. However we do not send that channel request when we close the Router directly via router.close(), so here a leak.

Regarding other entities (for instance, Consumer) we rely on Router::OnTransportConsumerClosed() to clean the entry in the map when the Consumer is closed, but right now I do not remember if such a Router::OnTransportConsumerClosed is called in every case. For example, if we call transport.close():

• OnTransportConsumerClosed() is called in Transport::CloseProducersAndConsumers(), so the same: I'm not sure if Transport::CloseProducersAndConsumers() is always called when closing the Transport directly or when closing the Router directly`.

Some applies to Producer and DataXxxxxxxx classes, and not sure about transports.

Issue 2 description

Now a different potential issue. Imagine this scenario:

• Worker is creating an entity (Producer or whatever).
• It calls OnChannelRequestHandlerRemoved(producerId) so it gets into the map.
• Next steps in Producer creation fail so the code throws.
• We have that producerId in the map forever.

Proposal that would fix all issues

Entities inherit from ChannelSocket and so on. Such a class may implement a constructor and a destructor so the constructor is the only one that calls OnChannelRequestHandlerAdded(this) and the destructor calls OnChannelRequestHandlerRemoved(this). We do something similar somewhere else to register and unregister things. I think this mechanism would be much safer and reliable than current one.

[ibc]

Proposal that would fix all issues

Entities inherit from ChannelSocket and so on. Such a class may implement a constructor and a destructor so the constructor is the only one that calls OnChannelRequestHandlerAdded(this) and the destructor calls OnChannelRequestHandlerRemoved(this). We do something similar somewhere else to register and unregister things. I think this mechanism would be much safer and reliable than current one.

Possible C++ problem here:

• Let's have class B that inherits from A.
• We define both a constructor and destructor for A and B.
• If the constructor of A throws, the destructor of A is not called, and I'm not sure if the destructor of B is called in that case.

Anyway, IMHO the solution is:

• Do not declare constructor and destructor in ChannelSocket class.
• Call this->OnChannelRequestHandlerAdded(this) at the end of the constructor (of Producer, etc), so it cannot throw anymore unless duplicated id, but in that case the destructor of Producer won't be called so cleanup must be done manually in a catch block in the constructor of Producer.
• Call this->OnChannelRequestHandlerRemoved(this) at the end of the destructor (of Producer, etc) and also in the catch block of the constructor of Producer.

[nazar-pc]

Makes sense to me, I didn't think about it. Do you have capacity to implement this before I do by any chance?

[ibc]

I can try tomorrow.

BTW reminder for me: include those Worker maps in dump() output so we can test their values in tests.

[ibc]

Working in #894