From 5429d71022cb7d29bfcee07995e232801fe1ed2a Mon Sep 17 00:00:00 2001
From: Victor Boivie <boivie@webrtc.org>
Date: Wed, 19 May 2021 22:49:28 +0200
Subject: [PATCH] dcsctp: Allow heartbeats to be disabled

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}
---
 net/dcsctp/public/dcsctp_options.h          |  2 +-
 net/dcsctp/socket/heartbeat_handler.cc      |  7 +++-
 net/dcsctp/socket/heartbeat_handler_test.cc | 44 ++++++++++++++++++---
 3 files changed, 46 insertions(+), 7 deletions(-)

diff --git a/net/dcsctp/public/dcsctp_options.h b/net/dcsctp/public/dcsctp_options.h
index 1c4e328ab6..caefcff4f5 100644
--- a/net/dcsctp/public/dcsctp_options.h
+++ b/net/dcsctp/public/dcsctp_options.h
@@ -112,7 +112,7 @@ struct DcSctpOptions {
   // T2-shutdown timeout.
   DurationMs t2_shutdown_timeout = DurationMs(1000);
 
-  // Hearbeat interval (on idle connections only).
+  // Hearbeat interval (on idle connections only). Set to zero to disable.
   DurationMs heartbeat_interval = DurationMs(30000);
 
   // The maximum time when a SACK will be sent from the arrival of an
diff --git a/net/dcsctp/socket/heartbeat_handler.cc b/net/dcsctp/socket/heartbeat_handler.cc
index 30a0001c68..78616d1033 100644
--- a/net/dcsctp/socket/heartbeat_handler.cc
+++ b/net/dcsctp/socket/heartbeat_handler.cc
@@ -104,10 +104,15 @@ HeartbeatHandler::HeartbeatHandler(absl::string_view log_prefix,
                        TimerBackoffAlgorithm::kExponential,
                        /*max_restarts=*/0))) {
   // The interval timer must always be running as long as the association is up.
-  interval_timer_->Start();
+  RestartTimer();
 }
 
 void HeartbeatHandler::RestartTimer() {
+  if (interval_duration_ == DurationMs(0)) {
+    // Heartbeating has been disabled.
+    return;
+  }
+
   if (interval_duration_should_include_rtt_) {
     // The RTT should be used, but it's not easy accessible. The RTO will
     // suffice.
diff --git a/net/dcsctp/socket/heartbeat_handler_test.cc b/net/dcsctp/socket/heartbeat_handler_test.cc
index 20c1d465db..2c5df9fd92 100644
--- a/net/dcsctp/socket/heartbeat_handler_test.cc
+++ b/net/dcsctp/socket/heartbeat_handler_test.cc
@@ -30,17 +30,19 @@ using ::testing::NiceMock;
 using ::testing::Return;
 using ::testing::SizeIs;
 
-DcSctpOptions MakeOptions() {
+constexpr DurationMs kHeartbeatInterval = DurationMs(30'000);
+
+DcSctpOptions MakeOptions(DurationMs heartbeat_interval) {
   DcSctpOptions options;
   options.heartbeat_interval_include_rtt = false;
-  options.heartbeat_interval = DurationMs(30'000);
+  options.heartbeat_interval = heartbeat_interval;
   return options;
 }
 
-class HeartbeatHandlerTest : public testing::Test {
+class HeartbeatHandlerTestBase : public testing::Test {
  protected:
-  HeartbeatHandlerTest()
-      : options_(MakeOptions()),
+  explicit HeartbeatHandlerTestBase(DurationMs heartbeat_interval)
+      : options_(MakeOptions(heartbeat_interval)),
         context_(&callbacks_),
         timer_manager_([this]() { return callbacks_.CreateTimeout(); }),
         handler_("log: ", options_, &context_, &timer_manager_) {}
@@ -63,6 +65,31 @@ class HeartbeatHandlerTest : public testing::Test {
   HeartbeatHandler handler_;
 };
 
+class HeartbeatHandlerTest : public HeartbeatHandlerTestBase {
+ protected:
+  HeartbeatHandlerTest() : HeartbeatHandlerTestBase(kHeartbeatInterval) {}
+};
+
+class DisabledHeartbeatHandlerTest : public HeartbeatHandlerTestBase {
+ protected:
+  DisabledHeartbeatHandlerTest() : HeartbeatHandlerTestBase(DurationMs(0)) {}
+};
+
+TEST_F(HeartbeatHandlerTest, HasRunningHeartbeatIntervalTimer) {
+  AdvanceTime(options_.heartbeat_interval);
+
+  // Validate that a heartbeat request was sent.
+  std::vector<uint8_t> payload = callbacks_.ConsumeSentPacket();
+  ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet, SctpPacket::Parse(payload));
+  ASSERT_THAT(packet.descriptors(), SizeIs(1));
+
+  ASSERT_HAS_VALUE_AND_ASSIGN(
+      HeartbeatRequestChunk request,
+      HeartbeatRequestChunk::Parse(packet.descriptors()[0].data));
+
+  EXPECT_TRUE(request.info().has_value());
+}
+
 TEST_F(HeartbeatHandlerTest, RepliesToHeartbeatRequests) {
   uint8_t info_data[] = {1, 2, 3, 4, 5};
   HeartbeatRequestChunk request(
@@ -120,5 +147,12 @@ TEST_F(HeartbeatHandlerTest, IncreasesErrorIfNotAckedInTime) {
   AdvanceTime(rto);
 }
 
+TEST_F(DisabledHeartbeatHandlerTest, IsReallyDisabled) {
+  AdvanceTime(options_.heartbeat_interval);
+
+  // Validate that a request was NOT sent.
+  EXPECT_THAT(callbacks_.ConsumeSentPacket(), IsEmpty());
+}
+
 }  // namespace
 }  // namespace dcsctp