As WebRTC now supports C++17, simplify the code of dcSCTP by binding
return values from std::pair or std::tuple to separate names.
Bug: webrtc:13220
Change-Id: Ie49154ff4c823e1528deaef7e372cbc550923bc2
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/246442
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35773}
the new spelling is more standard and more compact, in particular doesn't need extra include and thus dependency
Bug: None
Change-Id: Iaea69d2154e4d9eff2468514f5734cb3fe016ff8
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/245080
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35709}
This avoids copying the payload at all. Future CL will change the
transport.
In performance tests, memcpy was visible in the performance profiles
prior to this change.
Bug: webrtc:12943
Change-Id: I507a1a316165db748e73cf0d58c1be62cc76a2d2
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/236346
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35428}
Add implementation of RTC_DCHECK_NOTREACHED equal to the RTC_NOTREACHED.
The new macros will replace the old one when old one's usage will be
removed. The idea of the renaming to provide a clear signal that this
is debug build only macros and will be stripped in the production build.
Bug: webrtc:9065
Change-Id: I4c35d8b03e74a4b3fd1ae75dba2f9c05643101db
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/237802
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Artem Titov <titovartem@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35348}
It's put in the public folder since the intention is to expose it in
SendOptions.
Additionally, use TimeMs::InfiniteFuture() to represent sending a
message with no limited lifetime (i.e. to send it reliably).
One benefit for these two is avoiding using absl::optional more than
necessary, as it results in larger struct sizes for the outstanding
data chunks.
Bug: webrtc:12943
Change-Id: I87a340f0e0905342878fe9d2a74869bfcd6b0076
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/235984
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35323}
RetransmissionQueue was growing too long (almost 1000 lines), and as
there is reason to believe that more changes are necessary in it for
performance reasons, the data structure that handles managing the
in-flight outstanding data has been extracted as a separate class with
its own test cases. What remains in RetransmissionQueue is that it holds
OutstandingData, fetch data from the SendQueue and manage all congestion
control variables and algorithms.
Bug: webrtc:12943
Change-Id: I46062a774e0e76b44e36c66f836b7d992508bf5f
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/235980
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35279}
Networks tests were previously disabled if building in debug mode as
debug mode adds DCHECKs, and when DCHECKs are enabled, a lot of the
components in dcSCTP will add consistency checks, and they can be really
expensive to run in these network tests.
However, if running in with TSAN or MSAN sanitizers and with DCHECKs
enabled, they also take a long time.
Current run-time on my relatively fast CPU (with is_debug=false):
(no sanitizer) always_dcheck=false: 2.5s
(no sanitizer) always_dcheck=true: 31s
is_tsan=true, always_dcheck=false: 53s
is_tsan=true, always_dcheck=true: 5m50s <-- too slow
is_asan=true, always_dcheck=false: 13s
is_asan=true, always_dcheck=true: 47s
is_msan=true, always_dcheck=false: 35s
is_msan=true, always_dcheck=true: 1m53s <-- too slow
Note that buildbots may be much slower than my computer.
Bug: webrtc:12943
Change-Id: If044ee9936372d54c9899b4864156c9f680af0b6
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/236581
Reviewed-by: Mirko Bonadei <mbonadei@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35276}
A vector of which TSNs that were acked for each received SACK was
created, but only used in debug logs, which aren't enabled by default.
Removing them, as they don't add that much value and cost a bit
of performance.
Bug: webrtc:12943
Change-Id: Ice323cf46ca6e469fbbcf2a268ad67ca883bb2f5
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/235985
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35265}
This is explained in RFC 4960, section 6.1, A.
... However, regardless of the value of rwnd (including if it
is 0), the data sender can always have one DATA chunk in flight to
the receiver if allowed by cwnd ... This rule
allows the sender to probe for a change in rwnd that the sender
missed due to the SACK's having been lost in transit from the data
receiver to the data sender.
Before this change, when a receiver has advertised a zero receiver
window size (a_rwnd=0) and a subsequent SACK advertising a non-zero
receiver window was lost, the sender was blocked from sending and since
SACKs are only sent when a DATA chunk is received, it would be
deadlocked. The retransmission timer would fire, but nothing would be
retransmitted (as it respected the zero receiver window).
With this change, when the retransmission timer fires (after RTO), it
would send a single packet with DATA chunk(s) and then SACKs would
eventually be received, with the non-zero receiver window and the socket
would recover.
Bug: chromium:1258225
Change-Id: I1ea62fb3c002150eeada28d3e703dbc09cfd038e
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/235280
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35215}
This is a follow-up to change 232904 that also validates that the
timestamp from the heartbeat ack isn't negative (which the fuzzer
managed to set it to).
Bug: chromium:1252515
Change-Id: Idaac570589dbdaaee67b7785f6232b60226e88e1
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/234582
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35168}
The DcSctpSocket is not thread safe and must be called from a single
thread or from a task queue that serializes access to it. This is now
validated at run-time in debug builds.
Bug: None
Change-Id: I3ed816924c20f6ed7e84a3273bee5a3f8f74112b
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/233420
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35127}
This initial version contains a few tests, testing both lossy, non-lossy
and bandwidth limited networks. It uses simulated time, and runs much
faster than wall time on release builds, but slower on debug when there
is a lot of outstanding data (high throughput) as there are consistency
checks on outstanding data. Because of that, some tests had to be
disabled in debug build.
Bug: webrtc:12943
Change-Id: I9323f2dc99bca4e40558d05a283e99ce7dded7f1
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/225201
Reviewed-by: Artem Titov <titovartem@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35124}
The previous manual way of triggering the deferred callbacks was very
error-prone, and this was also forgotten at a few places.
We can do better.
Using the RAII programming idiom, the callbacks are now ensured to be
called before returning from public methods.
Also added additional debug checks to ensure that there is a
ScopedDeferrer active whenever callbacks are deferred.
Bug: webrtc:13217
Change-Id: I16a8343b52c00fb30acb018d3846acd0a64318e0
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/233242
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35117}
It's to be used for clients to record metrics and to e.g. attribute
metrics to which SCTP implementation the peer was using.
This is not explicitly signaled, so heuristics are used. These are not
guaranteed to come to the correct conclusion, and the data is not always
available.
Note: The behavior of dcSCTP will not change depending on the assumed
implementation - only by explicitly signaled capabilities.
Bug: webrtc:13216
Change-Id: I2f58054d17d53d947ed5845df7a08f974d42f918
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/233100
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35103}
When a HEARTBEAT is sent out, the current timestamp is stored in the
parameter that will be returned by the HEARTBEAT-ACK. If the timestamp
from the HEARTBEAT-ACK would be from the future (either by jumping
clocks, bit errors, exploiting peer or a fuzzer), the measured RTT would
become really large, and when it was calculated, it would result in an
integer overflow; a wrapping subtraction.
This isn't a problem, as RetransmissionTimeout::ObserveRTT method would
discard measurements that were negative or too large, but it would
trigger an UBSAN violation.
Adding an additional check so that the subtraction doesn't ever wrap.
Bug: chromium:1252515
Change-Id: I1f97b1e773a4639b8193a528716ebd6a27fcb740
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/232904
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35095}
This is mentioned in
https://datatracker.ietf.org/doc/html/rfc4960#section-6.3.1 and further
described in https://datatracker.ietf.org/doc/html/rfc6298#section-4.
The TCP RFCs mentioned G as the clock granularity, but in SCTP it should
be set much higher, to account for the delayed ack timeout (ATO) of the
peer (as that can be seen as a very high clock granularity). That one is
set to 200ms by default in many clients, so a reasonable default limit
could be set to 220 ms.
If the measured variance is higher, it will be used instead.
Bug: webrtc:12943
Change-Id: Ifc217daa390850520da8b3beb0ef214181ff8c4e
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/232614
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35068}
This was caused by change 231229 and was later caught when reviewing the
code. The rtt variable was accidentally re-used for another purpose, and
then assumed to still be used to represent the rtt.
There have been no issues found with this re-use, but it was wrong.
Bug: webrtc:12614
Change-Id: If1a180315cf833e293f78c54c3c3b29394a19a20
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/232610
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35064}
It's useful for other parts of WebRTC and there is no real reason why
it should be located in net/dcsctp.
Bug: None
Change-Id: Iccaed4e943e21ddaea8603182d693114b2da9f6b
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/232606
Reviewed-by: Mirko Bonadei <mbonadei@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35055}
This allows build targets that need only HandoverReadinessStatus
to depend only on public:socket, and not on socket:dcsctp_socket.
Bug: webrtc:13154
Change-Id: I29f41910cdb5baed96b57fd7284b96fc50a56ba4
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/232331
Reviewed-by: Victor Boivie <boivie@webrtc.org>
Commit-Queue: Sergey Sukhanov <sergeysu@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35037}
dcSCTP library users can set their custom
g_handover_state_transformer_for_test that can serialize and
deserialize the state. All dcSCTP handover tests call
g_handover_state_transformer_for_test. If some part of the state is
serialized incorrectly or is forgotten, high chance that it will
fail the tests.
Bug: webrtc:13154
Change-Id: I251a099be04dda7611e9df868d36e3a76dc7d1e1
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/232325
Commit-Queue: Sergey Sukhanov <sergeysu@webrtc.org>
Reviewed-by: Mirko Bonadei <mbonadei@webrtc.org>
Reviewed-by: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35035}
When there is no outstanding data, then next TSN to allocate should
always be one more than what the client has last ACKed.
Bug: None
Change-Id: Ieb8b5b23912d77d96fe3749fb53fd53652d97066
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/232002
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35016}
The buffer of reassembled messages in ReassemblyQueue is only to be
used while processing a DATA/I-DATA or FORWARD-TSN as processing these
chunks may result in assembling messages.
When the socket is idle - between API calls - it's supposed to be empty.
Instead of having it as a member in ReassemblyQueue, it could be
provided as an argument to ReassemblyQueue::Add and
ReassemblyQueue::Handle(ForwardTSN), but that would be a quite big
refactoring. That will be investigated separately.
Bug: None
Change-Id: I41238de28f32f2a622c1d045debe3ea11e7c23f6
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/232000
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35014}
The ReassemblyQueue will need to track which messages that have already
been delivered to the client so that they are not re-delivered on e.g.
retransmissions. It does that by tracking which TSNs that those messages
were built from. It tracks that in two variables,
`last_assembled_tsn_watermark` and `delivered_tsns_`, where the first
one represent that all TSNs including and prior this one have been
delivered and `delivered_tsns` contain additional ones when there are
gaps.
When receiving a FORWARD-TSN and asked to forget about some partially
received messages, these two variables were updated correctly, but the
`delivered_tsns_` were left in a state where it could be adjacent to the
`last_assembled_tsn_watermark` - when `last_assembled_tsn_watermark`
could actually have been moved further.
Added consistency check (that would trigger in existing tests) and
fixing the issue.
This bug is quite benign, as any received chunk would've corrected the
problem, and even at this faulty state, the ReassemblyQueue would
function completely fine.
Bug: webrtc:13154
Change-Id: Iaa7c612999c9dc609fc6e2fb3be2d0bd04534c90
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/232124
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Reviewed-by: Sergey Sukhanov <sergeysu@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35013}
Following Congestion avoidance and control by V. Jacobson at
https://dl.acm.org/doi/10.1145/52324.52356, use integer math instead
of floating point. Not that it matters, but it results in some code size
savings, and is more efficient. Due to not using floating point math,
some golden values in test cases were rounded a bit differently.
Bug: webrtc:12614
Change-Id: I0b7d54b8fd9ce7156e6b2582437ef5720f8838ea
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/231229
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#34956}
The previous limits were taken from Oracles SCTP stack[1] as they were
more up-to-date than the suggested ones in RFC4960. However, after
having evaluated them for a while, it's evident that they are a bit too
aggressive and likely have their origin from a wired LAN network.
Let's do a re-take. These values have been taken from Solaris TCP
stack[2]. They are even less aggressive than Linux defaults. This can be
iterated even more, and is always possible to override by the client.
It's generally the increase of rto_min that is helping here, as the
delayed SACK and RTT jitter require that the RTO.min is quite much
higher than the delayed SACK timeout of the peer (which isn't in control
by us, but one can assume it's 200ms or less). And with a too low
RTO.min, it's increased risk of getting spurious retransmissions and
decreasing the congestion window.
[1] https://docs.oracle.com/cd/E93309_01/docs.466/SIGTRAN/GUID-2136614F-4BED-407C-87B0-7EE10E0FF534.htm
[2] https://docs.oracle.com/cd/E19120-01/open.solaris/819-2724/chapter4-69/index.html
Bug: webrtc:12943
Change-Id: I9678ac4396286a55c251c5f57589379da70fd27d
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/231139
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#34927}
By allowing the max timer backoff duration to be limited, a socket can
fast recover in case of intermittent network issues. Before this CL, the
exponential backoff algorithm could result in very long retry durations
(in the order of minutes), when connection has been lost or been flaky
for a long while.
Note that limiting the maximum backoff duration might require
compensating the maximum retransmission limit to avoid closing the
socket prematurely due to reaching the maximum retransmission limit much
faster than previously.
Bug: webrtc:13129
Change-Id: Ib94030d666433e3fa1a2c8ef69750a1afab8ef94
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/230702
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#34913}
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}
