Delete a connection only if it has timed out on writing and not receiving for 10 seconds.

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 1750 matching lines @@
   uint32 remote_priority = conn1->remote_candidate().priority();
 
   // Create a higher priority candidate and make the connection
   // receiving/writable. This will prune conn1.
   ch.AddRemoteCandidate(CreateCandidate("2.2.2.2", 2, 2));
   cricket::Connection* conn2 = WaitForConnectionTo(&ch, "2.2.2.2", 2);
   ASSERT_TRUE(conn2 != nullptr);
   conn2->ReceivedPing();
   conn2->ReceivedPingResponse();
 
-  // Wait for conn1 to be destroyed.
-  EXPECT_TRUE_WAIT(GetConnectionTo(&ch, "1.1.1.1", 1) == nullptr, 3000);
-  cricket::Port* port = GetPort(&ch);
+  // Wait for conn1 to be pruned.
+  EXPECT_TRUE_WAIT(conn1->pruned(), 3000);
+  // Destroy the connection to test SignalUnknownAddress.
+  conn1->Destroy();
+  EXPECT_TRUE_WAIT(GetConnectionTo(&ch, "1.1.1.1", 1) == nullptr, 1000);
 
   // Create a minimal STUN message with prflx priority.
   cricket::IceMessage request;
   request.SetType(cricket::STUN_BINDING_REQUEST);
   request.AddAttribute(new cricket::StunByteStringAttribute(
       cricket::STUN_ATTR_USERNAME, kIceUfrag[1]));
   uint32 prflx_priority = cricket::ICE_TYPE_PREFERENCE_PRFLX << 24;
   request.AddAttribute(new cricket::StunUInt32Attribute(
       cricket::STUN_ATTR_PRIORITY, prflx_priority));
   EXPECT_NE(prflx_priority, remote_priority);
 
+  cricket::Port* port = GetPort(&ch);
   // conn1 should be resurrected with original priority.
   port->SignalUnknownAddress(port, rtc::SocketAddress("1.1.1.1", 1),
                              cricket::PROTO_UDP, &request, kIceUfrag[1], false);
   conn1 = WaitForConnectionTo(&ch, "1.1.1.1", 1);
   ASSERT_TRUE(conn1 != nullptr);
   EXPECT_EQ(conn1->remote_candidate().priority(), remote_priority);
 
   // conn3, a real prflx connection, should have prflx priority.
   port->SignalUnknownAddress(port, rtc::SocketAddress("3.3.3.3", 1),
                              cricket::PROTO_UDP, &request, kIceUfrag[1], false);
@@ skipping 238 matching lines @@
 
   ch.AddRemoteCandidate(CreateCandidate("3.3.3.3", 3, 1));
   cricket::Connection* conn3 = WaitForConnectionTo(&ch, "3.3.3.3", 3);
   ASSERT_TRUE(conn3 != nullptr);
   // The best connection should still be conn2. Even through conn3 has lower
   // priority and is not receiving/writable, it is not pruned because the best
   // connection is not receiving.
   WAIT(conn3->pruned(), 1000);
   EXPECT_FALSE(conn3->pruned());
 }
+
+// Test that when a low-priority connection is pruned, it will not be deleted
+// right away.
+TEST_F(P2PTransportChannelPingTest, TestDontDeleteRightAwayWhenPrune) {

[pthatcher1, 2015/09/28 23:07:27]

...WhenPrune => ...WhenPruned

[honghaiz3, 2015/09/29 18:47:49]

Done.

+  cricket::FakePortAllocator pa(rtc::Thread::Current(), nullptr);
+  cricket::P2PTransportChannel ch("test channel", 1, nullptr, &pa);
+  PrepareChannel(&ch);
+  ch.Connect();
+  ch.MaybeStartGathering();
+  ch.AddRemoteCandidate(CreateCandidate("1.1.1.1", 1, 100));
+  cricket::Connection* conn1 = WaitForConnectionTo(&ch, "1.1.1.1", 1);
+  ASSERT_TRUE(conn1 != nullptr);
+  EXPECT_EQ(conn1, ch.best_connection());
+  conn1->ReceivedPingResponse();  // Becomes writable and receiving
+
+  // Add a low-priority connection, which will be pruned right away, but it
+  // will not be deleted right away.
+  ch.AddRemoteCandidate(CreateCandidate("2.2.2.2", 2, 1));
+  cricket::Connection* conn2 = WaitForConnectionTo(&ch, "2.2.2.2", 2);
+  ASSERT_TRUE(conn2 != nullptr);
+  EXPECT_TRUE_WAIT(conn2->pruned(), 1000);
+  LOG(LS_INFO) << "A connection is pruned";
+  WAIT((conn2 = GetConnectionTo(&ch, "2.2.2.2", 2)) == nullptr, 1000);
+  EXPECT_FALSE(conn2 == nullptr);

[pthatcher1, 2015/09/28 23:07:27]

I think that what we really want to test is that when conn1 is destroyed, then
conn2 will begin pinging and can become writable and receiving.  Can we make it
more complete like that?

[honghaiz3, 2015/09/29 18:47:49]

Done. conn2 will start to ping once conn1 is not receiving. 

+}
+
+// Test that GetState returns the state correctly.
+TEST_F(P2PTransportChannelPingTest, TestGetState) {
+  cricket::FakePortAllocator pa(rtc::Thread::Current(), nullptr);
+  cricket::P2PTransportChannel ch("test channel", 1, nullptr, &pa);
+  PrepareChannel(&ch);
+  ch.Connect();
+  ch.MaybeStartGathering();
+  ch.AddRemoteCandidate(CreateCandidate("1.1.1.1", 1, 100));
+  ch.AddRemoteCandidate(CreateCandidate("2.2.2.2", 2, 1));
+  cricket::Connection* conn1 = WaitForConnectionTo(&ch, "1.1.1.1", 1);
+  cricket::Connection* conn2 = WaitForConnectionTo(&ch, "2.2.2.2", 2);
+  ASSERT_TRUE(conn1 != nullptr);
+  ASSERT_TRUE(conn2 != nullptr);
+  // Now there are two connections, so the transport channel is connecting.

[pthatcher1, 2015/09/28 23:07:27]

We need to check STATE_INIT before there are any connections.

[honghaiz3, 2015/09/29 18:47:49]

Done.

+  EXPECT_EQ(cricket::TransportChannelState::STATE_CONNECTING, ch.GetState());
+  // |conn1| becomes writable and receiving; it then should prune |conn2|.
+  conn1->ReceivedPingResponse();
+  EXPECT_TRUE_WAIT(conn2->pruned(), 1000);
+  EXPECT_EQ(cricket::TransportChannelState::STATE_COMPLETED, ch.GetState());

[pthatcher1, 2015/09/28 23:07:27]

To be more complete, I think we need to test the multi-network part of as well. 

[honghaiz3, 2015/09/29 18:47:49]

Added GetState tests in the MultihomedTest. 

+  conn1->Prune();  // All connections are pruned.
+  EXPECT_EQ(cricket::TransportChannelState::STATE_FAILED, ch.GetState());

[pthatcher1, 2015/09/28 23:07:27]

We should also test that not only Prune() get us in this state, but so does a
lose-writability event.

[honghaiz3, 2015/09/29 18:47:49]

Prune is used to emulate losing writability. 
If we want to do an actual lose-writability event, we will need one of the
following
1. Wait for 15 seconds until write timeout. That's pretty long for testing. 
2. Change the p2ptransportchannel so that we can change the write timeout
values. Currently it hard-coded with a constant. 
3. Call set_write_state to set to write_timeout. But this is pretty similar to
call Prune. 
Do you want to proceed along one of these?

[pthatcher1, 2015/09/29 19:51:32]

Is there a way we can just set_write_state(WRITE_TIMEOUT)?

[honghaiz3, 2015/09/29 20:40:42]

Yes although set_write_state() is protected. We can change it to public and add
comments saying it is for testing. But What Prune does is really just call
set_write_state(WRITE_TIMEOUT) (plus a few other things). 
Maybe we just revisit this when changing the behavior of Prune?

+}