Add field trial to let idle connection live longer than 30s

A connection is currently deleted if it has not recevied anything for
30s. This patch adds a field trial that allows modifying this value
if no pings are outstanding.

The motivation for this is to experiment with pinging slower than
once per 30s in order to save battery.

Bug: webrtc:10282
Change-Id: I3272b9d68d44fc30379bd9a6c643db6b09766486
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/175005
Commit-Queue: Jonas Oreland <[email protected]>
Reviewed-by: Harald Alvestrand <[email protected]>
Cr-Commit-Position: refs/heads/master@{#31239}

p2p/base/connection.cc

@@ -918,12 +918,31 @@ void Connection::ReceivedPingResponse(
 
 bool Connection::dead(int64_t now) const {
   if (last_received() > 0) {
-    // If it has ever received anything, we keep it alive until it hasn't
-    // received anything for DEAD_CONNECTION_RECEIVE_TIMEOUT. This covers the
-    // normal case of a successfully used connection that stops working. This
-    // also allows a remote peer to continue pinging over a locally inactive
-    // (pruned) connection.
-    return (now > (last_received() + DEAD_CONNECTION_RECEIVE_TIMEOUT));
+    // If it has ever received anything, we keep it alive
+    // - if it has recevied last DEAD_CONNECTION_RECEIVE_TIMEOUT (30s)
+    // - if it has a ping outstanding shorter than
+    // DEAD_CONNECTION_RECEIVE_TIMEOUT (30s)
+    // - else if IDLE let it live field_trials_->dead_connection_timeout_ms
+    //
+    // This covers the normal case of a successfully used connection that stops
+    // working. This also allows a remote peer to continue pinging over a
+    // locally inactive (pruned) connection. This also allows the local agent to
+    // ping with longer interval than 30s as long as it shorter than
+    // |dead_connection_timeout_ms|.
+    if (now <= (last_received() + DEAD_CONNECTION_RECEIVE_TIMEOUT)) {
+      // Not dead since we have received the last 30s.
+      return false;
+    }
+    if (!pings_since_last_response_.empty()) {
+      // Outstanding pings: let it live until the ping is unreplied for
+      // DEAD_CONNECTION_RECEIVE_TIMEOUT.
+      return now > (pings_since_last_response_[0].sent_time +
+                    DEAD_CONNECTION_RECEIVE_TIMEOUT);
+    }
+
+    // No outstanding pings: let it live until
+    // field_trials_->dead_connection_timeout_ms has passed.
+    return now > (last_received() + field_trials_->dead_connection_timeout_ms);
   }
 
   if (active()) {

p2p/base/p2p_transport_channel.cc

@@ -694,9 +694,18 @@ void P2PTransportChannel::SetIceConfig(const IceConfig& config) {
       &field_trials_.send_ping_on_switch_ice_controlling,
       // Reply to nomination ASAP.
       "send_ping_on_nomination_ice_controlled",
-      &field_trials_.send_ping_on_nomination_ice_controlled)
+      &field_trials_.send_ping_on_nomination_ice_controlled,
+      // Allow connections to live untouched longer that 30s.
+      "dead_connection_timeout_ms", &field_trials_.dead_connection_timeout_ms)
       ->Parse(webrtc::field_trial::FindFullName("WebRTC-IceFieldTrials"));
 
+  if (field_trials_.dead_connection_timeout_ms < 30000) {
+    RTC_LOG(LS_WARNING) << "dead_connection_timeout_ms set to "
+                        << field_trials_.dead_connection_timeout_ms
+                        << " increasing it to 30000";
+    field_trials_.dead_connection_timeout_ms = 30000;
+  }
+
   if (field_trials_.skip_relay_to_non_relay_connections) {
     RTC_LOG(LS_INFO) << "Set skip_relay_to_non_relay_connections";
   }

p2p/base/p2p_transport_channel_ice_field_trials.h

@@ -48,6 +48,10 @@ struct IceFieldTrials {
 
   // Sending a PING directly after a nomination on ICE_CONTROLLED-side.
   bool send_ping_on_nomination_ice_controlled = false;
+
+  // The timeout after which the connection will be considered dead if no
+  // traffic is received.
+  int dead_connection_timeout_ms = 30000;
 };
 
 }  // namespace cricket

p2p/base/port_unittest.cc

@@ -64,6 +64,7 @@
 #include "rtc_base/thread.h"
 #include "rtc_base/time_utils.h"
 #include "rtc_base/virtual_socket_server.h"
+#include "test/field_trial.h"
 #include "test/gtest.h"
 
 using rtc::AsyncPacketSocket;
@@ -1298,6 +1299,77 @@ TEST_F(PortTest, TestConnectionDead) {
   EXPECT_TRUE_WAIT(ch1.conn() == nullptr, kDefaultTimeout);
 }
 
+TEST_F(PortTest, TestConnectionDeadWithDeadConnectionTimeout) {
+  TestChannel ch1(CreateUdpPort(kLocalAddr1));
+  TestChannel ch2(CreateUdpPort(kLocalAddr2));
+  // Acquire address.
+  ch1.Start();
+  ch2.Start();
+  ASSERT_EQ_WAIT(1, ch1.complete_count(), kDefaultTimeout);
+  ASSERT_EQ_WAIT(1, ch2.complete_count(), kDefaultTimeout);
+
+  // Note: set field trials manually since they are parsed by
+  // P2PTransportChannel but P2PTransportChannel is not used in this test.
+  IceFieldTrials field_trials;
+  field_trials.dead_connection_timeout_ms = 90000;
+
+  // Create a connection again and receive a ping.
+  ch1.CreateConnection(GetCandidate(ch2.port()));
+  auto conn = ch1.conn();
+  conn->SetIceFieldTrials(&field_trials);
+
+  ASSERT_NE(conn, nullptr);
+  int64_t before_last_receiving = rtc::TimeMillis();
+  conn->ReceivedPing();
+  int64_t after_last_receiving = rtc::TimeMillis();
+  // The connection will be dead after 90s
+  conn->UpdateState(before_last_receiving + 90000 - 1);
+  rtc::Thread::Current()->ProcessMessages(100);
+  EXPECT_TRUE(ch1.conn() != nullptr);
+  conn->UpdateState(after_last_receiving + 90000 + 1);
+  EXPECT_TRUE_WAIT(ch1.conn() == nullptr, kDefaultTimeout);
+}
+
+TEST_F(PortTest, TestConnectionDeadOutstandingPing) {
+  auto port1 = CreateUdpPort(kLocalAddr1);
+  port1->SetIceRole(cricket::ICEROLE_CONTROLLING);
+  port1->SetIceTiebreaker(kTiebreaker1);
+  auto port2 = CreateUdpPort(kLocalAddr2);
+  port2->SetIceRole(cricket::ICEROLE_CONTROLLED);
+  port2->SetIceTiebreaker(kTiebreaker2);
+
+  TestChannel ch1(std::move(port1));
+  TestChannel ch2(std::move(port2));
+  // Acquire address.
+  ch1.Start();
+  ch2.Start();
+  ASSERT_EQ_WAIT(1, ch1.complete_count(), kDefaultTimeout);
+  ASSERT_EQ_WAIT(1, ch2.complete_count(), kDefaultTimeout);
+
+  // Note: set field trials manually since they are parsed by
+  // P2PTransportChannel but P2PTransportChannel is not used in this test.
+  IceFieldTrials field_trials;
+  field_trials.dead_connection_timeout_ms = 360000;
+
+  // Create a connection again and receive a ping and then send
+  // a ping and keep it outstanding.
+  ch1.CreateConnection(GetCandidate(ch2.port()));
+  auto conn = ch1.conn();
+  conn->SetIceFieldTrials(&field_trials);
+
+  ASSERT_NE(conn, nullptr);
+  conn->ReceivedPing();
+  int64_t send_ping_timestamp = rtc::TimeMillis();
+  conn->Ping(send_ping_timestamp);
+
+  // The connection will be dead 30s after the ping was sent.
+  conn->UpdateState(send_ping_timestamp + DEAD_CONNECTION_RECEIVE_TIMEOUT - 1);
+  rtc::Thread::Current()->ProcessMessages(100);
+  EXPECT_TRUE(ch1.conn() != nullptr);
+  conn->UpdateState(send_ping_timestamp + DEAD_CONNECTION_RECEIVE_TIMEOUT + 1);
+  EXPECT_TRUE_WAIT(ch1.conn() == nullptr, kDefaultTimeout);
+}
+
 // This test case verifies standard ICE features in STUN messages. Currently it
 // verifies Message Integrity attribute in STUN messages and username in STUN
 // binding request will have colon (":") between remote and local username.