commit d9f9d6e87397bd8f34b46ac1ea0db48522058eb7
author Honghai Zhang <honghaiz@webrtc.org> Thu Jul 28 20:23:05 2016
committer Honghai Zhang <honghaiz@webrtc.org> Thu Jul 28 20:23:13 2016
tree 5a269ccd0ea16bc6c6244624762eba511596b87d
parent 454a8230ea7d973fa5b913010928a4d59211b0d1

Delay destroying a port if new connections are created and destroyed.

If all connections on a port is destroyed, it will schedule an event
to check if it is dead after a timeout. Previously if a new connection
is created but destroyed before the event is fired, it will destroy the
port. With this change, we will not destoy it until it times out again
after the last created connection is destroyed.

BUG=
R=pthatcher@webrtc.org, zhihuang@webrtc.org

Review URL: https://codereview.webrtc.org/2184013003 .

Cr-Original-Commit-Position: refs/heads/master@{#13563}
Cr-Mirrored-From: https://chromium.googlesource.com/external/webrtc
Cr-Mirrored-Commit: b5db1ec0e5760e370c2afe7e4171a9128039ae53

p2p/base/port.cc

diff --git a/p2p/base/port.cc b/p2p/base/port.cc
index b6173eb..535dd90 100644
--- a/p2p/base/port.cc
+++ b/p2p/base/port.cc
@@ -645,7 +645,7 @@
 }
 
 void Port::OnMessage(rtc::Message *pmsg) {
-  ASSERT(pmsg->message_id == MSG_DEAD);
+  ASSERT(pmsg->message_id == MSG_CHECK_DEAD);
   if (dead()) {
     Destroy();
   }
@@ -702,12 +702,19 @@
   // On the controlled side, ports time out after all connections fail.
   // Note: If a new connection is added after this message is posted, but it
   // fails and is removed before kPortTimeoutDelay, then this message will
-  // still cause the Port to be destroyed.
-  if (dead()) {
-    thread_->PostDelayed(RTC_FROM_HERE, timeout_delay_, this, MSG_DEAD);
+  // not cause the Port to be destroyed.
+  if (ice_role_ == ICEROLE_CONTROLLED && connections_.empty()) {
+    last_time_all_connections_removed_ = rtc::TimeMillis();
+    thread_->PostDelayed(RTC_FROM_HERE, timeout_delay_, this, MSG_CHECK_DEAD);
   }
 }
 
+bool Port::dead() const {
+  return ice_role_ == ICEROLE_CONTROLLED && connections_.empty() &&
+         rtc::TimeMillis() - last_time_all_connections_removed_ >=
+             timeout_delay_;
+}
+
 void Port::Destroy() {
   ASSERT(connections_.empty());
   LOG_J(LS_INFO, this) << "Port deleted";

p2p/base/port.h

diff --git a/p2p/base/port.h b/p2p/base/port.h
index 37926c9..811ffed 100644
--- a/p2p/base/port.h
+++ b/p2p/base/port.h
@@ -297,10 +297,7 @@
   int16_t network_cost() const { return network_cost_; }
 
  protected:
-  enum {
-    MSG_DEAD = 0,
-    MSG_FIRST_AVAILABLE
-  };
+  enum { MSG_CHECK_DEAD = 0, MSG_FIRST_AVAILABLE };
 
   virtual void UpdateNetworkCost();
 
@@ -359,9 +356,7 @@
 
   // Whether this port is dead, and hence, should be destroyed on the controlled
   // side.
-  bool dead() const {
-    return ice_role_ == ICEROLE_CONTROLLED && connections_.empty();
-  }
+  bool dead() const;
 
   void OnNetworkTypeChanged(const rtc::Network* network);
 
@@ -401,6 +396,7 @@
   // (WiFi. vs. Cellular). It takes precedence over the priority when
   // comparing two connections.
   uint16_t network_cost_;
+  int64_t last_time_all_connections_removed_ = 0;
 
   friend class Connection;
 };

p2p/base/port_unittest.cc

diff --git a/p2p/base/port_unittest.cc b/p2p/base/port_unittest.cc
index e156e01..e84af9f 100644
--- a/p2p/base/port_unittest.cc
+++ b/p2p/base/port_unittest.cc
@@ -2592,15 +2592,17 @@
 }
 
 // This test case verifies that the CONTROLLED port does time out, but only
-// after connectivity is lost.
+// after connectivity is lost and no connection was created during the timeout
+// period.
 TEST_F(PortTest, TestControlledTimeout) {
+  rtc::ScopedFakeClock clock;
   UDPPort* port1 = CreateUdpPort(kLocalAddr1);
   port1->SetIceRole(cricket::ICEROLE_CONTROLLING);
   port1->SetIceTiebreaker(kTiebreaker1);
 
   UDPPort* port2 = CreateUdpPort(kLocalAddr2);
   ConnectToSignalDestroyed(port2);
-  port2->set_timeout_delay(10);  // milliseconds
+  port2->set_timeout_delay(100);  // milliseconds
   port2->SetIceRole(cricket::ICEROLE_CONTROLLED);
   port2->SetIceTiebreaker(kTiebreaker2);
 
@@ -2617,8 +2619,21 @@
   // Simulate a connection that succeeds, and then is destroyed.
   StartConnectAndStopChannels(&ch1, &ch2);
 
-  // The controlled port should be destroyed after 10 milliseconds.
-  EXPECT_TRUE_WAIT(destroyed(), kTimeout);
+  SIMULATED_WAIT(false, 80, clock);
+  ch2.CreateConnection(GetCandidate(ch1.port()));
+
+  // ch2 creates a connection so it will not be destroyed.
+  SIMULATED_WAIT(destroyed(), 80, clock);
+  EXPECT_FALSE(destroyed());
+
+  // Even if ch2 stops now, it won't be destroyed until 100ms after the
+  // connection is destroyed.
+  ch2.Stop();
+  SIMULATED_WAIT(destroyed(), 80, clock);
+  EXPECT_FALSE(destroyed());
+
+  // The controlled port should be destroyed after timeout.
+  EXPECT_TRUE_SIMULATED_WAIT(destroyed(), 30, clock);
 }
 
 // This test case verifies that if the role of a port changes from controlled