Open
Conversation
boivie
force-pushed
the
feature-improve-stream-reset
branch
from
5c3ffda to
b48ff88
Compare
With stream_reset.rs extracted into its own module, it's easier to create unit tests that verify edge cases etc. The existing C++ unit tests from stream_reset_handler_test.cc could now be migrated. To make tests more readable, a fixture was created that could be re-used in similar modules. For now, keep them private until we add more test coverage to the other modules. Some test cases could not easily be converted 1:1 as the C++ implementation works with the send queue as an interface (and mocks it), while the Rust implementation doesn't use mocks.
The current implementation of the Stream Reset Handler strictly validates incoming Re-configuration Request Sequence Numbers. If a request is a retransmission (same sequence number), it immediately returns the cached response, which was how RFC6525 was interpreted. However, per recent discussions (and there will be an errata to clarify this), if a request was previously responded to with "In Progress" (because the Sender's Last Assigned TSN hadn't been reached), the receiver must re-evaluate the condition upon receiving a retransmission. The current implementation creates a deadlock with peers that does this: The peer retransmits the same sequence number, and dcSCTP keeps replying "In Progress" from its cache without checking if the TSN condition is now met. This CL relaxes the validation logic. If a request is a retransmission and the last result was "In Progress", the handler will now re-evaluate the state (check the cumulative ack against the sender's last assigned TSN). If the condition is met, it proceeds to reset the streams; otherwise, it returns "In Progress" again. This change maintains backward compatibility with older peers (like current dcSCTP senders) that increment the sequence number when retrying after an "In Progress" response. This is a Rust backport of https://webrtc-review.googlesource.com/c/src/+/427361
Separate handling of (incoming, outgoing) requests, and responses.
boivie
force-pushed
the
feature-improve-stream-reset
branch
3 times, most recently
from
7e06093 to
d6176ef
Compare
RFC6525 requires that if a Re-configuration Request is responded to
with "In Progress", the sender should wait and then retransmit the same
request (with the same Request Sequence Number). Previously, dcSCTP
would treat an "In Progress" response as a trigger to retry with a new
sequence number (incremented). This behavior was initially kept for
compatibility while the receiver-side fix (to handle retransmissions
correctly) was being rolled out.
Now that receivers can correctly handle retransmissions of requests that
were previously "In Progress" (by re-evaluating the conditions), this CL
updates the sender to be compliant with the RFC.
When a Stream Reset Request receives an "In Progress" response:
1. The timer is restarted.
2. The request is re-sent with the same sequence number (instead of
incrementing it).
3. The retransmission is NOT counted as an error (timeout). This is
crucial because "In Progress" is a valid state, not a failure, and
counting it as an error could lead to premature connection tear-down.
This completes the work to make Stream Reset fully compliant with
RFC6525 regarding "In Progress" handling.
This is a Rust backport of
https://webrtc-review.googlesource.com/c/src/+/443941.
boivie
force-pushed
the
feature-improve-stream-reset
branch
from
d6176ef to
9d93b89
Compare
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
1 participant
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.
With commit 823705f, stream handling was extracted to its own module, similar to how to C++ module was initially written. With that, it was possible to backport the stream reset handling unit tests to this Rust implementation. With that, some additional validation was added that was missing in the Rust version.
The C++ version has had two recent improvements to become compliant with RFC6525, which was identified in https://issues.webrtc.org/issues/379844648. As this was a breaking change, this was rolled out into two parts in the C++ version: One part that relaxed sequence number handling for incoming requests, to support both the old implementation and the RFC compliant behavior. This was rolled out to Chrome 144. Now with Chrome 146 to be released next, the next part was submitted to the C++ version, to use the correct sequence number when sending out requests.
These two changes have now been backported to the Rust version.
Also, the code has been refactored, to make it easier to maintain.
Please try to enjoy each commit equally.