fix(peer_connection): implement SCTP/DataChannel close on PeerConnection shutdown by nightness · Pull Request #76 · webrtc-rs/rtc

nightness · 2026-04-01T12:54:12Z

Summary

SctpHandler::close(): was a no-op (Ok(())). Now iterates all SCTP associations, calls assoc.close(AssociationError::LocallyClosed) on each, clears the associations map, and drops the endpoint.
DataChannelHandler::close(): was a no-op. Now calls dc.close() on each RTCDataChannelInternal (transitions state to Closed, closes the underlying SCTP stream). Does not clear the data_channels map to avoid panics in public RTCDataChannel accessors that unwrap() map lookups.
handler/mod.rs: removes the stale ~40-line async-era /* TODO: */ comment block that used .await syntax incompatible with this synchronous sans-IO handler. Replaced with a concise comment mapping each W3C close step to the handler that implements it.
Close order: handlers are now closed in reverse order (Endpoint→Interceptor→SRTP→DataChannel→SCTP→DTLS→ICE→Demuxer) so higher-level protocols shut down before their transport dependencies. Errors are collected rather than short-circuiting.

ICE (IceHandler::close() → agent.close()), DTLS (DtlsHandler::close() → dtls_transport.stop()), and interceptor (InterceptorHandler::close() → interceptor.close()) were already correctly implemented.

Test plan

cargo test -p rtc — all 246 tests pass (167 unit + 79 doc)
Unit tests for close: state transitions, idempotency, data channel map preservation, post-close accessor safety
Integration: open a DataChannel, close the PeerConnection, verify the channel transitions to Closed state and no SCTP timers remain running

🤖 Generated with Claude Code

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Implements proper shutdown behavior for SCTP/DataChannels when a PeerConnection closes, and updates handler close documentation to match the current synchronous design.

Changes:

Implemented SctpHandler::close() to close all SCTP associations, clear transport state, and drop the SCTP endpoint.
Implemented DataChannelHandler::close() to close all RTCDataChannelInternal instances and clear the channel map.
Replaced an outdated async-era /* TODO */ block with a concise comment describing which handlers implement W3C close steps.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
rtc/src/peer_connection/handler/sctp.rs	Adds real SCTP shutdown by closing associations and dropping the endpoint, aggregating close errors.
rtc/src/peer_connection/handler/datachannel.rs	Adds DataChannel shutdown that closes all channels and aggregates close errors.
rtc/src/peer_connection/handler/mod.rs	Replaces stale async TODO with a brief mapping from W3C close steps to handler `close()` methods.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

codecov · 2026-04-04T14:28:57Z

Codecov Report

❌ Patch coverage is 87.50000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.17%. Comparing base (9feb4a3) to head (58c9cf4).

Files with missing lines	Patch %	Lines
rtc/src/peer_connection/handler/datachannel.rs	85.71%	1 Missing ⚠️
rtc/src/peer_connection/handler/sctp.rs	88.88%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master      #76   +/-   ##
=======================================
  Coverage   71.17%   71.17%           
=======================================
  Files         442      442           
  Lines       67330    67344   +14     
=======================================
+ Hits        47922    47934   +12     
- Misses      19408    19410    +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Address Copilot review comments on PR webrtc-rs#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) <noreply@anthropic.com>

…ion shutdown 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 <noreply@anthropic.com>

- 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) <noreply@anthropic.com>

Address Copilot review comments on PR webrtc-rs#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) <noreply@anthropic.com>

nightness · 2026-04-10T05:26:03Z

Rebased onto upstream/master so this PR contains only its own changes. Previous branch structure caused merge conflicts when PRs were merged in sequence. Each PR is now independently mergeable.

nightness force-pushed the fix/peerconnection-close branch from d7d5a8b to 58c9cf4 Compare April 1, 2026 18:43

rainliu requested a review from Copilot April 4, 2026 14:07

Copilot AI reviewed Apr 4, 2026

View reviewed changes

Comment thread rtc/src/peer_connection/handler/sctp.rs Outdated

Comment thread rtc/src/peer_connection/handler/mod.rs Outdated

Comment thread rtc/src/peer_connection/handler/sctp.rs Outdated

Copilot started reviewing on behalf of rainliu April 4, 2026 14:20 View session

nightness requested a review from Copilot April 8, 2026 08:00

Copilot started reviewing on behalf of nightness April 8, 2026 08:00 View session

Copilot AI reviewed Apr 8, 2026

View reviewed changes

Comment thread rtc/src/peer_connection/handler/mod.rs Outdated

Comment thread rtc/src/peer_connection/handler/mod.rs Outdated

Comment thread rtc/src/peer_connection/handler/datachannel.rs Outdated

nightness and others added 3 commits April 10, 2026 00:15

nightness force-pushed the fix/peerconnection-close branch from 4160151 to bf36bbe Compare April 10, 2026 05:16

nightness mentioned this pull request Apr 10, 2026

Summary: 19 PRs ready for review #87

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(peer_connection): implement SCTP/DataChannel close on PeerConnection shutdown#76

fix(peer_connection): implement SCTP/DataChannel close on PeerConnection shutdown#76
nightness wants to merge 3 commits into
webrtc-rs:masterfrom
Brainwires:fix/peerconnection-close

nightness commented Apr 1, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Apr 4, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nightness commented Apr 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

nightness commented Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Apr 4, 2026

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nightness commented Apr 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nightness commented Apr 1, 2026 •

edited

Loading