From 8937437872d8ff5b30866ae4de4abf005afc7539 Mon Sep 17 00:00:00 2001 From: honghaiz Date: Thu, 24 Sep 2015 13:14:47 -0700 Subject: [PATCH] Do not prune if the current best connection is weak. Otherwise, we may delete a useful connection because the current best connection may be failing. BUG= Review URL: https://codereview.webrtc.org/1364683002 Cr-Commit-Position: refs/heads/master@{#10063} --- webrtc/p2p/base/p2ptransportchannel.cc | 15 +++---- .../p2p/base/p2ptransportchannel_unittest.cc | 41 ++++++++++++++++++- webrtc/p2p/base/port.h | 2 + 3 files changed, 50 insertions(+), 8 deletions(-) diff --git a/webrtc/p2p/base/p2ptransportchannel.cc b/webrtc/p2p/base/p2ptransportchannel.cc index cd1af82c8d..2a851f5c72 100644 --- a/webrtc/p2p/base/p2ptransportchannel.cc +++ b/webrtc/p2p/base/p2ptransportchannel.cc @@ -1000,7 +1000,7 @@ void P2PTransportChannel::PruneConnections() { // which point, we would prune out the current best connection). We leave // connections on other networks because they may not be using the same // resources and they may represent very distinct paths over which we can - // switch. If the |primier| connection is not connected, we may be + // switch. If the |premier| connection is not connected, we may be // reconnecting a TCP connection and temporarily do not prune connections in // this network. See the big comment in CompareConnections. @@ -1010,14 +1010,16 @@ void P2PTransportChannel::PruneConnections() { networks.insert(conn->port()->Network()); } for (rtc::Network* network : networks) { - Connection* primier = GetBestConnectionOnNetwork(network); - if (!(primier && primier->writable() && primier->connected())) { + Connection* premier = GetBestConnectionOnNetwork(network); + // Do not prune connections if the current best connection is weak on this + // network. Otherwise, it may delete connections prematurely. + if (!premier || premier->Weak()) { continue; } for (Connection* conn : connections_) { - if ((conn != primier) && (conn->port()->Network() == network) && - (CompareConnectionCandidates(primier, conn) >= 0)) { + if ((conn != premier) && (conn->port()->Network() == network) && + (CompareConnectionCandidates(premier, conn) >= 0)) { conn->Prune(); } } @@ -1092,8 +1094,7 @@ void P2PTransportChannel::HandleAllTimedOut() { } bool P2PTransportChannel::Weak() const { - return !(best_connection_ && best_connection_->receiving() && - best_connection_->writable()); + return !best_connection_ || best_connection_->Weak(); } // If we have a best connection, return it, otherwise return top one in the diff --git a/webrtc/p2p/base/p2ptransportchannel_unittest.cc b/webrtc/p2p/base/p2ptransportchannel_unittest.cc index fedd064b2b..2987ba5b23 100644 --- a/webrtc/p2p/base/p2ptransportchannel_unittest.cc +++ b/webrtc/p2p/base/p2ptransportchannel_unittest.cc @@ -1832,7 +1832,7 @@ TEST_F(P2PTransportChannelPingTest, TestSelectConnectionBeforeNomination) { // as the best connection. ch.AddRemoteCandidate(CreateCandidate("2.2.2.2", 2, 10)); cricket::Connection* conn2 = WaitForConnectionTo(&ch, "2.2.2.2", 2); - ASSERT_TRUE(conn1 != nullptr); + ASSERT_TRUE(conn2 != nullptr); EXPECT_EQ(conn2, ch.best_connection()); // If a stun request with use-candidate attribute arrives, the receiving @@ -1992,3 +1992,42 @@ TEST_F(P2PTransportChannelPingTest, TestSelectConnectionBasedOnMediaReceived) { conn2->OnReadPacket("XYZ", 3, rtc::CreatePacketTime(0)); EXPECT_EQ(conn3, ch.best_connection()); } + +// When the current best connection is strong, lower-priority connections will +// be pruned. Otherwise, lower-priority connections are kept. +TEST_F(P2PTransportChannelPingTest, TestDontPruneWhenWeak) { + cricket::FakePortAllocator pa(rtc::Thread::Current(), nullptr); + cricket::P2PTransportChannel ch("test channel", 1, nullptr, &pa); + PrepareChannel(&ch); + ch.SetIceRole(cricket::ICEROLE_CONTROLLED); + ch.Connect(); + ch.MaybeStartGathering(); + ch.AddRemoteCandidate(CreateCandidate("1.1.1.1", 1, 1)); + 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 + + // When a higher-priority, nominated candidate comes in, the connections with + // lower-priority are pruned. + ch.AddRemoteCandidate(CreateCandidate("2.2.2.2", 2, 10)); + cricket::Connection* conn2 = WaitForConnectionTo(&ch, "2.2.2.2", 2); + ASSERT_TRUE(conn2 != nullptr); + conn2->ReceivedPingResponse(); // Becomes writable and receiving + conn2->set_nominated(true); + conn2->SignalNominated(conn2); + EXPECT_TRUE_WAIT(conn1->pruned(), 3000); + + ch.SetReceivingTimeout(500); + // Wait until conn2 becomes not receiving. + EXPECT_TRUE_WAIT(!conn2->receiving(), 3000); + + 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()); +} diff --git a/webrtc/p2p/base/port.h b/webrtc/p2p/base/port.h index 8e7a2592d9..47903be05b 100644 --- a/webrtc/p2p/base/port.h +++ b/webrtc/p2p/base/port.h @@ -436,6 +436,8 @@ class Connection : public rtc::MessageHandler, // be false for TCP connections. bool connected() const { return connected_; } + bool Weak() const { return !(writable() && receiving() && connected()); } + // Estimate of the round-trip time over this connection. uint32 rtt() const { return rtt_; }