Most of the changes are trivial.
Bug: webrtc:14432
Change-Id: I0444527bf57c72c8d65f69754b4a4a1c1d7b2e92
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/275340
Reviewed-by: Erik Språng <sprang@webrtc.org>
Reviewed-by: Mirko Bonadei <mbonadei@webrtc.org>
Commit-Queue: Mirko Bonadei <mbonadei@webrtc.org>
Reviewed-by: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#38074}
This was a mistake from change 273800 in that it could try to send
packets if there wasn't a connection established - when tcb_ was
nullptr.
Bug: chromium:1360268
Change-Id: Idd4406071dbd8ac89303aef61840895505417569
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/274405
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Auto-Submit: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#38031}
The RTT will quickly be updated whenever a DATA chunk is acked, but
the initial value was set to zero. In unit tests, which often are
short and rarely increase the simulated time between a DATA is sent,
and acked, the smoothed RTT would usually stay at 0, which causes
e.g. the rate limiting of FORWARD not to work.
Bug: None
Change-Id: Ieb515fe875ce88d001777b00d6efd9762565a09d
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/273900
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#38013}
When re-receiving a stream reset request with the same request
sequence number, reply with the same result as previous time. In
case the original request was deferred, and "in progress" was
replied, it's important to not indicate that it was performed
successfully as that will make the sender believe it has completed
before it did.
Bug: webrtc:14277
Change-Id: I5c7082dc285180d62061d7ebebe05566e5c4195c
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/274080
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#38012}
When a stream reset response has been received, this may result
in unpausing the streams (either because it was successful or
because it failed - but streams will be unpaused). Directly after
receiving the response, send out any pending chunks that are
ready to be sent.
Before this CL, they would eventually be sent out, but that is
unnecessary and undeterministic.
Bug: webrtc:14277
Change-Id: Ic1ab38bc3cea96cfec7419e25001f12807523a3a
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/273800
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#38009}
When a RECONFIG has been received with a last assigned TSN that is
not yet seen by the receiver, it will enter deferred reset mode
(https://www.rfc-editor.org/rfc/rfc6525#section-5.2.2, E2).
When more DATA is received, moving the cumulative acknowledgment point,
the request will finally be processed. But the last chunk that has the
same TSN as the last assigned TSN was before this CL not applied before
doing the reset - it was applied after.
This would result of a message getting lost or possibly getting
truncated or incorrectly merged with another.
Handling the message before resetting the stream is the simple
solution here.
Bug: webrtc:14277
Change-Id: Iea9fa227778077a9ff2f78bc77b5d93cc32b702b
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/273323
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#37993}
To allow the transport to be able to know which ranges of
stream identifiers it can use, the negotiated incoming/inbound
and outgoing/outbound stream counts will be exposed. They are
added to Metrics, and guaranteed to be available from within
the OnConnected callback.
In this CL, dcSCTP will not validate that the client is sending
on a stream that is within the negotiated bounds. That will be
done as a follow-up CL.
Bug: webrtc:14277
Change-Id: Ic764e5f93f53d55633ee547df86246022f4097cf
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/272321
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#37876}
To allow the transport to be able to know which ranges of
stream identifiers it can be use, the negotiated incoming/inbound
and outgoing/outbound stream counts will be exposed.
This is first added to handover state, with the actual implementation
to follow.
Bug: webrtc:14277
Change-Id: Idd821ecbd8fcb588c88d69f617889318b4b03d43
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/272320
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#37863}
This adds the final piece, which makes the socket and the retransmission
queue generate the callbacks.
Bug: webrtc:5696
Change-Id: I1e28c98e9660bd018e817a3ba0fa6b03940fcd33
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/264125
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#37455}
For all messages where the last fragment was _not_ put on the wire, the
send queue is responsible for generating lifecycle events, but once all
fragments have been put on the wire, it's the retransmission queue that
is responsible. It does that by marking the final fragment of a message
with the lifecycle identifier, and once that message has been fully
acked by the cumulative ack TSN, it's clear that the peer has fully
received all fragments of the message, as the last fragment was acked.
For abandoned messages - where FORWARD-TSNs are sent, those will be
replied to by a SACK as well, and then we report abandoned messages
separately, to later trigger `OnLifecycleMessageExpired`.
This CL adds support in OutstandingData, which doesn't generate the
callbacks itself, but just reports them in the AckInfo.
Bug: webrtc:5696
Change-Id: I64092f13bcfda685443b7df9967b04d54aedd36a
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/264124
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#37448}
The send queue is responsible for generating lifecycle events for all
messages that are still in the queue. Because, if they are still in the
queue, that means that the last fragment of the message hasn't been sent
yet (because then it would have been in the retransmission queue
instead). And if the last fragment hasn't been sent, the send queue is
responsible for generating the
`OnLifecycleMessageExpired(/*maybe_sent=*/false)` event.
Bug: webrtc:5696
Change-Id: Icd5956d6aa0f392cae54f2a05bd20728d9f7f0a6
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/264144
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#37419}
Minor refactoring of the API, to put optional arguments last. Also
changed internal structures to reflect that order, for consistency.
Also reduced size of Item from 88 to 72 bytes, by packing fields better.
Bug: webrtc:5696
Change-Id: I1b9d50831a8e9a358224682d06a782a3269b8416
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/264123
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#37403}
Let the OutgoingStream reference the parent instead of passing
references to individual items it needs, as follow-up CLs will add even
more items.
No functional change - pure refactoring.
Bug: webrtc:5696
Change-Id: I914e590c0d90e898d7d230a16170cf4faff2338c
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/264142
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#37398}
This CL adds the API to enable message lifecycle events to be generated.
Those can in turn be used to generate metrics, e.g. latency metrics
tracking the time to send a message, the time until it's acknowledged,
and metrics tracking how often messages are expired.
This will be used to validate that message interleaving really improves
latency for high priority data channels.
The actual implementation of the API will be provided in follow-up CLs.
Bug: webrtc:5696
Change-Id: Ic06f8244d1c79a336975e35479130521dff17519
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/264141
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Reviewed-by: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#37396}
There was also some refactoring to create the TCB at the same time,
to ensure the metric is always set.
Bug: webrtc:13052, webrtc:5696
Change-Id: I5557ad5f0fc4a0520de1eaaafa15459b3200c4f5
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/262259
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#37388}
Before this CL, some components, e.g. the SendQueue, was first created
and then later restored from handover state, while some were created from
the handover state, as an optional parameter to their constructors.
This CL will make it consistent, by always creating the components in a
pristine state, and then modifying it when restoring them from handover
state. The name "RestoreFromState" was used to be consistent with SendQueue
and the socket.
This is just refactoring.
Bug: None
Change-Id: Ifad2d2e84a74a12a93abbfb0fe1027ebb9580e73
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/267006
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#37384}
This adds support to enable message interleaving in the stream scheduler
from the socket, proxied by the send queue.
It also adds socket unit tests to ensure that prioritization and
interleaving works. Also, send queue test has been added to validate the
integration of the stream scheduler. But the actual scheduling parts of
it will be tested in the stream scheduler unit tests.
Bug: webrtc:5696
Change-Id: Ic7d3d2dc28405c77a107f0148f0096882961eec7
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/262248
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#37355}
This mainly modifies the stream scheduler to add a weighted fair queuing
algorithm in addition to its round robin algorithm. The WFQ algorithm is
selected whenever interleaving is enabled, to ensure that the socket
stays backwards compatible in the normal (non-interleaved) scenario.
Adaptation to send queue and socket comes in a follow-up CL.
Bug: webrtc:5696
Change-Id: I8f0dbfa8c2f40f2e84cee536ea821e7ef4af6310
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/261947
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#37330}
This is a revert of the revert of commit d729d12454
which was reverted because it caused upstream test failures.
Contains fix in StreamScheduler::GetActiveStreamsForTesting.
This reverts commit 5df960d307.
Bug: webrtc:5696
Change-Id: I89dada257a6fb1f149f50067ab66b17e24a7c01a
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/266368
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#37299}
RFC 4960, 7.2.4. Fast Retransmit on Gap Reports
4) Restart the T3-rtx timer only if the last SACK acknowledged the
lowest outstanding TSN number sent to that address, or the
endpoint is retransmitting the first outstanding DATA chunk sent
to that address.
The second part of this sentence, "or the endpoint is retransmitting the
first outstanding DATA chunk sent to that address."
This means that on fast retransmit, and if the retransmitted chunks
weren't quickly acknowledged by the peer, the retransmission timer would
expire too quickly. With this CL, it will restart, as it should.
Bug: webrtc:12943
Change-Id: I7627253718530876acc516460562e66bfcc79533
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/265620
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#37193}
This reverts commit d729d12454.
Reason for revert: Breaks downstream project.
Original change's description:
> dcsctp: Use stream scheduler in send queue
>
> Changing the currently embedded scheduler that was implemented using a
> revolving pointer, to the parameterized stream scheduler that is
> implemented using a "virtual finish time" approach.
>
> Also renamed StreamCallback to StreamProducer, per review comments.
>
> Bug: webrtc:5696
> Change-Id: I7719678776ddbe05b688ada1b52887e5ca2fb206
> Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/262160
> Reviewed-by: Harald Alvestrand <hta@webrtc.org>
> Commit-Queue: Victor Boivie <boivie@webrtc.org>
> Cr-Commit-Position: refs/heads/main@{#37170}
Bug: webrtc:5696
Change-Id: Iaf3608b52a31eb31b4ca604539edb2e8ca89399b
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/265389
Auto-Submit: Victor Boivie <boivie@webrtc.org>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Bot-Commit: rubber-stamper@appspot.gserviceaccount.com <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#37172}
Changing the currently embedded scheduler that was implemented using a
revolving pointer, to the parameterized stream scheduler that is
implemented using a "virtual finish time" approach.
Also renamed StreamCallback to StreamProducer, per review comments.
Bug: webrtc:5696
Change-Id: I7719678776ddbe05b688ada1b52887e5ca2fb206
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/262160
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#37170}
This adds a stream scheduler using virtual finish time (as defined in
e.g. many Fair Queuing scheduler implementations), which indicates when
a stream's next sent packet is supposed to be sent.
In the initial version, this will be used to implement a round robin
scheduler, by emulating that a stream's virtual finish time - when
scheduled - is the "one more" than all existing virtual finish times.
That will make the scheduler simply iterate between the streams in
round robin order.
The stream scheduler component is tested in isolation, and follow-up
CLs will integrate it into the send queue.
Bug: webrtc:5696
Change-Id: Iaa2c204f9b9a00517f55355cb11cfd25bb415f9e
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/261946
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#37157}
Make HasDataToSend not mutate any state, and let the Produce method do
all state mutation and possibly indicate if there is nothing that can be
sent. This is helpful preparation for extracting the scheduling part of
the send queue to a separate component.
Bug: webrtc:5696
Change-Id: I132779e77d3ce6a41e5fcf4432140d3728d03cdc
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/261945
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#37141}
This is the receive-side part of supporting what is frequently called
"ndata", but actually RFC8260 - "User Message Interleaving".
This CL adds a new ReassemblyStreams implementation that can assemble
I-DATA chunks and process I-FORWARD-TSN for partial reliability.
Bug: webrtc:5696
Change-Id: I3cfbea62e7b6c02fbd3f51b43ba3fb7863cf0f88
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/218506
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#37128}
This is a re-land of commit 3180a5ad06.
This is an issue found in fuzzer, and doesn't really happen in WebRTC
as it never closes the connection and reconnects.
The issue is that the send queue outlives any connection since you're
allowed to send messages (well, enqueue them) before the association is
fully connected. So the send queue is always present but the TCB
(information about the connection) is torn down when the connection is
closed for example. And the TCB holds the Stream Reset handler, which is
responsible for e.g. keeping track of stream reset sequence numbers and
such - which is tied to the connection.
So to ensure that the Stream Reset Handler is in charge of deciding
if a stream reset is taking place, make sure that the send queue is in
a known good state when the Stream Reset handler is created.
Bug: webrtc:13994, chromium:1320194
Change-Id: Ib8254488523c7abb58057c602f76f411fce896fa
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/265000
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#37115}
The current code assumed that chunks that were scheduled for fast
retransmission would never be abandoned, as chunks marked for fast
retransmission would be immediately sent after the SACK has been
processed, giving no time for them to be abandoned.
But fuzzers keep on fuzzing, and can craft a sequence of chunks that
result in a SACK that both marks the chunks for fast retransmission and
later (while processing the same SACK) abandons them.
Bug: chromium:1331087
Change-Id: Id218607e18a6f3a9d6d51044dccb920e1e77372a
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/264960
Commit-Queue: Florent Castelli <orphis@webrtc.org>
Auto-Submit: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#37108}
This is a reland of commit 17a02a31d7.
This is the first part of supporting stream priorities, and adds the API
and very basic support for setting and retrieving the stream priority.
This commit doesn't in any way change the actual packet sending - the
specified priority values are stored, but not acted on.
This is all that is client visible, so clients can start using the API
as written, and they would never notice that things are missing.
Bug: webrtc:5696
Change-Id: I04d64a63cbaec67568496ad99667e14eba85f2e0
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/264424
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#37081}
Adding it as a separate CL to allow upstream code to support it.
Bug: webrtc:5696
Change-Id: I817a09e1b1121e5baf88b9922f84a2de245e6cc2
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/264447
Commit-Queue: Florent Castelli <orphis@webrtc.org>
Auto-Submit: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#37046}
This reverts commit 17a02a31d7.
Reason for revert: Breaks downstream test
Original change's description:
> dcsctp: Add public API for setting priorities
>
> This is the first part of supporting stream priorities, and adds the API
> and very basic support for setting and retrieving the stream priority.
>
> This commit doesn't in any way change the actual packet sending - the
> specified priority values are stored, but not acted on.
>
> This is all that is client visible, so clients can start using the API
> as written, and they would never notice that things are missing.
>
> Bug: webrtc:5696
> Change-Id: I24fce8cbb6f3cba187df99d1d3f45e73621c93c6
> Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/261943
> Reviewed-by: Harald Alvestrand <hta@webrtc.org>
> Commit-Queue: Victor Boivie <boivie@webrtc.org>
> Cr-Commit-Position: refs/heads/main@{#37034}
Bug: webrtc:5696
Change-Id: If172d9c9dbce7aae72152abbbae1ccc77340bbc1
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/264444
Owners-Override: Björn Terelius <terelius@webrtc.org>
Bot-Commit: rubber-stamper@appspot.gserviceaccount.com <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Björn Terelius <terelius@webrtc.org>
Auto-Submit: Björn Terelius <terelius@webrtc.org>
Reviewed-by: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#37039}
This is the first part of supporting stream priorities, and adds the API
and very basic support for setting and retrieving the stream priority.
This commit doesn't in any way change the actual packet sending - the
specified priority values are stored, but not acted on.
This is all that is client visible, so clients can start using the API
as written, and they would never notice that things are missing.
Bug: webrtc:5696
Change-Id: I24fce8cbb6f3cba187df99d1d3f45e73621c93c6
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/261943
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#37034}
This CL first restricts Metrics to be retrievable when the socket is
created. This avoids having most fields as optional and makes it
easier to add more metrics.
Secondly, the peer implementation is moved into Metrics.
Bug: webrtc:13052
Change-Id: I6cb53eeef3f84ac34f3efc883853338f903cc758
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/262256
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36888}
This was found during code review. This code is essentially dead code
until interleaved messaging is implemented, which is disabled both in
configuration and due to missing code.
Bug: None
Change-Id: Idea87dfe2be204361774d8964140fd9947a66410
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/261944
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36850}
When an outgoing stream reset is sent, with sequence number A, and the
receiver can't perform it immediately, it will return IN_PROGRESS with
response sequence number A.
This is then retried with sequence number A+1, and the peer would then
possibly respond PERFORMED with response sequence number A+1.
Before this CL, whenever a request was sent that didn't immediately
succeed, it wouldn't increment its expected response sequence number.
So in the retry above, the socket would still expect the response
sequence number to stay at A, not at A+1.
Bug: webrtc:13994
Change-Id: I6f36d45229a7fb312e97ad15826e0377f4efb64f
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/261310
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36797}
This reverts commit 3180a5ad06.
Reason for revert: Speculative revert due to failures in downstream tests.
Original change's description:
> dcsctp: Reset send queue when recreating TCB
>
> This is an issue found in fuzzer, and doesn't really happen in WebRTC
> as it never closes the connection and reconnects.
>
> The issue is that the send queue outlives any connection since you're
> allowed to send messages (well, enqueue them) before the association is
> fully connected. So the send queue is always present but the TCB
> (information about the connection) is torn down when the connection is
> closed for example. And the TCB holds the Stream Reset handler, which is
> responsible for e.g. keeping track of stream reset sequence numbers and
> such - which is tied to the connection.
>
> So to ensure that the Stream Reset Handler is in charge of deciding
> if a stream reset is taking place, make sure that the send queue is in
> a known good state when the Stream Reset handler is created.
>
> Bug: webrtc:13994, chromium:1320194
> Change-Id: I940e4690ac9237ac99dd69a9ffc060cdac61711d
> Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/261260
> Reviewed-by: Harald Alvestrand <hta@webrtc.org>
> Commit-Queue: Victor Boivie <boivie@webrtc.org>
> Cr-Commit-Position: refs/heads/main@{#36779}
Bug: webrtc:13994, chromium:1320194
Change-Id: I89bb9cae60adc53902c1304e79047d18e72594a5
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/261302
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Auto-Submit: Björn Terelius <terelius@webrtc.org>
Bot-Commit: rubber-stamper@appspot.gserviceaccount.com <rubber-stamper@appspot.gserviceaccount.com>
Owners-Override: Björn Terelius <terelius@webrtc.org>
Reviewed-by: Victor Boivie <boivie@google.com>
Commit-Queue: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36783}
This is an issue found in fuzzer, and doesn't really happen in WebRTC
as it never closes the connection and reconnects.
The issue is that the send queue outlives any connection since you're
allowed to send messages (well, enqueue them) before the association is
fully connected. So the send queue is always present but the TCB
(information about the connection) is torn down when the connection is
closed for example. And the TCB holds the Stream Reset handler, which is
responsible for e.g. keeping track of stream reset sequence numbers and
such - which is tied to the connection.
So to ensure that the Stream Reset Handler is in charge of deciding
if a stream reset is taking place, make sure that the send queue is in
a known good state when the Stream Reset handler is created.
Bug: webrtc:13994, chromium:1320194
Change-Id: I940e4690ac9237ac99dd69a9ffc060cdac61711d
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/261260
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36779}
When an SCTP stream is closing, a stream reset needs
to be sent from both ends.
The remote was not sending a stream reset and quickly
opening another stream with the same StreamID could
cause SCTP errors.
Bug: webrtc:13994
Change-Id: I3abc74ddc88b3fcf7e6495d76e7d77f52280b5d1
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/260922
Commit-Queue: Florent Castelli <orphis@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Reviewed-by: Mirko Bonadei <mbonadei@webrtc.org>
Reviewed-by: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36773}
When streams were to be reset, but there was already an ongoing
stream reset command in-flight, those streams wouldn't be properly
reset. When multiple streams were reset close to each other (within
an RTT), some streams would not have their SSNs reset, which resulted
in the stream resuming the SSN sequence. This could result in ordered
streams not delivering all messages as the receiver wouldn't deliver any
messages with SSN different from the expected SSN=0.
In WebRTC data channels, this would be triggered if multiple channels
were closed at roughly the same time, then re-opened, and continued
to be used in ordered mode. Unordered messages would still be delivered,
but the stream state could be wrong as the DATA_CHANNEL_ACK message is
sent ordered, and possibly not delivered.
There were unit tests for this, but not on the socket level using
real components, but just on the stream reset handler using mocks,
where this issue wasn't found. Also, those mocks didn't validate that
the correct parameters were provided, so that's fixed now.
The root cause was the PrepareResetStreams was only called if there
wasn't an ongoing stream reset operation in progress. One may try to
solve it by always calling PrepareResetStreams also when there is an
ongoing request, or to call it when the request has finished. One would
then realize that when the response of the outgoing stream request is
received, and CommitResetStreams is called, it would reset all paused
and (prepared) to-be-reset streams - not just the ones in the outgoing
stream request.
One cause of this was the lack of a single source of truth of the stream
states. The SendQueue kept track of which streams that were paused, but
the stream reset handler kept track of which streams that were
resetting. As that's error prone, this CL moves the source of truth
completely to the SendQueue and defining explicit stream pause states. A
stream can be in one of these possible states:
* Not paused. This is the default for an active stream.
* Pending to be paused. This is when it's about to be reset, but
there is a message that has been partly sent, with fragments
remaining to be sent before it can be paused.
* Paused, with no partly sent message. In this state, it's ready to
be reset.
* Resetting. A stream transitions into this state when it has been
paused and has been included in an outgoing stream reset request.
When this request has been responded to, the stream can really be
reset (SSN=0, MID=0).
This CL also improves logging, and adds socket tests to catch this
issue.
Bug: webrtc:13994, chromium:1320194
Change-Id: I883570d1f277bc01e52b1afad62d6be2aca930a2
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/261180
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36771}
Before this CL, fast retransmission didn't follow the SHOULDs:
https://datatracker.ietf.org/doc/html/rfc4960#section-7.2.4
* "the sender SHOULD ignore the value of cwnd (...)"
* "(...) and SHOULD NOT delay retransmission for this single
packet."
With this CL, chunks that are eligible for fast retransmission (limited
to what can fit in a single packet) will be sent just after having
received the SACK that reported them missing and transitioned the socket
into fast recovery, and they will be sent even if the congestion window
is full.
Bug: webrtc:13969
Change-Id: I12c7e191a8ffd67973db7f083bad8a6061549fa2
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/259866
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36724}
This CL makes OutstandingData keep track of chunks that are eligible for
fast retransmission. When the socket goes into fast recovery, the
reported missing chunks can be retransmitted quickly (ignoring the
congestion window) according to
https://datatracker.ietf.org/doc/html/rfc4960#section-7.2.4.
The CL also adds the new API to OutstandingData to retrieve only the
chunks that are eligible for fast retransmission, and moves the
remaining chunks to the ordinary list of chunks to be retransmitted
later.
This solves an issue where the retransmission timer wouldn't start if
there wouldn't be any chunks to fast-retransmit.
It doesn't, however, make sure that chunks that should be fast
retransmitted can send even when the congestion window is full. That
will be solved in the follow-up CL.
Bug: webrtc:13969
Change-Id: If4012d1cb284ef4a2d815683ed60cbbbff5b3c3b
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/259865
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36721}
This CL replaces two booleans, that could never be active at the same
time (there is no such thing as an abandoned chunk that is scheduled
for retransmission), with a single enum.
Just for increased readability, and to understand that there is no such
thing as an abandoned chunk that will be retransmitted.
Bug: None
Change-Id: I1682c383aed692db07fd4ae1f84c0166db86c062
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/259864
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36707}
This is mainly an issue when sending items with partial reliability,
with high bandwidth on a link with packet loss.
In SCTP, when a fragment isn't included in the SACK a number of times,
it's scheduled to be retransmitted or abandoned, if it has been
retransmitted too many times already (depending configuration). Before
this CL, if a fragment was NACKed and scheduled for retransmission, but
couldn't be retransmitted immediately (due to congestion window not
allowing it), future received SACKs - that would still indicate that the
fragment hasn't been received yet - would still increment the
retransmission counter. Which wasn't fair, because this fragment hasn't
had a chance to be retransmitted yet.
With this CL, the fragment will only have its retransmission counter
increased when it is not already scheduled to be retransmitted, but
actually sent on the wire and considered in-flight again.
Bug: webrtc:12943
Change-Id: I2af08255650221c044cc14591a5835c885e94c58
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/259825
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36683}
