diff --git a/p2p/base/connection.cc b/p2p/base/connection.cc index a9d570652a..afb1457567 100644 --- a/p2p/base/connection.cc +++ b/p2p/base/connection.cc @@ -918,12 +918,31 @@ void Connection::ReceivedPingResponse( bool Connection::dead(int64_t now) const { if (last_received() > 0) { - // If it has ever received anything, we keep it alive until it hasn't - // received anything for DEAD_CONNECTION_RECEIVE_TIMEOUT. This covers the - // normal case of a successfully used connection that stops working. This - // also allows a remote peer to continue pinging over a locally inactive - // (pruned) connection. - return (now > (last_received() + DEAD_CONNECTION_RECEIVE_TIMEOUT)); + // If it has ever received anything, we keep it alive + // - if it has recevied last DEAD_CONNECTION_RECEIVE_TIMEOUT (30s) + // - if it has a ping outstanding shorter than + // DEAD_CONNECTION_RECEIVE_TIMEOUT (30s) + // - else if IDLE let it live field_trials_->dead_connection_timeout_ms + // + // This covers the normal case of a successfully used connection that stops + // working. This also allows a remote peer to continue pinging over a + // locally inactive (pruned) connection. This also allows the local agent to + // ping with longer interval than 30s as long as it shorter than + // |dead_connection_timeout_ms|. + if (now <= (last_received() + DEAD_CONNECTION_RECEIVE_TIMEOUT)) { + // Not dead since we have received the last 30s. + return false; + } + if (!pings_since_last_response_.empty()) { + // Outstanding pings: let it live until the ping is unreplied for + // DEAD_CONNECTION_RECEIVE_TIMEOUT. + return now > (pings_since_last_response_[0].sent_time + + DEAD_CONNECTION_RECEIVE_TIMEOUT); + } + + // No outstanding pings: let it live until + // field_trials_->dead_connection_timeout_ms has passed. + return now > (last_received() + field_trials_->dead_connection_timeout_ms); } if (active()) { diff --git a/p2p/base/p2p_transport_channel.cc b/p2p/base/p2p_transport_channel.cc index 33325693b9..73d12c7741 100644 --- a/p2p/base/p2p_transport_channel.cc +++ b/p2p/base/p2p_transport_channel.cc @@ -694,9 +694,18 @@ void P2PTransportChannel::SetIceConfig(const IceConfig& config) { &field_trials_.send_ping_on_switch_ice_controlling, // Reply to nomination ASAP. "send_ping_on_nomination_ice_controlled", - &field_trials_.send_ping_on_nomination_ice_controlled) + &field_trials_.send_ping_on_nomination_ice_controlled, + // Allow connections to live untouched longer that 30s. + "dead_connection_timeout_ms", &field_trials_.dead_connection_timeout_ms) ->Parse(webrtc::field_trial::FindFullName("WebRTC-IceFieldTrials")); + if (field_trials_.dead_connection_timeout_ms < 30000) { + RTC_LOG(LS_WARNING) << "dead_connection_timeout_ms set to " + << field_trials_.dead_connection_timeout_ms + << " increasing it to 30000"; + field_trials_.dead_connection_timeout_ms = 30000; + } + if (field_trials_.skip_relay_to_non_relay_connections) { RTC_LOG(LS_INFO) << "Set skip_relay_to_non_relay_connections"; } diff --git a/p2p/base/p2p_transport_channel_ice_field_trials.h b/p2p/base/p2p_transport_channel_ice_field_trials.h index 8b208e339e..f30366fd1f 100644 --- a/p2p/base/p2p_transport_channel_ice_field_trials.h +++ b/p2p/base/p2p_transport_channel_ice_field_trials.h @@ -48,6 +48,10 @@ struct IceFieldTrials { // Sending a PING directly after a nomination on ICE_CONTROLLED-side. bool send_ping_on_nomination_ice_controlled = false; + + // The timeout after which the connection will be considered dead if no + // traffic is received. + int dead_connection_timeout_ms = 30000; }; } // namespace cricket diff --git a/p2p/base/port_unittest.cc b/p2p/base/port_unittest.cc index a7ac1fafdb..7703a9c281 100644 --- a/p2p/base/port_unittest.cc +++ b/p2p/base/port_unittest.cc @@ -64,6 +64,7 @@ #include "rtc_base/thread.h" #include "rtc_base/time_utils.h" #include "rtc_base/virtual_socket_server.h" +#include "test/field_trial.h" #include "test/gtest.h" using rtc::AsyncPacketSocket; @@ -1298,6 +1299,77 @@ TEST_F(PortTest, TestConnectionDead) { EXPECT_TRUE_WAIT(ch1.conn() == nullptr, kDefaultTimeout); } +TEST_F(PortTest, TestConnectionDeadWithDeadConnectionTimeout) { + TestChannel ch1(CreateUdpPort(kLocalAddr1)); + TestChannel ch2(CreateUdpPort(kLocalAddr2)); + // Acquire address. + ch1.Start(); + ch2.Start(); + ASSERT_EQ_WAIT(1, ch1.complete_count(), kDefaultTimeout); + ASSERT_EQ_WAIT(1, ch2.complete_count(), kDefaultTimeout); + + // Note: set field trials manually since they are parsed by + // P2PTransportChannel but P2PTransportChannel is not used in this test. + IceFieldTrials field_trials; + field_trials.dead_connection_timeout_ms = 90000; + + // Create a connection again and receive a ping. + ch1.CreateConnection(GetCandidate(ch2.port())); + auto conn = ch1.conn(); + conn->SetIceFieldTrials(&field_trials); + + ASSERT_NE(conn, nullptr); + int64_t before_last_receiving = rtc::TimeMillis(); + conn->ReceivedPing(); + int64_t after_last_receiving = rtc::TimeMillis(); + // The connection will be dead after 90s + conn->UpdateState(before_last_receiving + 90000 - 1); + rtc::Thread::Current()->ProcessMessages(100); + EXPECT_TRUE(ch1.conn() != nullptr); + conn->UpdateState(after_last_receiving + 90000 + 1); + EXPECT_TRUE_WAIT(ch1.conn() == nullptr, kDefaultTimeout); +} + +TEST_F(PortTest, TestConnectionDeadOutstandingPing) { + auto port1 = CreateUdpPort(kLocalAddr1); + port1->SetIceRole(cricket::ICEROLE_CONTROLLING); + port1->SetIceTiebreaker(kTiebreaker1); + auto port2 = CreateUdpPort(kLocalAddr2); + port2->SetIceRole(cricket::ICEROLE_CONTROLLED); + port2->SetIceTiebreaker(kTiebreaker2); + + TestChannel ch1(std::move(port1)); + TestChannel ch2(std::move(port2)); + // Acquire address. + ch1.Start(); + ch2.Start(); + ASSERT_EQ_WAIT(1, ch1.complete_count(), kDefaultTimeout); + ASSERT_EQ_WAIT(1, ch2.complete_count(), kDefaultTimeout); + + // Note: set field trials manually since they are parsed by + // P2PTransportChannel but P2PTransportChannel is not used in this test. + IceFieldTrials field_trials; + field_trials.dead_connection_timeout_ms = 360000; + + // Create a connection again and receive a ping and then send + // a ping and keep it outstanding. + ch1.CreateConnection(GetCandidate(ch2.port())); + auto conn = ch1.conn(); + conn->SetIceFieldTrials(&field_trials); + + ASSERT_NE(conn, nullptr); + conn->ReceivedPing(); + int64_t send_ping_timestamp = rtc::TimeMillis(); + conn->Ping(send_ping_timestamp); + + // The connection will be dead 30s after the ping was sent. + conn->UpdateState(send_ping_timestamp + DEAD_CONNECTION_RECEIVE_TIMEOUT - 1); + rtc::Thread::Current()->ProcessMessages(100); + EXPECT_TRUE(ch1.conn() != nullptr); + conn->UpdateState(send_ping_timestamp + DEAD_CONNECTION_RECEIVE_TIMEOUT + 1); + EXPECT_TRUE_WAIT(ch1.conn() == nullptr, kDefaultTimeout); +} + // This test case verifies standard ICE features in STUN messages. Currently it // verifies Message Integrity attribute in STUN messages and username in STUN // binding request will have colon (":") between remote and local username.