fix(datachannel): send DataChannelOpen for pre-negotiated channels (closes #61)#70
Open
nightness wants to merge 13 commits into
Open
fix(datachannel): send DataChannelOpen for pre-negotiated channels (closes #61)#70nightness wants to merge 13 commits into
nightness wants to merge 13 commits into
Conversation
nightness
force-pushed
the
fix/negotiated-datachannel
branch
from
09db64c to
1dea4fa
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #70 +/- ##
==========================================
- Coverage 71.17% 71.17% -0.01%
==========================================
Files 442 442
Lines 67330 67334 +4
==========================================
+ Hits 47922 47923 +1
- Misses 19408 19411 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses an SCTP stream registration gap that prevented sending messages on pre-negotiated (out-of-band) DataChannels, as reported in #61.
Changes:
- Always queue a DCEP
DATA_CHANNEL_OPENmessage inDataChannel::dial(), including fornegotiated=true. - In the SCTP write path, treat
ErrStreamAlreadyExistas non-fatal when processing outboundDATA_CHANNEL_OPEN, and proceed to update stream reliability parameters.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| rtc/src/peer_connection/handler/sctp.rs | Handles racing pre-negotiated DATA_CHANNEL_OPEN sends by tolerating ErrStreamAlreadyExist and still applying reliability params. |
| rtc-datachannel/src/data_channel/mod.rs | Changes dial() to always enqueue DATA_CHANNEL_OPEN so negotiated channels register/open SCTP streams and can send data. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
nightness
added a commit
to Brainwires/webrtc-rs-rtc
that referenced
this pull request
Apr 8, 2026
- Negotiated channels no longer send DCEP DataChannelOpen over the wire,
matching the W3C spec ("not announced in-band"). The outbound message
is flagged with `negotiated: true` so the SCTP handler opens the
stream locally but suppresses the wire write.
- Remove the ErrStreamAlreadyExist fallback (no longer needed since
negotiated channels don't race DCEP messages).
- Add `negotiated` field to DataChannelMessage for internal routing.
- Add two new tests: one verifying negotiated channels produce an
internal-only DCEP message, one verifying in-band channels still
send DCEP normally.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
nightness
added a commit
to Brainwires/webrtc-rs-rtc
that referenced
this pull request
Apr 8, 2026
…sertions) - Add #[non_exhaustive] to DataChannelMessage with a new() constructor to prevent semver-breaking changes from future field additions - Add negotiated_dial_duplicate_stream_returns_already_exist test covering the peer race scenario where both sides open the same pre-negotiated stream (ErrStreamAlreadyExist) - Strengthen error assertion in non_negotiated_datachannel_open test to check for specific ErrPayloadDataStateNotExist variant - Migrate all cross-crate DataChannelMessage construction to use new() Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Author
|
It's taking a little long than a previous message from another commit. I should have everything cleaned up in hopefully less than 24 hours. |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Fixes webrtc-rs#61 — negotiated DataChannels could open but not send. Root cause: DataChannel::dial() skipped queuing the DataChannelOpen DCEP message for pre-negotiated channels (negotiated=true). The SctpHandler calls conn.open_stream() only when it processes a DataChannelOpen write, so no stream was ever registered in the SCTP association. Any subsequent write failed with "Stream not existed". Fix: always queue DataChannelOpen in dial(), for both negotiated and non-negotiated channels. The DCEP exchange opens the SCTP stream on both sides. Also handle ErrStreamAlreadyExist in the DataChannelOpen write path: when both pre-negotiated peers race to send DataChannelOpen simultaneously the remote's message may auto-create the stream first via get_or_create_stream; treat that as non-fatal. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Negotiated channels no longer send DCEP DataChannelOpen over the wire,
matching the W3C spec ("not announced in-band"). The outbound message
is flagged with `negotiated: true` so the SCTP handler opens the
stream locally but suppresses the wire write.
- Remove the ErrStreamAlreadyExist fallback (no longer needed since
negotiated channels don't race DCEP messages).
- Add `negotiated` field to DataChannelMessage for internal routing.
- Add two new tests: one verifying negotiated channels produce an
internal-only DCEP message, one verifying in-band channels still
send DCEP normally.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The test only verifies that `dial()` flags the outbound message with `negotiated = true`; it does not exercise the SctpHandler path that suppresses the wire write. Rename from `test_data_channel_negotiated_no_dcep_on_wire` to `test_data_channel_negotiated_dial_flags_message` and update the doc comment to clarify scope. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add unit tests for the SctpHandler's DCEP dispatch to exercise the negotiated-channel wire-write suppression (lines 293-294 in sctp.rs). - negotiated_datachannel_open_suppresses_wire_write: verifies that handle_write succeeds when negotiated=true (the wire write is suppressed, avoiding the ErrPayloadDataStateNotExist that would occur if write_with_ppi were called on a non-established association) - non_negotiated_datachannel_open_attempts_wire_write: confirms the non-negotiated path does attempt the wire write (and fails on a non-established association), proving the negotiated flag is the deciding factor Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Adds an rtc-to-rtc integration test that creates negotiated DataChannels (negotiated: Some(5)) on both peers, connects them, and verifies bidirectional message exchange. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…sertions) - Add #[non_exhaustive] to DataChannelMessage with a new() constructor to prevent semver-breaking changes from future field additions - Add negotiated_dial_duplicate_stream_returns_already_exist test covering the peer race scenario where both sides open the same pre-negotiated stream (ErrStreamAlreadyExist) - Strengthen error assertion in non_negotiated_datachannel_open test to check for specific ErrPayloadDataStateNotExist variant - Migrate all cross-crate DataChannelMessage construction to use new() Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Replace debug string matching with matches! macro for ErrPayloadDataStateNotExist assertion in sctp.rs. Improve doc comment on dial() in mod.rs to force outdated the review thread on lines 81-95. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Adds a brief rustdoc comment to the private `new()` constructor describing its purpose and initial state, improving code readability. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add explanatory comment above #[non_exhaustive] noting it is intentional
for this pre-1.0 crate (touches mod.rs lines 29/35/40)
- Rename _a1 to a1 in test since it is used by close_association_pair
(touches data_channel_test.rs lines 223/259/260)
Note: several comments were already addressed in prior commits:
- mod.rs:28 grammar fix ("is used for data") already applied
- mod.rs:95 detailed DCEP/negotiated explanation already present
- data_channel_test.rs:303 trailing comment already removed
- sctp.rs:616/623/666/682 already using matches!() macro
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
nightness
force-pushed
the
fix/negotiated-datachannel
branch
from
5656216 to
8ff2f2c
Compare
Author
|
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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #61 — negotiated DataChannels open but cannot send messages.
Root cause:
DataChannel::dial()skipped theDataChannelOpenDCEP message whenconfig.negotiated == true. TheSctpHandleronly callsconn.open_stream()when processing aDataChannelOpenwrite, so no stream entry was ever created in the SCTP association. Every subsequent write failed with "Stream not existed".Fix (updated per review feedback):
DataChannel::dial()still queues aDataChannelOpenfor negotiated channels, but marks it withnegotiated: trueonDataChannelMessage. This lets the SCTP handler open the stream locally without sending the DCEP payload over the wire — matching the W3C spec that negotiated channels "will not be announced in-band".negotiatedfield toDataChannelMessage(marked#[non_exhaustive]with anew()constructor) to communicate the internal-only routing intent to the SCTP handler.ErrStreamAlreadyExistfallback (no longer needed since negotiated channels no longer race DCEP messages with the peer).Test plan
cargo test -p rtc-datachannelpasses (27 tests, including 2 new ones)cargo clippypassescargo fmt --checkpassescargo test -p rtcpasses (166 unit tests + 3 SCTP handler tests)negotiated: Some(5)) on both peers — both sides can exchange messages after connectionNew tests
rtc-datachannel unit tests
test_data_channel_negotiated_dial_flags_message— verifies negotiated channels produce an internal-only DCEP message (flaggednegotiated: true) and do not send DCEP over the wiretest_data_channel_non_negotiated_sends_dcep— verifies in-band channels still send a normal DCEP DataChannelOpen that can be accepted by the peerrtc SCTP handler tests
negotiated_datachannel_open_suppresses_wire_write— verifies the SCTP handler opens the stream but suppresses the wire write for negotiated channelsnon_negotiated_datachannel_open_attempts_wire_write— verifies in-band channels attempt the wire write (fails withErrPayloadDataStateNotExiston a non-established association)negotiated_dial_duplicate_stream_returns_already_exist— exercises the peer-race scenario where both sides open the same pre-negotiated stream ID, assertingErrStreamAlreadyExistrtc integration test
test_negotiated_data_channel_rtc_to_rtc— two rtc peers create negotiated DataChannels (negotiated: Some(5)) and exchange messages bidirectionally after connection