versatica / mediasoup-client

mediasoup client side JavaScript library
https://mediasoup.org
ISC License
591 stars 237 forks source link

producer.close() throws unhandled rejection if called when PeerConnection's signalingState is "closed" #274

Closed ibc closed 1 year ago

ibc commented 1 year ago
CleanShot-2023-07-13-at-14 24 17@2x

So if producer.close() has been called and later Transport calls handler.stopSending() this happens in Chrome111 (and probably in many other handlers):

async stopSending(localId: string): Promise<void>
{
        this.assertSendDirection();

        logger.debug('stopSending() [localId:%s]', localId);

        const transceiver = this._mapMidTransceiver.get(localId);

        if (!transceiver)
        {
            throw new Error('associated RTCRtpTransceiver not found');
        }

        transceiver.sender.replaceTrack(null);

        this._pc.removeTrack(transceiver.sender);

        const mediaSectionClosed =
            this._remoteSdp!.closeMediaSection(transceiver.mid!);

and this._pc.removeTrack(transceiver.sender); will throw with "DOMException: The peer connection is closed".

It happens here in Transport.ts:

CleanShot-2023-07-13-at-14 27 35@2x

This block is just wrong:

this._awaitQueue.push(
                async () => this._handler.stopSending(producer.localId),
                'producer @close event')
                .catch((error: Error) => logger.warn('producer.close() failed:%o', error));

Here we are NOT doing await this._handler.stopSending(producer.localId) or this._handler.stopSending(producer.localId).catch(() => {}) so if it rejects then an unhandled exception happens.

So possible solutions:

Solution 1

In all methods in handlers (specially in stopSending() check if this._pc.signalingState === 'closed' and if so do nothing (early return).

Solution 2

In the producer.on('@close') listener in Transport.ts add await to this._handler.stopSending(producer.localId). Problems:

  1. It will make things slower since the queue must wait for the stopSending() method to resolve (or reject) before it processes next enqueued task.
  2. It will still show the warning "producer.close() failed". We don't want to show that error since it's not legitimate. We closed the Producer already so it shouldn't happen.

Proposed Solution

A mix between both above:

WOW!

But this is funny because right now the queue is NOT enqueing anything. It's just enqueing NON async methods since there is no await at all. To be clear, this._awaitQueue.push(async () => this._handler.stopSending(producer.localId) ... is like introducing a sync function in the queue. The task given to AwaitQueue is async () => this._handler.stopSending(producer.localId). This is gonna return immediately. For it to really wait until the task completes, it should be async () => await this._handler.stopSending(producer.localId).