Do not switch to a high-cost connection that is not receiving

webrtc/p2p/base/p2ptransportchannel_unittest.cc

 /*
  *  Copyright 2009 The WebRTC Project Authors. All rights reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 36 matching lines @@
 static const int LOW_RTT = 20;
 // Addresses on the public internet.
 static const SocketAddress kPublicAddrs[2] =
     { SocketAddress("11.11.11.11", 0), SocketAddress("22.22.22.22", 0) };
 // IPv6 Addresses on the public internet.
 static const SocketAddress kIPv6PublicAddrs[2] = {
     SocketAddress("2400:4030:1:2c00:be30:abcd:efab:cdef", 0),
     SocketAddress("2620:0:1000:1b03:2e41:38ff:fea6:f2a4", 0)
 };
 // For configuring multihomed clients.
-static const SocketAddress kAlternateAddrs[2] =
-    { SocketAddress("11.11.11.101", 0), SocketAddress("22.22.22.202", 0) };
+static const SocketAddress kAlternateAddrs[2] = {
+    SocketAddress("101.101.101.101", 0), SocketAddress("202.202.202.202", 0)};
 // Addresses for HTTP proxy servers.
 static const SocketAddress kHttpsProxyAddrs[2] =
     { SocketAddress("11.11.11.1", 443), SocketAddress("22.22.22.1", 443) };
 // Addresses for SOCKS proxy servers.
 static const SocketAddress kSocksProxyAddrs[2] =
     { SocketAddress("11.11.11.1", 1080), SocketAddress("22.22.22.1", 1080) };
 // Internal addresses for NAT boxes.
 static const SocketAddress kNatAddrs[2] =
     { SocketAddress("192.168.1.1", 0), SocketAddress("192.168.2.1", 0) };
 // Private addresses inside the NAT private networks.
@@ skipping 619 matching lines @@
     } else {
       main_->Post(RTC_FROM_HERE, this, MSG_ADD_CANDIDATES,
                   new CandidatesData(ch, c));
     }
   }
   void OnSelectedCandidatePairChanged(
       TransportChannel* transport_channel,
       CandidatePairInterface* selected_candidate_pair,
       int last_sent_packet_id,
       bool ready_to_send) {
-    ++selected_candidate_pair_switches_;
+    // Do not count if it switches to nullptr. This may happen if all
+    // connections timed out.
+    if (selected_candidate_pair != nullptr) {
+      ++selected_candidate_pair_switches_;
+    }
   }
 
   int reset_selected_candidate_pair_switches() {
     int switches = selected_candidate_pair_switches_;
     selected_candidate_pair_switches_ = 0;
     return switches;
   }
 
   void PauseCandidates(int endpoint) {
     GetEndpoint(endpoint)->save_candidates_ = true;
@@ skipping 1144 matching lines @@
       cricket::P2PTransportChannel* channel,
       const SocketAddress& address) {
     for (cricket::Connection* conn : channel->connections()) {
       if (conn->remote_candidate().address().EqualIPs(address)) {
         return conn;
       }
     }
     return nullptr;
   }
 
-  const cricket::Connection* GetConnectionWithLocalAddress(
+  cricket::Connection* GetConnectionWithLocalAddress(
       cricket::P2PTransportChannel* channel,
       const SocketAddress& address) {
     for (cricket::Connection* conn : channel->connections()) {
       if (conn->local_candidate().address().EqualIPs(address)) {
         return conn;
       }
     }
     return nullptr;
   }
 
@@ skipping 299 matching lines @@
   // dampening.
   EXPECT_TRUE_SIMULATED_WAIT(ep1_ch1()->selected_connection()->receiving() &&
                                  ep2_ch1()->selected_connection()->receiving(),
                              3000, clock);
   EXPECT_TRUE(LocalCandidate(ep1_ch1())->address().EqualIPs(kPublicAddrs[0]));
   EXPECT_TRUE(RemoteCandidate(ep2_ch1())->address().EqualIPs(kPublicAddrs[0]));
   EXPECT_EQ(0, reset_selected_candidate_pair_switches());
   DestroyChannels();
 }
 
+// Tests that if the remote side's network failed, it won't cause the local
+// side to switch connections and networks.
+TEST_F(P2PTransportChannelMultihomedTest, TestRemoteFailover) {
+  rtc::ScopedFakeClock clock;
+  // Start the clock at 100 seconds. P2PTransportChannel::IsPingable assumes
+  // that the clock time is sufficently large.
+  clock.AdvanceTime(rtc::TimeDelta::FromSeconds(100u));

[Taylor Brandstetter, 2016/08/11 22:54:40]

Can you explain why this is necessary? Can IsPingable be fixed?

[honghaiz3, 2016/08/12 17:04:26]

A backup connection is pinged at a slower rate (which is configurable). This is
controlled by checking the difference between now and the last time ping
response is received. When no ping response is received, the latter is 0. If the
clock time starts from 0, it may take a long time for the backup connection to
get the first chance to be pinged. In this test, we need to make sure the backup
connection become writable first.

It never has this issue with real clock. 

I think it may be good to initialize the FakeClock with the current reading of
the wall clock when it is created. That would have avoided several test issues
we have incurred and that will simulate more practical settings.  

[Taylor Brandstetter, 2016/08/15 17:37:32]

So then the issue is that 0 is treated as a special value meaning "no ping
responses received"? I know this is a corner case issue, but can the logic be
changed so that "no ping responses received" is detected in some other way?

Also, I disagree about initializing the fake clock with a wall clock time. The
fake clock should allow writing fully repeatable tests, so it should use a
repeatable initial time. And if we did want to do time "fuzzing" for a specific
test, it would be better to use fully random values rather than the wall clock
time. The wall-clock time on the VMs we run the tests on could be biased. I
believe this has caused at least one bug to go under the radar in the past; a
64-bit timestamp was being cast to a 32-bit timestamp but the bot didn't catch
this because the timestamp always ended up in a 32-bit value range.

[honghaiz3, 2016/08/16 00:43:07]

Added checking on the "no ping response is received using the rtt_samples(). 
If a 32-bit initial timestamp may be an issue, using 0 as the default initial
timestamp may be a bigger issue. Using the random value might be better than the
wall clock time in the case you described, but we need to choose the range and
distribution reasonably. I am not sure if it is worth the complexity. If using
the call-clock is an issue, it will be an issue with a non-fake clock anyway
because the latter is what we use for all non-fake clocks. So it will be as good
as the real clock.  

Probably we want to allow the test code to initialize the fake clock with a
chosen value. 

+  // The interface names are chosen so that |cellular| would have higher
+  // candidate priority and higher cost.
+  auto& wifi = kPublicAddrs;
+  auto& cellular = kAlternateAddrs;
+  AddAddress(0, wifi[0], "wifi0", rtc::ADAPTER_TYPE_WIFI);
+  AddAddress(0, cellular[0], "cellular0", rtc::ADAPTER_TYPE_CELLULAR);
+  AddAddress(1, wifi[1], "wifi0", rtc::ADAPTER_TYPE_WIFI);
+
+  // Use only local ports for simplicity.
+  SetAllocatorFlags(0, kOnlyLocalPorts);
+  SetAllocatorFlags(1, kOnlyLocalPorts);
+  // Create channels and let them go writable, as usual.
+  CreateChannels(1);
+  // Make the receiving timeout shorter for testing.
+  // Set the backup connection ping interval to 25s.
+  IceConfig config = CreateIceConfig(1000, GATHER_ONCE, 25000);
+  // Ping the best connection more frequently since we don't have traffic.
+  config.stable_writable_connection_ping_interval = 900;
+  ep1_ch1()->SetIceConfig(config);
+  ep2_ch1()->SetIceConfig(config);
+  // Need to wait to make sure the connections on both networks are writable.
+  EXPECT_TRUE_SIMULATED_WAIT(ep1_ch1()->receiving() && ep1_ch1()->writable() &&
+                                 ep2_ch1()->receiving() &&
+                                 ep2_ch1()->writable(),
+                             3000, clock);
+  EXPECT_TRUE_SIMULATED_WAIT(
+      ep1_ch1()->selected_connection() &&
+          LocalCandidate(ep1_ch1())->address().EqualIPs(wifi[0]) &&
+          RemoteCandidate(ep1_ch1())->address().EqualIPs(wifi[1]),
+      kDefaultTimeout, clock);
+  Connection* backup_conn =
+      GetConnectionWithLocalAddress(ep1_ch1(), cellular[0]);
+  ASSERT_NE(nullptr, backup_conn);
+  // After a short while, the backup connection will be writable but not
+  // receiving because backup connection is pinged at a slower rate.
+  EXPECT_TRUE_SIMULATED_WAIT(
+      backup_conn->writable() && !backup_conn->receiving(), 5000, clock);
+  reset_selected_candidate_pair_switches();
+  // Blackhole any traffic to or from the remote WiFi networks.
+  LOG(LS_INFO) << "Failing over...";
+  fw()->AddRule(false, rtc::FP_ANY, rtc::FD_ANY, wifi[1]);
+
+  int num_switches = 0;
+  SIMULATED_WAIT((num_switches = reset_selected_candidate_pair_switches()) > 0,
+                 20000, clock);
+  EXPECT_EQ(0, num_switches);
+  DestroyChannels();
+}
+
 // Tests that a Wifi-Wifi connection has the highest precedence.
 TEST_F(P2PTransportChannelMultihomedTest, TestPreferWifiToWifiConnection) {
   // The interface names are chosen so that |cellular| would have higher
   // candidate priority if it is not for the network type.
   auto& wifi = kAlternateAddrs;
   auto& cellular = kPublicAddrs;
   AddAddress(0, wifi[0], "test0", rtc::ADAPTER_TYPE_WIFI);
   AddAddress(0, cellular[0], "test1", rtc::ADAPTER_TYPE_CELLULAR);
   AddAddress(1, wifi[1], "test0", rtc::ADAPTER_TYPE_WIFI);
   AddAddress(1, cellular[1], "test1", rtc::ADAPTER_TYPE_CELLULAR);
@@ skipping 1761 matching lines @@
 
   // TCP Relay/Relay is the next.
   VerifyNextPingableConnection(RELAY_PORT_TYPE, RELAY_PORT_TYPE,
                                TCP_PROTOCOL_NAME);
 
   // Finally, Local/Relay will be pinged.
   VerifyNextPingableConnection(LOCAL_PORT_TYPE, RELAY_PORT_TYPE);
 }
 
 }  // namespace cricket {