Skip to content
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

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

Open
Snarpix opened this issue Nov 30, 2023 · 1 comment
Open

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

Snarpix opened this issue Nov 30, 2023 · 1 comment

Comments

@Snarpix
Copy link

Snarpix commented Nov 30, 2023

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
Copy link
Author

Snarpix commented Dec 6, 2023

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()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant