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

webrtc/p2p/base/p2ptransportchannel_unittest.cc

Index: webrtc/p2p/base/p2ptransportchannel_unittest.cc
diff --git a/webrtc/p2p/base/p2ptransportchannel_unittest.cc b/webrtc/p2p/base/p2ptransportchannel_unittest.cc
index 8c25d820685bc773ba9efa2c1a72a5ef21608c59..73799d97992b1fed36116eecb228299d75babd73 100644
--- a/webrtc/p2p/base/p2ptransportchannel_unittest.cc
+++ b/webrtc/p2p/base/p2ptransportchannel_unittest.cc
@@ -1768,9 +1768,11 @@ TEST_F(P2PTransportChannelPingTest, ConnectionResurrection) {
   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;
@@ -1782,6 +1784,7 @@ TEST_F(P2PTransportChannelPingTest, ConnectionResurrection) {
       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);
@@ -2040,3 +2043,51 @@ TEST_F(P2PTransportChannelPingTest, TestDontPruneWhenWeak) {
   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?

+}