webrtc icon indicating copy to clipboard operation
webrtc copied to clipboard

RTCDataChannel::close() does not wait for channel to close

Open Snarpix opened this issue 1 year ago • 1 comments

RTCDataChannel::close() calls DataChannel::close(), which calls Stream::shutdown() which calls Stream::send_reset_request().

Stream::send_reset_request() does not wait for connection close. So there is no way to wait for shutdown handshake to complete.

In my code I call RTCPeerConnection::close() after RTCDataChannel::close() and since we there is no wait for buffered messages to be sent some messages will be lost.

Snarpix avatar Nov 30 '23 15:11 Snarpix

It looks like the DataChannel::close() is incorrect. Comment says that only outgoing streams should be reset, but it shutdowns both streams.

    /// Close closes the DataChannel and the underlying SCTP stream.
    pub async fn close(&self) -> Result<()> {
        // https://tools.ietf.org/html/draft-ietf-rtcweb-data-channel-13#section-6.7
        // Closing of a data channel MUST be signaled by resetting the
        // corresponding outgoing streams [RFC6525].  This means that if one
        // side decides to close the data channel, it resets the corresponding
        // outgoing stream.  When the peer sees that an incoming stream was
        // reset, it also resets its corresponding outgoing stream.  Once this
        // is completed, the data channel is closed.  Resetting a stream sets
        // the Stream Sequence Numbers (SSNs) of the stream back to 'zero' with
        // a corresponding notification to the application layer that the reset
        // has been performed.  Streams are available for reuse after a reset
        // has been performed.
        Ok(self.stream.shutdown(Shutdown::Both).await?)
    }

Also Stream::shutdown() calls send_reset_request only when both channels are shutdown.

    pub async fn shutdown(&self, how: Shutdown) -> Result<()> {
        if self.read_shutdown.load(Ordering::SeqCst) && self.write_shutdown.load(Ordering::SeqCst) {
            return Ok(());
        }

        if how == Shutdown::Write || how == Shutdown::Both {
            self.write_shutdown.store(true, Ordering::SeqCst);
        }

        if (how == Shutdown::Read || how == Shutdown::Both)
            && !self.read_shutdown.swap(true, Ordering::SeqCst)
        {
            self.read_notifier.notify_waiters();
        }

        if how == Shutdown::Both
            || (self.read_shutdown.load(Ordering::SeqCst)
                && self.write_shutdown.load(Ordering::SeqCst))
        {
            // Reset the stream
            // https://tools.ietf.org/html/rfc6525
            self.send_reset_request(self.stream_identifier).await?;
        }

        Ok(())
    }

If I have understood correctly, there should be a way to shutdown write side, and call send_reset_request. After that we can read the Stream until EOF which confirms that receiving end have acknowledged the shutdown. Read side will be shut down in AssociationInternal::unregister_stream()

Snarpix avatar Dec 06 '23 13:12 Snarpix