From fd16da290ce90cb0810890a8b7ffcee98cdb5c3c Mon Sep 17 00:00:00 2001 From: Honghai Zhang Date: Wed, 17 Aug 2016 16:12:46 -0700 Subject: [PATCH] Do not switch to a high-cost connection that is not receiving. This prevents connection switching due to remote-side network down. R=deadbeef@webrtc.org, pthatcher@webrtc.org Review URL: https://codereview.webrtc.org/2232563002 . Cr-Commit-Position: refs/heads/master@{#13807} --- webrtc/p2p/base/p2ptransportchannel.cc | 13 +++- .../p2p/base/p2ptransportchannel_unittest.cc | 65 +++++++++++++++++-- webrtc/p2p/base/port.h | 2 + 3 files changed, 75 insertions(+), 5 deletions(-) diff --git a/webrtc/p2p/base/p2ptransportchannel.cc b/webrtc/p2p/base/p2ptransportchannel.cc index ef17620b25..ebe59bfb95 100644 --- a/webrtc/p2p/base/p2ptransportchannel.cc +++ b/webrtc/p2p/base/p2ptransportchannel.cc @@ -186,6 +186,8 @@ void P2PTransportChannel::AddConnection(Connection* connection) { // b) last data received time. // iii) Lower cost / higher priority. // iv) rtt. +// To further prevent switching to high-cost networks, does not switch to +// a high-cost connection if it is not receiving. // TODO(honghaiz): Stop the aggressive nomination on the controlling side and // implement the ice-renomination option. bool P2PTransportChannel::ShouldSwitchSelectedConnection( @@ -199,6 +201,14 @@ bool P2PTransportChannel::ShouldSwitchSelectedConnection( return true; } + // Do not switch to a connection that is not receiving if it has higher cost + // because it may be just spuriously better. + if (new_connection->ComputeNetworkCost() > + selected_connection_->ComputeNetworkCost() && + !new_connection->receiving()) { + return false; + } + rtc::Optional receiving_unchanged_threshold( rtc::TimeMillis() - config_.receiving_switching_delay.value_or(0)); int cmp = CompareConnections(selected_connection_, new_connection, @@ -1525,7 +1535,8 @@ bool P2PTransportChannel::IsPingable(const Connection* conn, // Always ping active connections regardless whether the channel is completed // or not, but backup connections are pinged at a slower rate. if (IsBackupConnection(conn)) { - return (now >= conn->last_ping_response_received() + + return conn->rtt_samples() == 0 || + (now >= conn->last_ping_response_received() + config_.backup_connection_ping_interval); } // Don't ping inactive non-backup connections. diff --git a/webrtc/p2p/base/p2ptransportchannel_unittest.cc b/webrtc/p2p/base/p2ptransportchannel_unittest.cc index 9f6b3c96b1..9342a201a1 100644 --- a/webrtc/p2p/base/p2ptransportchannel_unittest.cc +++ b/webrtc/p2p/base/p2ptransportchannel_unittest.cc @@ -54,8 +54,8 @@ static const SocketAddress kIPv6PublicAddrs[2] = { SocketAddress("2620:0:1000:1b03:2e41:38ff:fea6:f2a4", 0) }; // For configuring multihomed clients. -static const SocketAddress kAlternateAddrs[2] = - { SocketAddress("11.11.11.101", 0), SocketAddress("22.22.22.202", 0) }; +static const SocketAddress kAlternateAddrs[2] = { + SocketAddress("101.101.101.101", 0), SocketAddress("202.202.202.202", 0)}; // Addresses for HTTP proxy servers. static const SocketAddress kHttpsProxyAddrs[2] = { SocketAddress("11.11.11.1", 443), SocketAddress("22.22.22.1", 443) }; @@ -673,7 +673,11 @@ class P2PTransportChannelTestBase : public testing::Test, CandidatePairInterface* selected_candidate_pair, int last_sent_packet_id, bool ready_to_send) { - ++selected_candidate_pair_switches_; + // Do not count if it switches to nullptr. This may happen if all + // connections timed out. + if (selected_candidate_pair != nullptr) { + ++selected_candidate_pair_switches_; + } } int reset_selected_candidate_pair_switches() { @@ -1859,7 +1863,7 @@ class P2PTransportChannelMultihomedTest : public P2PTransportChannelTestBase { return nullptr; } - const cricket::Connection* GetConnectionWithLocalAddress( + cricket::Connection* GetConnectionWithLocalAddress( cricket::P2PTransportChannel* channel, const SocketAddress& address) { for (cricket::Connection* conn : channel->connections()) { @@ -2179,6 +2183,59 @@ TEST_F(P2PTransportChannelMultihomedTest, DestroyChannels(); } +// Tests that if the remote side's network failed, it won't cause the local +// side to switch connections and networks. +TEST_F(P2PTransportChannelMultihomedTest, TestRemoteFailover) { + rtc::ScopedFakeClock clock; + // The interface names are chosen so that |cellular| would have higher + // candidate priority and higher cost. + auto& wifi = kPublicAddrs; + auto& cellular = kAlternateAddrs; + AddAddress(0, wifi[0], "wifi0", rtc::ADAPTER_TYPE_WIFI); + AddAddress(0, cellular[0], "cellular0", rtc::ADAPTER_TYPE_CELLULAR); + AddAddress(1, wifi[1], "wifi0", rtc::ADAPTER_TYPE_WIFI); + + // Use only local ports for simplicity. + SetAllocatorFlags(0, kOnlyLocalPorts); + SetAllocatorFlags(1, kOnlyLocalPorts); + // Create channels and let them go writable, as usual. + CreateChannels(1); + // Make the receiving timeout shorter for testing. + // Set the backup connection ping interval to 25s. + IceConfig config = CreateIceConfig(1000, GATHER_ONCE, 25000); + // Ping the best connection more frequently since we don't have traffic. + config.stable_writable_connection_ping_interval = 900; + ep1_ch1()->SetIceConfig(config); + ep2_ch1()->SetIceConfig(config); + // Need to wait to make sure the connections on both networks are writable. + EXPECT_TRUE_SIMULATED_WAIT(ep1_ch1()->receiving() && ep1_ch1()->writable() && + ep2_ch1()->receiving() && + ep2_ch1()->writable(), + 3000, clock); + EXPECT_TRUE_SIMULATED_WAIT( + ep1_ch1()->selected_connection() && + LocalCandidate(ep1_ch1())->address().EqualIPs(wifi[0]) && + RemoteCandidate(ep1_ch1())->address().EqualIPs(wifi[1]), + kDefaultTimeout, clock); + Connection* backup_conn = + GetConnectionWithLocalAddress(ep1_ch1(), cellular[0]); + ASSERT_NE(nullptr, backup_conn); + // After a short while, the backup connection will be writable but not + // receiving because backup connection is pinged at a slower rate. + EXPECT_TRUE_SIMULATED_WAIT( + backup_conn->writable() && !backup_conn->receiving(), 5000, clock); + reset_selected_candidate_pair_switches(); + // Blackhole any traffic to or from the remote WiFi networks. + LOG(LS_INFO) << "Failing over..."; + fw()->AddRule(false, rtc::FP_ANY, rtc::FD_ANY, wifi[1]); + + int num_switches = 0; + SIMULATED_WAIT((num_switches = reset_selected_candidate_pair_switches()) > 0, + 20000, clock); + EXPECT_EQ(0, num_switches); + DestroyChannels(); +} + // Tests that a Wifi-Wifi connection has the highest precedence. TEST_F(P2PTransportChannelMultihomedTest, TestPreferWifiToWifiConnection) { // The interface names are chosen so that |cellular| would have higher diff --git a/webrtc/p2p/base/port.h b/webrtc/p2p/base/port.h index f94972fb9d..2dc9deaddb 100644 --- a/webrtc/p2p/base/port.h +++ b/webrtc/p2p/base/port.h @@ -553,6 +553,8 @@ class Connection : public CandidatePairInterface, int64_t last_ping_response_received() const { return last_ping_response_received_; } + // Used to check if any STUN ping response has been received. + int rtt_samples() const { return rtt_samples_; } // Called whenever a valid ping is received on this connection. This is // public because the connection intercepts the first ping for us.