diff --git a/rtc/src/peer_connection/handler/datachannel.rs b/rtc/src/peer_connection/handler/datachannel.rs index 260bea94..9f27efbf 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,16 @@ 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); + } + } + // Do NOT clear data_channels here: public RTCDataChannel methods + // (label(), ready_state(), etc.) use unwrap() on map lookups, so + // clearing the map would panic if the application still holds a + // DataChannel handle after PeerConnection::close(). + flatten_errs(errs) } } diff --git a/rtc/src/peer_connection/handler/mod.rs b/rtc/src/peer_connection/handler/mod.rs index f212fca0..c85d3108 100644 --- a/rtc/src/peer_connection/handler/mod.rs +++ b/rtc/src/peer_connection/handler/mod.rs @@ -378,63 +378,87 @@ where // https://www.w3.org/TR/webrtc/#dom-rtcpeerconnection-close (step #3) self.signaling_state = RTCSignalingState::Closed; - // Try closing everything and collect the errors - // Shutdown strategy: - // 1. All Conn close by closing their underlying Conn. - // 2. A Mux stops this chain. It won't close the underlying - // Conn if one of the endpoints is closed down. To - // continue the chain the Mux has to be closed. - for_each_handler!(forward: process_handler!(self, handler, { - handler.close()?; + // Close handlers in reverse order (Endpoint→Interceptor→SRTP→DataChannel→SCTP→ + // DTLS→ICE→Demuxer) so that higher-level protocols (DataChannels, SCTP) shut down + // before the transports they depend on (DTLS, ICE). + // Collect all errors instead of short-circuiting so every handler gets a chance + // to clean up even if an earlier one fails. + let mut close_errs: Vec = vec![]; + for_each_handler!(reverse: process_handler!(self, handler, { + if let Err(e) = handler.close() { + close_errs.push(e); + } })); - let close_errs: Vec = vec![]; + // 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() - /* TODO: - if let Err(err) = self.interceptor.close().await { - close_errs.push(Error::new(format!("interceptor: {err}"))); - } + self.update_connection_state(true); - // 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(); - } + flatten_errs(close_errs) + } +} - // 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(); - } +#[cfg(test)] +mod tests { + use super::*; + use crate::peer_connection::RTCPeerConnectionBuilder; + use sansio::Protocol; - // 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}"))); - } + #[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); - // 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}"))); - } + pc.close().unwrap(); - // 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}"))); - } - */ + assert_eq!(pc.peer_connection_state, RTCPeerConnectionState::Closed); + assert_eq!(pc.signaling_state, RTCSignalingState::Closed); + } - self.update_connection_state(true); + #[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); + } - flatten_errs(close_errs) + #[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(); } } diff --git a/rtc/src/peer_connection/handler/sctp.rs b/rtc/src/peer_connection/handler/sctp.rs index 8f1d4f41..ffebaef5 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 drop the endpoint. + let sctp = &mut self.ctx.sctp_transport; + let mut errs = vec![]; + for assoc in sctp.sctp_associations.values_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) } }