From 5f80bdc9e81b285645a4234c2f9ccd0fb87c687c Mon Sep 17 00:00:00 2001 From: nightness Date: Wed, 1 Apr 2026 07:53:52 -0500 Subject: [PATCH 1/3] fix(peer_connection): implement SCTP/DataChannel close on PeerConnection shutdown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SctpHandler::close() and DataChannelHandler::close() both returned Ok(()) without actually closing anything. On PeerConnection close, SCTP associations were left in a running state and data channels were never transitioned to Closed. - SctpHandler::close(): close all SCTP associations with LocallyClosed reason, then clear the associations map and drop the endpoint. - DataChannelHandler::close(): close each RTCDataChannelInternal instance (sets ready_state → Closed, closes underlying data channel stream), then clear the data channels map. - handler/mod.rs: remove stale async-era TODO comment block that cannot compile in this synchronous sans-IO handler; replace with a comment documenting which handler handles which W3C close step. ICE, DTLS, and interceptor close were already correctly implemented in their respective handlers. Co-Authored-By: Claude Sonnet 4.6 --- .../peer_connection/handler/datachannel.rs | 11 +++- rtc/src/peer_connection/handler/mod.rs | 51 +++---------------- rtc/src/peer_connection/handler/sctp.rs | 17 +++++-- 3 files changed, 31 insertions(+), 48 deletions(-) diff --git a/rtc/src/peer_connection/handler/datachannel.rs b/rtc/src/peer_connection/handler/datachannel.rs index 260bea94..97f51a4e 100644 --- a/rtc/src/peer_connection/handler/datachannel.rs +++ b/rtc/src/peer_connection/handler/datachannel.rs @@ -11,7 +11,7 @@ use crate::statistics::accumulator::RTCStatsAccumulator; use log::{debug, warn}; use sctp::PayloadProtocolIdentifier; use shared::TransportContext; -use shared::error::{Error, Result}; +use shared::error::{Error, Result, flatten_errs}; use std::collections::{HashMap, VecDeque}; use std::time::Instant; @@ -329,6 +329,13 @@ impl<'a> sansio::Protocol Result<()> { - Ok(()) + let mut errs = vec![]; + for dc in self.data_channels.values_mut() { + if let Err(e) = dc.close() { + errs.push(e); + } + } + self.data_channels.clear(); + flatten_errs(errs) } } diff --git a/rtc/src/peer_connection/handler/mod.rs b/rtc/src/peer_connection/handler/mod.rs index f212fca0..ea10c500 100644 --- a/rtc/src/peer_connection/handler/mod.rs +++ b/rtc/src/peer_connection/handler/mod.rs @@ -388,51 +388,16 @@ where handler.close()?; })); + // W3C WebRTC §close steps #4–#10 are implemented in individual handler close() methods: + // InterceptorHandler::close() → interceptor.close() + // DataChannelHandler::close() → closes all RTCDataChannelInternal instances + // SctpHandler::close() → closes all SCTP associations + endpoint + // DtlsHandler::close() → dtls_transport.stop() + // IceHandler::close() → ice_transport.agent.close() + // + // The for_each_handler!(forward: ...) loop above invokes each handler's close() in order. let close_errs: Vec = vec![]; - /* TODO: - if let Err(err) = self.interceptor.close().await { - close_errs.push(Error::new(format!("interceptor: {err}"))); - } - - // https://www.w3.org/TR/webrtc/#dom-rtcpeerconnection-close (step #4) - { - let mut rtp_transceivers = self.internal.rtp_transceivers.lock().await; - for t in &*rtp_transceivers { - if let Err(err) = t.stop().await { - close_errs.push(Error::new(format!("rtp_transceivers: {err}"))); - } - } - rtp_transceivers.clear(); - } - - // https://www.w3.org/TR/webrtc/#dom-rtcpeerconnection-close (step #5) - { - let mut data_channels = self.internal.sctp_transport.data_channels.lock().await; - for d in &*data_channels { - if let Err(err) = d.close().await { - close_errs.push(Error::new(format!("data_channels: {err}"))); - } - } - data_channels.clear(); - } - - // https://www.w3.org/TR/webrtc/#dom-rtcpeerconnection-close (step #6) - if let Err(err) = self.internal.sctp_transport.stop().await { - close_errs.push(Error::new(format!("sctp_transport: {err}"))); - } - - // https://www.w3.org/TR/webrtc/#dom-rtcpeerconnection-close (step #7) - if let Err(err) = self.internal.dtls_transport.stop().await { - close_errs.push(Error::new(format!("dtls_transport: {err}"))); - } - - // https://www.w3.org/TR/webrtc/#dom-rtcpeerconnection-close (step #8, #9, #10) - if let Err(err) = self.internal.ice_transport.stop().await { - close_errs.push(Error::new(format!("ice_transport: {err}"))); - } - */ - self.update_connection_state(true); flatten_errs(close_errs) diff --git a/rtc/src/peer_connection/handler/sctp.rs b/rtc/src/peer_connection/handler/sctp.rs index 8f1d4f41..6b3f8044 100644 --- a/rtc/src/peer_connection/handler/sctp.rs +++ b/rtc/src/peer_connection/handler/sctp.rs @@ -12,8 +12,8 @@ use datachannel::message::Message; use datachannel::message::message_channel_threshold::DataChannelThreshold; use log::{debug, warn}; use sctp::{ - AssociationEvent, AssociationHandle, ClientConfig, DatagramEvent, EndpointEvent, Event, - Payload, PayloadProtocolIdentifier, StreamEvent, + AssociationError, AssociationEvent, AssociationHandle, ClientConfig, DatagramEvent, + EndpointEvent, Event, Payload, PayloadProtocolIdentifier, StreamEvent, }; use shared::error::{Error, Result}; use shared::marshal::Unmarshal; @@ -467,7 +467,18 @@ impl<'a> sansio::Protocol Result<()> { - Ok(()) + // Close all SCTP associations and drain the endpoint so state machines shut down cleanly. + let sctp = &mut self.ctx.sctp_transport; + let mut errs = vec![]; + for (_, assoc) in sctp.sctp_associations.iter_mut() { + if let Err(e) = assoc.close(AssociationError::LocallyClosed) { + errs.push(e); + } + } + sctp.sctp_associations.clear(); + sctp.sctp_endpoint = None; + + shared::error::flatten_errs(errs) } } From e45e503aac8cc7736bb099c6d57fc489a45a0fbc Mon Sep 17 00:00:00 2001 From: nightness Date: Tue, 7 Apr 2026 11:31:03 -0500 Subject: [PATCH 2/3] fix: address Copilot review comments - Use values_mut() instead of iter_mut() to avoid unused key - Fix comment: "drop the endpoint" instead of "drain the endpoint" - Update W3C close steps comment to avoid misleading step range reference Co-Authored-By: Claude Opus 4.6 (1M context) --- rtc/src/peer_connection/handler/mod.rs | 6 ++++-- rtc/src/peer_connection/handler/sctp.rs | 4 ++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/rtc/src/peer_connection/handler/mod.rs b/rtc/src/peer_connection/handler/mod.rs index ea10c500..d8c3fb85 100644 --- a/rtc/src/peer_connection/handler/mod.rs +++ b/rtc/src/peer_connection/handler/mod.rs @@ -388,14 +388,16 @@ where handler.close()?; })); - // W3C WebRTC §close steps #4–#10 are implemented in individual handler close() methods: + // W3C WebRTC §close shutdown work is implemented in individual handler close() methods, + // including: // InterceptorHandler::close() → interceptor.close() // DataChannelHandler::close() → closes all RTCDataChannelInternal instances // SctpHandler::close() → closes all SCTP associations + endpoint // DtlsHandler::close() → dtls_transport.stop() // IceHandler::close() → ice_transport.agent.close() // - // The for_each_handler!(forward: ...) loop above invokes each handler's close() in order. + // The for_each_handler!(forward: ...) loop above invokes these handler close() methods + // in order. let close_errs: Vec = vec![]; self.update_connection_state(true); diff --git a/rtc/src/peer_connection/handler/sctp.rs b/rtc/src/peer_connection/handler/sctp.rs index 6b3f8044..ffebaef5 100644 --- a/rtc/src/peer_connection/handler/sctp.rs +++ b/rtc/src/peer_connection/handler/sctp.rs @@ -467,10 +467,10 @@ impl<'a> sansio::Protocol Result<()> { - // Close all SCTP associations and drain the endpoint so state machines shut down cleanly. + // Close all SCTP associations and drop the endpoint. let sctp = &mut self.ctx.sctp_transport; let mut errs = vec![]; - for (_, assoc) in sctp.sctp_associations.iter_mut() { + for assoc in sctp.sctp_associations.values_mut() { if let Err(e) = assoc.close(AssociationError::LocallyClosed) { errs.push(e); } From bf36bbe8ea2ffc10d3f8d50202799936f76db24f Mon Sep 17 00:00:00 2001 From: nightness Date: Wed, 8 Apr 2026 09:14:56 -0500 Subject: [PATCH 3/3] fix: reverse handler close order, preserve data_channels map, add tests Address Copilot review comments on PR #76: - Close handlers in reverse order (Endpoint->DataChannel->SCTP->DTLS->ICE->Demuxer) so higher-level protocols shut down before their transport dependencies. - Collect all close errors instead of short-circuiting on the first failure, ensuring every handler gets a chance to clean up. - Remove data_channels.clear() from DataChannelHandler::close() to prevent panics in public RTCDataChannel methods (label(), ready_state(), etc.) that unwrap() map lookups. - Update comment to accurately describe reverse shutdown order. - Add 4 unit tests covering close state transitions, idempotency, data channel map preservation, and post-close accessor safety. Co-Authored-By: Claude Opus 4.6 (1M context) --- .../peer_connection/handler/datachannel.rs | 5 +- rtc/src/peer_connection/handler/mod.rs | 85 ++++++++++++++++--- 2 files changed, 75 insertions(+), 15 deletions(-) diff --git a/rtc/src/peer_connection/handler/datachannel.rs b/rtc/src/peer_connection/handler/datachannel.rs index 97f51a4e..9f27efbf 100644 --- a/rtc/src/peer_connection/handler/datachannel.rs +++ b/rtc/src/peer_connection/handler/datachannel.rs @@ -335,7 +335,10 @@ impl<'a> sansio::Protocol = vec![]; + for_each_handler!(reverse: process_handler!(self, handler, { + if let Err(e) = handler.close() { + close_errs.push(e); + } })); - // W3C WebRTC §close shutdown work is implemented in individual handler close() methods, - // including: + // W3C WebRTC §close shutdown work is implemented in individual handler close() methods: // InterceptorHandler::close() → interceptor.close() // DataChannelHandler::close() → closes all RTCDataChannelInternal instances // SctpHandler::close() → closes all SCTP associations + endpoint // DtlsHandler::close() → dtls_transport.stop() // IceHandler::close() → ice_transport.agent.close() - // - // The for_each_handler!(forward: ...) loop above invokes these handler close() methods - // in order. - let close_errs: Vec = vec![]; self.update_connection_state(true); flatten_errs(close_errs) } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::peer_connection::RTCPeerConnectionBuilder; + use sansio::Protocol; + + #[test] + fn close_transitions_to_closed_state() { + let mut pc = RTCPeerConnectionBuilder::new().build().unwrap(); + assert_ne!(pc.peer_connection_state, RTCPeerConnectionState::Closed); + assert_ne!(pc.signaling_state, RTCSignalingState::Closed); + + pc.close().unwrap(); + + assert_eq!(pc.peer_connection_state, RTCPeerConnectionState::Closed); + assert_eq!(pc.signaling_state, RTCSignalingState::Closed); + } + + #[test] + fn close_is_idempotent() { + let mut pc = RTCPeerConnectionBuilder::new().build().unwrap(); + pc.close().unwrap(); + // Second close should be a no-op (early return for already-closed) + pc.close().unwrap(); + assert_eq!(pc.peer_connection_state, RTCPeerConnectionState::Closed); + } + + #[test] + fn close_preserves_data_channels_in_map() { + let mut pc = RTCPeerConnectionBuilder::new().build().unwrap(); + // Create a data channel before closing + let dc = pc.create_data_channel("test", None).unwrap(); + let dc_id = dc.id(); + assert!(pc.data_channels.contains_key(&dc_id)); + + pc.close().unwrap(); + + // Data channels must remain in the map so that public RTCDataChannel + // methods (label(), ready_state(), etc.) which use unwrap() don't panic. + assert!( + pc.data_channels.contains_key(&dc_id), + "data_channels map must not be cleared on close" + ); + } + + #[test] + fn close_with_data_channel_does_not_panic() { + let mut pc = RTCPeerConnectionBuilder::new().build().unwrap(); + let dc = pc.create_data_channel("my-dc", None).unwrap(); + let dc_id = dc.id(); + pc.close().unwrap(); + + // After close, public accessors on the data channel must still work + // (they unwrap() the map entry). + let dc = pc.data_channel(dc_id).unwrap(); + assert_eq!(dc.label(), "my-dc"); + let _ = dc.ready_state(); + } +}