From b5db1ec0e5760e370c2afe7e4171a9128039ae53 Mon Sep 17 00:00:00 2001 From: Honghai Zhang Date: Thu, 28 Jul 2016 13:23:05 -0700 Subject: [PATCH] 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-Commit-Position: refs/heads/master@{#13563} --- webrtc/p2p/base/port.cc | 15 +++++++++++---- webrtc/p2p/base/port.h | 10 +++------- webrtc/p2p/base/port_unittest.cc | 23 +++++++++++++++++++---- 3 files changed, 33 insertions(+), 15 deletions(-) diff --git a/webrtc/p2p/base/port.cc b/webrtc/p2p/base/port.cc index b6173ebf45..535dd90f99 100644 --- a/webrtc/p2p/base/port.cc +++ b/webrtc/p2p/base/port.cc @@ -645,7 +645,7 @@ void Port::SendBindingErrorResponse(StunMessage* request, } 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 @@ void Port::OnConnectionDestroyed(Connection* conn) { // 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"; diff --git a/webrtc/p2p/base/port.h b/webrtc/p2p/base/port.h index 37926c9556..811ffed2cc 100644 --- a/webrtc/p2p/base/port.h +++ b/webrtc/p2p/base/port.h @@ -297,10 +297,7 @@ class Port : public PortInterface, public rtc::MessageHandler, 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 @@ class Port : public PortInterface, public rtc::MessageHandler, // 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 @@ class Port : public PortInterface, public rtc::MessageHandler, // (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; }; diff --git a/webrtc/p2p/base/port_unittest.cc b/webrtc/p2p/base/port_unittest.cc index e156e017fe..e84af9f2be 100644 --- a/webrtc/p2p/base/port_unittest.cc +++ b/webrtc/p2p/base/port_unittest.cc @@ -2592,15 +2592,17 @@ TEST_F(PortTest, TestControllingNoTimeout) { } // 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 @@ TEST_F(PortTest, TestControlledTimeout) { // 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