The restart limit for timers can already be limitless, but the
RetransmissionErrorCounter didn't support this. With this change, the
max_retransmissions and max_init_retransmits can be absl::nullopt to
indicate that there should be infinite retries.
Bug: webrtc:13129
Change-Id: Ia6e91cccbc2e1bb77b3fdd7f37436290adc2f483
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/230701
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#34882}
dcSCTP seems to be able to provoke usrsctp to send ABORT in some
situations, as described in
https://github.com/sctplab/usrsctp/issues/597. Using a packetdrill
script, it seems as a contributing factor to this behavior is when a
FORWARD-TSN chunk is bundled with a DATA chunk. This is a valid and
recommended pattern in RFC3758:
"F2) The data sender SHOULD always attempt to bundle an outgoing
FORWARD TSN with outbound DATA chunks for efficiency."
However, that seems to be a rare event in usrsctp, which generally sends
each FORWARD-TSN in a separate packet.
By doing the same, the assumption is that this scenario will generally
be avoided.
With many browsers and other clients using usrsctp, and which will not
be upgraded for a long time, there is an advantage of avoiding the issue
even if it is according to specification.
Before this change, a FORWARD-TSN was bundled with outgoing DATA and due
to this, it wasn't rate limited as the overhead was very little. With
this change, a rate limiting behavior has been added to avoid sending
too many FORWARD-TSN in small packets. It will be sent every RTT, or
200 ms, whichever is smallest. This is also described in the RFC.
Bug: webrtc:12961
Change-Id: I3d8036a34f999f405958982534bfa0e99e330da3
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/229101
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34801}
The congestion window is unlikely to be even divisible by the size
of a packet, so when the congestion window is almost full, there is
often just a few bytes remaining in it. Before this change, a small
packet was created to fill the remaining bytes in the congestion window,
to make it really full.
Small packets don't add much. The cost of sending a small packet is
often the same as sending a large one, and you usually get lower
throughput sending many small packets compared to few larger ones.'
This mode will only be enabled when the congestion window is large, so
if the congestion window is small - e.g. due to poor network conditions,
it will allow packets to become fragmented into small parts, in order to
fully utilize the congestion window.
Bug: webrtc:12943
Change-Id: I8522459174bc72df569edd57f5cc4a494a4b93a8
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/228526
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34778}
For symmetry, as the outstanding_bytes is increased/decreased by
the serialized chunk size (not just the payload) - which is compared
to the congestion window, the congestion window should be increased
by the serialized size of chunks acked - not just their payload.
Bug: webrtc:12943
Change-Id: I0a06033e8ca0d58433138df6442ca80494918cf2
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/228525
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34775}
The same code was done for both acking chunks due to moving the
cum-ack-tsn and when acking gap-ack-blocks. Unify them completely
to have a single code path.
Bug: webrtc:12943
Change-Id: I3b864e41cc2ec674460517660c23b72a70edf720
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/228521
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34773}
This is mainly a refactoring commit, to break out packet sending to a
dedicated component.
Bug: webrtc:12943
Change-Id: I78f18933776518caf49737d3952bda97f19ef335
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/228565
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34772}
Before this change, there was no way for a client to indicate to the
dcSCTP library if a packet that was supposed to be sent, was actually
sent. It was assumed that it always was.
To handle temporary failures better, such as retrying to send packets
that failed to be sent when the send buffer was full, this information
is propagated to the library.
Note that this change only covers the API and adaptations to clients.
The actual implementation to make use of this information is done as a
follow-up change.
Bug: webrtc:12943
Change-Id: I8f9c62e17f1de1566fa6b0f13a57a3db9f4e7684
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/228563
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Niels Moller <nisse@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34767}
Some deployments, e.g. Chromium, has a limited send buffer. It's
reasonable that it's quite small, as it avoids queuing too much, which
typically results in increased latency for real-time communication. To
avoid SCTP to fill up the entire buffer at once - especially when doing
fast retransmissions - limit the amount of packets that are sent in one
go.
In a typical scenario, SCTP will not send more than three packets for
each incoming packet, which is is the case when a SACK is received which
has acknowledged two large packets, and which also adds the MTU to the
congestion window (due to in slow-start mode), which then may result in
sending three packets. So setting this value to four makes any
retransmission not use that much more of the send buffer.
This is analogous to usrsctp_sysctl_set_sctp_fr_max_burst_default in
usrsctp, which also has the default value of four (4).
Bug: webrtc:12943
Change-Id: Iff76a1668beadc8776fab10312ef9ee26f24e442
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/228480
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34744}
To support implementing RTCSctpTransportStats, a few metrics are needed.
Some more were added that are useful for metric collection in SFUs.
Bug: webrtc:13052
Change-Id: Idafd49e1084922d01d3e6c5860715f63aea08b7d
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/228243
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34708}
This corresponds to one part of sstat_unackdata in RFC6458. The
remaining part is the data in the send queue, which isn't packetized
yet, so it must be estimated. But the DATA items in the retransmission
queue is already determined, so it can be easily tracked and retrieved.
Bug: webrtc:13052
Change-Id: I16c3b5b61eb6b3022d7104e6457d943d5df3d6b9
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/228240
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34706}
As part of go/coil update code search links to not point to the
"master" branch.
Bug: chromium:1226942
Change-Id: I0ae9e84ecc660f789a69fe0b226f93bbc39a8a66
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/226081
Commit-Queue: Tony Herre <toprice@chromium.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34531}
The previous logic to abandon chunks when partial reliability was used
was a bit too eager and trigger happy.
* Chunks with limited retransmissions should only be abandoned when a
chunk is really considered lost. It should follow the same rules as
for retransmitting chunks - that it must be nacked three times or
due to a T3-RTX expiration. Before this change, a single SACK not
referencing it would be enough to abandon it. This resulted in a lot
of unnecessary sent FORWARD-TSN and undelivered messages - especially
if running with zero retransmissions.
The logic to expire chunks by limited retransmissions will now only
be applied when a chunk is actually nacked.
* The second partial reliability trigger - expiration time - wasn't
evaluated when producing a middle chunk of a larger message.
A number of test cases were added and updated as chunks will now be
abandoned immediately instead of first scheduled for retransmission and
later abandoned.
Bug: webrtc:12961
Change-Id: I0ae17b2672568bdbdc32073a99d4c24b09ff5fe9
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/225548
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34458}
CL partially auto-generated with:
git grep -l "\bassert(" | grep "\.[c|h]" | \
xargs sed -i 's/\bassert(/RTC_DCHECK(/g'
And with:
git grep -l "RTC_DCHECK(false)" | \
xargs sed -i 's/RTC_DCHECK(false)/RTC_NOTREACHED()/g'
With some manual changes to include "rtc_base/checks.h" where
needed.
A follow-up CL will remove assert() from Obj-C code as well
and remove the #include of <assert.h>.
The choice to replace with RTC_DCHECK is because assert()
is because RTC_DCHECK has similar behavior as assert()
based on NDEBUG.
This CL also contains manual changes to switch from
basic RTC_DCHECK to other (preferred) versions like
RTC_DCHECK_GT (and similar).
Bug: webrtc:6779
Change-Id: I00bed8886e03d685a2f42324e34aef2c9b7a63b0
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/224846
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Mirko Bonadei <mbonadei@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34442}
When a single stream is reset, and an outgoing SSN reset request is sent
and later acked by the peer sending a reconfiguration response with
status=Performed, the sender should unpause the paused stream and reset
the SSNs of that (ordered) stream. But only the single stream that was
paused, and not all streams. In this scenario, dcSCTP would - when the
peer acked the SSN reset request - reset the SSN of all streams.
This was found by orphis@webrtc.org using a data channel test
application. The peer, if it's a usrsctp client, will ABORT with
PROTOCOL_VIOLATION as it has already seen that SSN on that stream but
with a different TSN.
This bug was introduced when implementing the Round Robin scheduler in
https://webrtc-review.googlesource.com/c/src/+/219682. The FCFS
scheduler prior to this change was implemented correctly.
Bug: webrtc:12952
Change-Id: I3ea144a1df303145f69a5b03aada7f448c8c8163
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/225266
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34436}
This is for convenience to the users of dcSCTP, which may want to have
unit tests where the socket is mocked. And since it's best practice not
to mock other teams' or project's classes, a mock will be provided by
the upstream project - this one.
Bug: webrtc:12614
Change-Id: I65d5d21097e7feda9162567560d3838759c962fc
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/224161
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34385}
The factory allows us to isolate the implementation from users who only
need to depend directly on the public folder now.
Bug: webrtc:12614
Change-Id: Ied09cf772ed427eaf17a7b5705f587da57405640
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/220939
Commit-Queue: Florent Castelli <orphis@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34330}
While in the COOKIE ECHO state, there is a TCB and there might be data
in the send buffer, and RFC4960 allows the COOKIE ECHO chunk to bundle
additional DATA chunks in the same packet, but there mustn't be more
than one such packet sent, and that packet must have a COOKIE ECHO chunk
as the first chunk in it.
When the COOKIE ACK chunk has been received, the socket is allowed to
send multiple packets.
Previously, this was state managed by the socket and not the TCB, as
the socket is responsible for moving between the different states. And
when the COOKIE ECHO chunk was sent, the TCB was instructed to only send
a single packet by the socket.
However, if there were retransmissions or anything else that could
result in calling TransmissionControlBlock::SendBufferedChunks, it would
do as instructed and send those, even if the socket was in a state where
that wasn't allowed.
When the peer was dcSCTP, this didn't cause any issues as dcSCTP tries
to be tolerant in what it receives (but strict in what it sends, except
for when there are bugs). When the peer was usrsctp, it would send an
ABORT for each received packet that didn't have a COOKIE ECHO as the
first chunk, and then restart the handshake (sending an INIT). So this
resulted in a longer handshake, but the connection would eventually be
correctly established and any DATA chunks that resulted in the ABORTs
would've been retransmitted.
By making the TCB aware of that particular state, and to make it
responsible for creating the SCTP packet with the COOKIE ECHO chunk
first, and also to only send a single packet when it is in that state,
there will not be any way to bypass this limitation.
Also, while not explicitly mentioned in the RFC, the retransmission
timer will not affect resending any outstanding DATA chunks that were
bundled together with the COOKIE ECHO chunk, as then there would be two
timers that both would drive resending COOKIE ECHO and DATA chunks.
Bug: webrtc:12880
Change-Id: I76f215a03cceab5bafe9f16eb4775f3dc68a6f05
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/222645
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34329}
The previous approach was that the caller was responsible for ensuring
that any buffer passed in to the Bounded IO wrappers, and that any
offset from where sub-readers were created were valid. The called would
always do a validation of the data and return proper error messages
if they were not.
This didn't pan out. https://crbug.com/1216758 found an overflow that
fooled the validation logic and the fuzzer could read out-of-bounds,
although it would always crash in that particular case.
There was already bounds checking, but under DCHECKs. This CL changes
that so that any bounds checking is done with CHECKS, as would've been
done in Rust. It's better to crash than to read arbitrary memory.
Bug: chromium:1216758
Change-Id: I89b52f0758495b5fe46f926c142870a263b96314
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/221743
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34303}
This was found when fuzzing. If the specified number of parameter count
was larger than std::numeric_limits<size_t>::max()/2, the comparison
would overflow and read out-of-bounds. This would only apply to 32-bit
platforms and it would lead to a crash as it would access all of the
virtual memory range, and more.
Fixed: chromium:1216758
Change-Id: I2193d3ed078120b6c3e4645c0b16b9f230055e8d
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/221742
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34256}
If there is only little space left in a packet, and the remaining data
for a partially sent message is much larger, it will not generate a
small fragment for this message. This is to avoid fragmenting a message
into too many packets, as that increases the risk of losing messages
when partial reliability is enabled.
And when a stream doesn't want to generate a too small fragment, the
scheduler should _not_ switch streams. It should only switch streams
when a message has been fully sent. Previously, it would switch stream
when a stream doesn't want to produce a message, but as noted above,
that could happen for other reasons.
This required some refactoring, which also increased its robustness by
now only doing explicit stream switching on fully produced messages.
Bug: webrtc:12832
Change-Id: Icb213774fd0d26fba5640b00aac0407d393e4bfc
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/220937
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34197}
The way that the "next stream" was picked when round-robin cycling was
flawed. When a message was produced in its entirety, the "next stream"
would be put at a stream identifier value that was just larger than what
was previously used. And then, for each fragment that was to be created,
it would try to resolve the nearest stream (above or equal to that
number) that had messages to send - always starting from that stream id
that didn't necessarily point to the stream for which fragments were
actually produced.
For example, if the previous stream ID for which a message was fully
produced on was 5, then the next_stream_id would be set to 6, and then
when producing next fragment, it might have produced something from
stream_id=1, because that was the only stream with messages in it. It
wouldn't update next_stream_id at this time; it would still be 6.
After a single fragment had been produced from that stream, a message
was queued on stream_id=6. The next time a fragment was to be produced,
it would not continue one stream_id=1, but instead pick the new stream,
which would suddenly produce a new fragment (with B flag set) while the
previous message (from stream_id=1) wasn't finished yet.
The fix is simple; Just ensure that we continue iterating from where we
ever produce a fragment from.
Bug: webrtc:12832
Change-Id: Icc761c572ed200db607a7609dab1ac6a8aeb2f04
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/220938
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34190}
Before this CL, before sending out any chunk, all inflight data chunks
were inspected to find out if they were supposed to be retransmitted.
When the congestion window is large, this is a lot of data chunks to
inspect, which takes time.
By having a separate collection for chunks to be retransmitted, this
becomes a much faster operation. In most cases, constant in time.
Bug: webrtc:12799
Change-Id: I0d43ba7a88656eead26d5e0b9c4735622a8d080e
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/219626
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34178}
There is no need to iterate through all outstanding data chunks to know
if a FORWARD-TSN can be sent. As the FORWARD-TSN will just move the
cumulative TSN ack, if a chunk is found that is not to be expired,
there is no need to continue any further. This makes it much faster
to know if to send a FORWARD-TSN when the congestion window is large.
Bug: webrtc:12799
Change-Id: I58bce408ae9814c8d3d7bbb480b0037a2cf88dd7
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/219625
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34176}
Before this CL, a SACK was generated from scratch based on information
about each received fragment, to generate correct gap-ack-blocks.
When there was a lot of data in the data tracker (due to packet loss),
this took considerate time, as generating a SACK was O(N), where N is
the amount of fragments in the data tracker.
By instead having precomputed gap-ack-blocks that are continuously
updated, generating a SACK is much faster and the memory usage goes down
a bit as well.
Bug: webrtc:12799
Change-Id: I924752c1d6d31f06d27246e10b595e9ccb19320f
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/220763
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34171}
There limit that decides if an incoming TSN should be accepted or not
was decided based on very small transfers with no packet loss. But in
simulations where a socket tries to send a lot of data and when there
is moderate packet loss, the number of tracker data chunks on the
receive side will be considerably higher than what the limit was.
Set the limit to allow high data rate also on moderate packet loss.
Bug: webrtc:12799
Change-Id: I6ca237e5609d8b511e9b10c919da33dca7420c01
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/220761
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34169}
The receive buffer mustn't be full; If it's full, and a message can't be
assembled, the socket can't accept more data. To avoid this, there is
a high watermark limit that, when reached, will make the socket only
accept chunks that advance the cumulative ack TSN.
Before this CL, the announced receiver window size in every sent SACK
was based on what the receive buffer could maximally be, which means
that in really high data rate applications, the amount of outstanding
data could actually fill the receive buffer (due to packet loss, that
prevents messages from being reassembled). As the socket started
behaving more conservatively when the high watermark limit was reached,
this resulted in unnecessary T3-RTXes. But by announcing the high
watermark limit instead, the sender will stay within it, and will have
a peer socket that behaves as expected.
Bug: webrtc:12799
Change-Id: Ife2f409914a230640217553c54f60d05843efc70
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/220762
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34168}
There were some missing unit tests that are now written. When doing
this, it was found that SACKs weren't sent for duplicate received
chunks, which they should be according to the spec.
Bug: webrtc:12614
Change-Id: I8296473c0c8cbaf0329785de95e9b9945f254339
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/220607
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34165}
This is useful in tests and in scenarios where the connection is
monitored externally and the heartbeat monitoring would be of no use.
Bug: webrtc:12614
Change-Id: Ida4f4e2e40fc4d2aa0c27ae9431f434da4cc8313
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/220766
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34164}
This is similar to Change-Id: I12a16f44f775da3711f3aa52a68a0bf24f70d2f8
but with the entire send buffer as scope, not a single stream.
This can be used by clients to take alternate action (such as delaying
transmission or using other buffering) if the send buffer ever becomes
full, as they can now be notified when the send buffer is no longer
full.
Bug: webrtc:12794
Change-Id: Icf3be3b118888ffb5ced955fd7ba4826a37140f9
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/220360
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34143}
This adds the necessary properties and callback to the Send Queue to
support the bufferedAmount & bufferedAmountLowThreshold properties and
the bufferedamountlow event in RTCDataChannel.
The public API changes and socket support comes in a follow-up CL.
Bug: webrtc:12794
Change-Id: I12a16f44f775da3711f3aa52a68a0bf24f70d2f8
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/219690
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34142}
If a not fully sent message is abandoned, there must be a TSN
representing the end of that message (even if that fragment is never
sent), as the receiver can otherwise reject the next sent message as it
hasn't seen any end of the previous one.
A long explanation can be found at
https://github.com/sctplab/usrsctp/issues/592#issuecomment-849047689
Bug: webrtc:12812
Change-Id: I09c571bd6dd2774b0c147d4e5ddac67d2aa64fea
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/220361
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34140}
Recalculating outstanding bytes is expensive when the congestion window
is large, as it iterates over all inflight data chunks. By doing it
incrementally, it will be a constant operation in most cases, and
in the remaining cases, a function of the number of chunks acked in a
single SACK, which is typically just a few chunks.
Implementing this fix required some refactoring to calculate it
correctly (and to be honest, it was likely done incorrectly previously).
Previously, the state of an item in the retransmission queue was
simplified as "in flight", "acked", "nacked", "abandoned", but these
were not completely orthogonal. A chunk could be abandoned while it was
in-flight or it could be abandoned because it was lost. The difference
between these if that chunk should be accounted for in
outstanding_bytes() or not.
Unit tests have been added to verify this.
Bug: webrtc:12799
Change-Id: I72341538bb0c4f8f89555b08f0c8a28815f0f828
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/219623
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34139}
Today, there is no actual limit on how large a SACK chunk can be. And
having limits is good to be able to stay within the MTU.
This commit adds a limit to the number of reported duplicate TSNs as
well as the number of reported gap-ack-blocks in a SACK chunk. These
limits are never expected to be reached in a real-life situation.
Bug: webrtc:12614
Change-Id: Ib2c143714a214cd3d961e8a52dac26a04b909b80
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/219464
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34108}
The current send queue implements SCTP_SS_FCFS as defined in
https://datatracker.ietf.org/doc/html/rfc8260#section-3.1, but that has
always been known to be a temporary solution. The end goal is to
implement a Weighted Fair Queueing Scheduler (SCTP_SS_WFQ), but that's
likely to take some time.
Meanwhile, a round robin scheduler (SCTP_SS_RR) will be used to avoid
some issues with the current scheduler, such as a single data channel
completely blocking all others if it sends a lot of messages.
In this first commit, the code has simply been renamed and is still
implementing first-come-first-served. That will be fixed in follow-up
CLS.
Bug: webrtc:12793
Change-Id: Idc03b1594551bfe1ddbe1710872814b9fdf60cc9
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/219684
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34090}
The retransmission timeout (RTO) value is updated on every measured
RTT and is a function of the RTT value and its stability. In reality,
the RTT is never constant - it fluctuates, which makes the RTO become
much larger than the RTT. But for extremely stable RTTs, which we get
in simulations, the RTO value can become the same as the RTT, and that
makes expiration timers be scheduled to the RTT value, and will race
with packets that are expected to stop the expiration timer. And that
race should be avoided in simulations.
So ensuring that the RTO value is always greater, if only be a single
millisecond, will work fine in these simulations.
Bug: webrtc:12614
Change-Id: I30cf9c97e50449849ab35de52696c618d8498128
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/219680
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34084}
Reporting the duplicate TSNs is a SHOULD in the RFC, and using the
duplicate TNSs is a MAY, and in reality I haven't seen an implementation
use it yet. However, it's good for debugging and for stats generation.
Bug: webrtc:12614
Change-Id: I1cc3f86961a8d289708cbf50d98dedfd25077955
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/219462
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34053}
The minimum RTO time shouldn't be lower than the delayed ack timeout
of the peer to avoid sending retransmissions before the peer has
actually intended to reply.
In usrsctp, the default delayed ack timeout is 200ms and configurable
using the `sctp_delayed_sack_time_default` option. In dcsctp, it's
min(RTO/2, 200ms), to avoid this issue.
Bug: webrtc:12614
Change-Id: Ie84c331334af660d66b1a7d90d20f5cf7e2a5103
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/219100
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34026}
The socket fuzzer is build as a structure-aware fuzzer where the full
public API is exercised as well as receival of SCTP packets with random
sequences of valid chunks.
It begins by putting the socket in a defined starting state and then,
based on the fuzzing data, performs a sequence of operations on the
socket such as receiving packets, sending data, resetting streams or
expiring timers.
This is the first iteration, and when running it a while and analyzing
code coverage, it will be modified to perform better. It could probably
be a little more random.
Bug: webrtc:12614
Change-Id: I50d6ffaecef5722be5cf666fee2f0de7d15cc2e8
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/218500
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#33998}
When a socket is shutting down, either explicitly by the ULP calling
Shutdown(), or when the socket receives a SHUTDOWN chunk, the socket
should send all outstanding data and when all is sent and acked,
_then_ it should continue the shutdown protocol.
As it currently doesn't calculate correctly when all data has been sent,
as NACKED chunks are not included in what it believes is remaining in
the retransmission queue, it will shut down prematurely and may go back
to a previous state (ShutdownPending) from ShutdownSent or
ShutdownAckSent.
This is a workaround that just avoids the illegal state transition as
that puts the socket in an inconsistent state. The bug is merely
theoretical as WebRTC doesn't currently gracefully shut down a SCTP
socket, but just terminates the DTLS transport.
As TODOs mention, this will be fixed correctly a bit later.
Bug: webrtc:12739
Change-Id: Ibde2acc3a6aca701ac178d6181028404d470a5d5
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/218340
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#33982}
While it's not strictly defined, the expectation is that sending a
message with a lifetime parameter set to zero (0) ms should allow it to
be sent if it can be sent without being buffered. If it can't be
directly sent, it should be discarded.
This is initial support for it. Small messages can now be delivered fine
if they are not to be buffered, but fragmented messages could be partly
sent (if this fills up the congestion window), which means that the
message will then fail to be sent whenever the congestion window frees
up again. It would be better to - at a higher level - realize early that
the message can't be sent in full, and discard it without sending
anything. But that's an optimization that can be done later.
A few off-by-one errors were found when strictly defining that the
message is alive during its entire lifetime. It will expire just _after_
its lifetime.
Sending messages with a lifetime of zero may not supported in all
libraries, so a workaround would be to set a very small timeout instead,
which is tested as well.
Bug: webrtc:12614
Change-Id: I9a00bedb639ad7b3b565b750ef2a49c9020745f1
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/217562
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#33977}
