A few updates on connection states

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 1475 matching lines @@
 // Test that we can establish connectivity when both peers are multihomed.
 TEST_F(P2PTransportChannelMultihomedTest, DISABLED_TestBasic) {
   AddAddress(0, kPublicAddrs[0]);
   AddAddress(0, kAlternateAddrs[0]);
   AddAddress(1, kPublicAddrs[1]);
   AddAddress(1, kAlternateAddrs[1]);
   Test(kLocalUdpToLocalUdp);
 }
 
 // Test that we can quickly switch links if an interface goes down.
-TEST_F(P2PTransportChannelMultihomedTest, TestFailover) {
+// The controlled side has two interfaces and one will die.
+TEST_F(P2PTransportChannelMultihomedTest, TestFailoverControlledSide) {
   AddAddress(0, kPublicAddrs[0]);
   // Adding alternate address will make sure |kPublicAddrs| has the higher
   // priority than others. This is due to FakeNetwork::AddInterface method.
   AddAddress(1, kAlternateAddrs[1]);
   AddAddress(1, kPublicAddrs[1]);
 
   // Use only local ports for simplicity.
   SetAllocatorFlags(0, kOnlyLocalPorts);
   SetAllocatorFlags(1, kOnlyLocalPorts);
 
   // Create channels and let them go writable, as usual.
   CreateChannels(1);
   EXPECT_TRUE_WAIT(ep1_ch1()->receiving() && ep1_ch1()->writable() &&
                    ep2_ch1()->receiving() && ep2_ch1()->writable(),
                    1000);
   EXPECT_TRUE(
       ep1_ch1()->best_connection() && ep2_ch1()->best_connection() &&
       LocalCandidate(ep1_ch1())->address().EqualIPs(kPublicAddrs[0]) &&
       RemoteCandidate(ep1_ch1())->address().EqualIPs(kPublicAddrs[1]));
 
   // Blackhole any traffic to or from the public addrs.
   LOG(LS_INFO) << "Failing over...";
-  fw()->AddRule(false, rtc::FP_ANY, rtc::FD_ANY,
-                kPublicAddrs[1]);
+  fw()->AddRule(false, rtc::FP_ANY, rtc::FD_ANY, kPublicAddrs[1]);
+  // The best connections will switch, so keep references to them.
+  const cricket::Connection* best_connection1 = ep1_ch1()->best_connection();
+  const cricket::Connection* best_connection2 = ep2_ch1()->best_connection();
+  // We should detect loss of receiving within 2.5 seconds or so.
+  EXPECT_TRUE_WAIT(
+      !best_connection1->receiving() && !best_connection2->receiving(), 3000);

[pthatcher1, 2015/09/22 23:41:46]

See my comments below and please change this place as well.

[honghaiz3, 2015/09/23 03:16:10]

Done.

 
-  // We should detect loss of connectivity within 5 seconds or so.
-  EXPECT_TRUE_WAIT(!ep1_ch1()->writable(), 7000);
-
-  // We should switch over to use the alternate addr immediately
-  // when we lose writability.
+  // We should switch over to use the alternate addr immediately on both sides
+  // when we are not receiving.
   EXPECT_TRUE_WAIT(
-      ep1_ch1()->best_connection() && ep2_ch1()->best_connection() &&
-      LocalCandidate(ep1_ch1())->address().EqualIPs(kPublicAddrs[0]) &&
-      RemoteCandidate(ep1_ch1())->address().EqualIPs(kAlternateAddrs[1]),
-      3000);
+      ep1_ch1()->best_connection()->receiving() &&
+      ep2_ch1()->best_connection()->receiving(), 1000);
+  best_connection1 = ep1_ch1()->best_connection();
+  best_connection2 = ep2_ch1()->best_connection();
+  EXPECT_TRUE(best_connection1->writable());
+  EXPECT_TRUE(best_connection2->writable());
+  EXPECT_TRUE(LocalCandidate(ep1_ch1())->address().EqualIPs(kPublicAddrs[0]));
+  EXPECT_TRUE(
+      RemoteCandidate(ep1_ch1())->address().EqualIPs(kAlternateAddrs[1]));
+  EXPECT_TRUE(
+      LocalCandidate(ep2_ch1())->address().EqualIPs(kAlternateAddrs[1]));
 
   DestroyChannels();
 }
+
+// Test that we can quickly switch links if an interface goes down.
+// The controlling side has two interfaces and one will die.
+TEST_F(P2PTransportChannelMultihomedTest, TestFailoverControllingSide) {
+  // Adding alternate address will make sure |kPublicAddrs| has the higher
+  // priority than others. This is due to FakeNetwork::AddInterface method.
+  AddAddress(0, kAlternateAddrs[0]);
+  AddAddress(0, kPublicAddrs[0]);
+
+  AddAddress(1, kPublicAddrs[1]);
+
+  // Use only local ports for simplicity.
+  SetAllocatorFlags(0, kOnlyLocalPorts);
+  SetAllocatorFlags(1, kOnlyLocalPorts);
+
+  // Create channels and let them go writable, as usual.
+  CreateChannels(1);
+  EXPECT_TRUE_WAIT(ep1_ch1()->receiving() && ep1_ch1()->writable() &&
+                   ep2_ch1()->receiving() && ep2_ch1()->writable(),
+                   1000);
+  EXPECT_TRUE(
+      ep1_ch1()->best_connection() && ep2_ch1()->best_connection() &&
+      LocalCandidate(ep1_ch1())->address().EqualIPs(kPublicAddrs[0]) &&
+      RemoteCandidate(ep1_ch1())->address().EqualIPs(kPublicAddrs[1]));
+
+  // Blackhole any traffic to or from the public addrs.
+  LOG(LS_INFO) << "Failing over...";
+  fw()->AddRule(false, rtc::FP_ANY, rtc::FD_ANY, kPublicAddrs[0]);
+  // The best connections will switch, so keep references to them.
+  const cricket::Connection* best_connection1 = ep1_ch1()->best_connection();
+  const cricket::Connection* best_connection2 = ep2_ch1()->best_connection();
+  // We should detect loss of receiving within 2.5 seconds or so.
+  EXPECT_TRUE_WAIT(
+      !best_connection1->receiving() && !best_connection2->receiving(), 3000);

[pthatcher1, 2015/09/22 23:41:46]

Can you lower the receive timeout for the tests to make them run faster?    And
can you bump the expect timeout to make sure this test doesn't go flaky?

[honghaiz3, 2015/09/23 03:16:10]

I set the receive timeout to 1 second. Because of that, I think we do not need
to increase the expect timeout any more. 

+
+  // We should switch over to use the alternate addr immediately on both sides
+  // when we are not receiving.
+  EXPECT_TRUE_WAIT(
+      ep1_ch1()->best_connection()->receiving() &&
+      ep2_ch1()->best_connection()->receiving(), 1000);
+  best_connection1 = ep1_ch1()->best_connection();
+  best_connection2 = ep2_ch1()->best_connection();
+  EXPECT_TRUE(best_connection1->writable());
+  EXPECT_TRUE(best_connection2->writable());

[pthatcher1, 2015/09/22 23:41:46]

Do you really need to check this?  It doesn't matter right?   And if a test bot
gets really slow, this might be false by the time you get here.

[honghaiz3, 2015/09/23 03:16:10]

Right. removed these two lines here and above.

+  EXPECT_TRUE(
+    LocalCandidate(ep1_ch1())->address().EqualIPs(kAlternateAddrs[0]));
+  EXPECT_TRUE(RemoteCandidate(ep1_ch1())->address().EqualIPs(kPublicAddrs[1]));
+  EXPECT_TRUE(
+      RemoteCandidate(ep2_ch1())->address().EqualIPs(kAlternateAddrs[0]));
+
+  DestroyChannels();
+}
 
 /*
 
 TODO(pthatcher): Once have a way to handle network interfaces changes
 without signalling an ICE restart, put a test like this back.  In the
 mean time, this test only worked for GICE.  With ICE, it's currently
 not possible without an ICE restart.
 
 // Test that we can switch links in a coordinated fashion.
 TEST_F(P2PTransportChannelMultihomedTest, TestDrain) {
@@ skipping 388 matching lines @@
   conn3->ReceivedPingResponse();           // Become writable.
   EXPECT_EQ(conn3, ch.best_connection());
 
   // Now another data packet will not switch the best connection because the
   // best connection was nominated by the controlling side.
   conn2->ReceivedPing();
   conn2->ReceivedPingResponse();
   conn2->OnReadPacket("XYZ", 3, rtc::CreatePacketTime(0));
   EXPECT_EQ(conn3, ch.best_connection());
 }