dcsctp: Support unlimited max_retransmissions

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}

net/dcsctp/public/BUILD.gn

@@ -22,6 +22,7 @@ rtc_source_set("types") {
     "dcsctp_options.h",
     "types.h",
   ]
+  absl_deps = [ "//third_party/abseil-cpp/absl/types:optional" ]
 }
 
 rtc_source_set("socket") {

net/dcsctp/public/dcsctp_options.h

@@ -13,6 +13,7 @@
 #include <stddef.h>
 #include <stdint.h>
 
+#include "absl/types/optional.h"
 #include "net/dcsctp/public/types.h"
 
 namespace dcsctp {
@@ -150,11 +151,13 @@ struct DcSctpOptions {
   // retransmission scenarios.
   int max_burst = 4;
 
-  // Maximum Data Retransmit Attempts (per DATA chunk).
+  // Maximum Data Retransmit Attempts (per DATA chunk). Set to absl::nullopt for
-  int max_retransmissions = 10;
+  // no limit.
+  absl::optional<int> max_retransmissions = 10;
 
-  // Max.Init.Retransmits (https://tools.ietf.org/html/rfc4960#section-15)
+  // Max.Init.Retransmits (https://tools.ietf.org/html/rfc4960#section-15). Set
-  int max_init_retransmits = 8;
+  // to absl::nullopt for no limit.
+  absl::optional<int> max_init_retransmits = 8;
 
   // RFC3758 Partial Reliability Extension
   bool enable_partial_reliability = true;

net/dcsctp/socket/dcsctp_socket.cc

@@ -775,7 +775,7 @@ bool DcSctpSocket::HandleUnrecognizedChunk(
 absl::optional<DurationMs> DcSctpSocket::OnInitTimerExpiry() {
   RTC_DLOG(LS_VERBOSE) << log_prefix() << "Timer " << t1_init_->name()
                        << " has expired: " << t1_init_->expiration_count()
-                       << "/" << t1_init_->options().max_restarts;
+                       << "/" << t1_init_->options().max_restarts.value_or(-1);
   RTC_DCHECK(state_ == State::kCookieWait);
 
   if (t1_init_->is_running()) {
@@ -796,7 +796,8 @@ absl::optional<DurationMs> DcSctpSocket::OnCookieTimerExpiry() {
   // user."
   RTC_DLOG(LS_VERBOSE) << log_prefix() << "Timer " << t1_cookie_->name()
                        << " has expired: " << t1_cookie_->expiration_count()
-                       << "/" << t1_cookie_->options().max_restarts;
+                       << "/"
+                       << t1_cookie_->options().max_restarts.value_or(-1);
 
   RTC_DCHECK(state_ == State::kCookieEchoed);
 
@@ -813,7 +814,8 @@ absl::optional<DurationMs> DcSctpSocket::OnCookieTimerExpiry() {
 absl::optional<DurationMs> DcSctpSocket::OnShutdownTimerExpiry() {
   RTC_DLOG(LS_VERBOSE) << log_prefix() << "Timer " << t2_shutdown_->name()
                        << " has expired: " << t2_shutdown_->expiration_count()
-                       << "/" << t2_shutdown_->options().max_restarts;
+                       << "/"
+                       << t2_shutdown_->options().max_restarts.value_or(-1);
 
   if (!t2_shutdown_->is_running()) {
     // https://tools.ietf.org/html/rfc4960#section-9.2

net/dcsctp/socket/dcsctp_socket_test.cc

@@ -487,7 +487,7 @@ TEST_F(DcSctpSocketTest, ResendingInitTooManyTimesAborts) {
                               SctpPacket::Parse(cb_a_.ConsumeSentPacket()));
   EXPECT_EQ(init_packet.descriptors()[0].type, InitChunk::kType);
 
-  for (int i = 0; i < options_.max_init_retransmits; ++i) {
+  for (int i = 0; i < *options_.max_init_retransmits; ++i) {
     AdvanceTime(options_.t1_init_timeout * (1 << i));
     RunTimers();
 
@@ -498,7 +498,7 @@ TEST_F(DcSctpSocketTest, ResendingInitTooManyTimesAborts) {
   }
 
   // Another timeout, after the max init retransmits.
-  AdvanceTime(options_.t1_init_timeout * (1 << options_.max_init_retransmits));
+  AdvanceTime(options_.t1_init_timeout * (1 << *options_.max_init_retransmits));
   EXPECT_CALL(cb_a_, OnAborted).Times(1);
   RunTimers();
 
@@ -543,7 +543,7 @@ TEST_F(DcSctpSocketTest, ResendingCookieEchoTooManyTimesAborts) {
                               SctpPacket::Parse(cb_a_.ConsumeSentPacket()));
   EXPECT_EQ(init_packet.descriptors()[0].type, CookieEchoChunk::kType);
 
-  for (int i = 0; i < options_.max_init_retransmits; ++i) {
+  for (int i = 0; i < *options_.max_init_retransmits; ++i) {
     AdvanceTime(options_.t1_cookie_timeout * (1 << i));
     RunTimers();
 
@@ -555,7 +555,7 @@ TEST_F(DcSctpSocketTest, ResendingCookieEchoTooManyTimesAborts) {
 
   // Another timeout, after the max init retransmits.
   AdvanceTime(options_.t1_cookie_timeout *
-              (1 << options_.max_init_retransmits));
+              (1 << *options_.max_init_retransmits));
   EXPECT_CALL(cb_a_, OnAborted).Times(1);
   RunTimers();
 
@@ -647,7 +647,7 @@ TEST_F(DcSctpSocketTest, ShutdownTimerExpiresTooManyTimeClosesConnection) {
 
   EXPECT_EQ(sock_a_.state(), SocketState::kShuttingDown);
 
-  for (int i = 0; i < options_.max_retransmissions; ++i) {
+  for (int i = 0; i < *options_.max_retransmissions; ++i) {
     AdvanceTime(DurationMs(options_.rto_initial * (1 << i)));
     RunTimers();
 
@@ -658,7 +658,7 @@ TEST_F(DcSctpSocketTest, ShutdownTimerExpiresTooManyTimeClosesConnection) {
     EXPECT_TRUE(cb_a_.ConsumeSentPacket().empty());
   }
   // The last expiry, makes it abort the connection.
-  AdvanceTime(options_.rto_initial * (1 << options_.max_retransmissions));
+  AdvanceTime(options_.rto_initial * (1 << *options_.max_retransmissions));
   EXPECT_CALL(cb_a_, OnAborted).Times(1);
   RunTimers();
 
@@ -795,7 +795,7 @@ TEST_F(DcSctpSocketTest, CloseConnectionAfterTooManyLostHeartbeats) {
 
   DurationMs time_to_next_hearbeat = options_.heartbeat_interval;
 
-  for (int i = 0; i < options_.max_retransmissions; ++i) {
+  for (int i = 0; i < *options_.max_retransmissions; ++i) {
     RTC_LOG(LS_INFO) << "Letting HEARTBEAT interval timer expire - sending...";
     AdvanceTime(time_to_next_hearbeat);
     RunTimers();
@@ -834,7 +834,7 @@ TEST_F(DcSctpSocketTest, RecoversAfterASuccessfulAck) {
 
   DurationMs time_to_next_hearbeat = options_.heartbeat_interval;
 
-  for (int i = 0; i < options_.max_retransmissions; ++i) {
+  for (int i = 0; i < *options_.max_retransmissions; ++i) {
     AdvanceTime(time_to_next_hearbeat);
     RunTimers();
 

net/dcsctp/timer/timer.cc

@@ -93,8 +93,8 @@ void Timer::Trigger(TimerGeneration generation) {
   if (is_running_ && generation == generation_) {
     ++expiration_count_;
     is_running_ = false;
-    if (options_.max_restarts < 0 ||
+    if (!options_.max_restarts.has_value() ||
-        expiration_count_ <= options_.max_restarts) {
+        expiration_count_ <= *options_.max_restarts) {
       // The timer should still be running after this triggers. Start a new
       // timer. Note that it might be very quickly restarted again, if the
       // `on_expired_` callback returns a new duration.

net/dcsctp/timer/timer.h

@@ -42,10 +42,10 @@ struct TimerOptions {
   explicit TimerOptions(DurationMs duration)
       : TimerOptions(duration, TimerBackoffAlgorithm::kExponential) {}
   TimerOptions(DurationMs duration, TimerBackoffAlgorithm backoff_algorithm)
-      : TimerOptions(duration, backoff_algorithm, -1) {}
+      : TimerOptions(duration, backoff_algorithm, absl::nullopt) {}
   TimerOptions(DurationMs duration,
                TimerBackoffAlgorithm backoff_algorithm,
-               int max_restarts)
+               absl::optional<int> max_restarts)
       : duration(duration),
         backoff_algorithm(backoff_algorithm),
         max_restarts(max_restarts) {}
@@ -55,8 +55,9 @@ struct TimerOptions {
   // If the duration should be increased (using exponential backoff) when it is
   // restarted. If not set, the same duration will be used.
   const TimerBackoffAlgorithm backoff_algorithm;
-  // The maximum number of times that the timer will be automatically restarted.
+  // The maximum number of times that the timer will be automatically restarted,
-  const int max_restarts;
+  // or absl::nullopt if there is no limit.
+  const absl::optional<int> max_restarts;
 };
 
 // A high-level timer (in contrast to the low-level `Timeout` class).

net/dcsctp/tx/retransmission_error_counter.cc

@@ -15,14 +15,14 @@
 namespace dcsctp {
 bool RetransmissionErrorCounter::Increment(absl::string_view reason) {
   ++counter_;
-  if (counter_ > limit_) {
+  if (limit_.has_value() && counter_ > limit_.value()) {
     RTC_DLOG(LS_INFO) << log_prefix_ << reason
                       << ", too many retransmissions, counter=" << counter_;
     return false;
   }
 
   RTC_DLOG(LS_VERBOSE) << log_prefix_ << reason << ", new counter=" << counter_
-                       << ", max=" << limit_;
+                       << ", max=" << limit_.value_or(-1);
   return true;
 }
 

net/dcsctp/tx/retransmission_error_counter.h

@@ -33,7 +33,7 @@ class RetransmissionErrorCounter {
   // Increments the retransmission timer. If the maximum error count has been
   // reached, `false` will be returned.
   bool Increment(absl::string_view reason);
-  bool IsExhausted() const { return counter_ > limit_; }
+  bool IsExhausted() const { return limit_.has_value() && counter_ > *limit_; }
 
   // Clears the retransmission errors.
   void Clear();
@@ -43,7 +43,7 @@ class RetransmissionErrorCounter {
 
  private:
   const std::string log_prefix_;
-  const int limit_;
+  const absl::optional<int> limit_;
   int counter_ = 0;
 };
 }  // namespace dcsctp

net/dcsctp/tx/retransmission_error_counter_test.cc

@@ -72,5 +72,15 @@ TEST(RetransmissionErrorCounterTest, ClearingCounter) {
   EXPECT_TRUE(counter.IsExhausted());
 }
 
+TEST(RetransmissionErrorCounterTest, CanBeLimitless) {
+  DcSctpOptions options;
+  options.max_retransmissions = absl::nullopt;
+  RetransmissionErrorCounter counter("log: ", options);
+  for (int i = 0; i < 100; ++i) {
+    EXPECT_TRUE(counter.Increment("test"));
+    EXPECT_FALSE(counter.IsExhausted());
+  }
+}
+
 }  // namespace
 }  // namespace dcsctp