From 715d02334bc0ad1fb60da1f0ca920e658a83852b Mon Sep 17 00:00:00 2001 From: Jonas Oreland Date: Thu, 19 Nov 2020 15:46:50 +0100 Subject: [PATCH] Add feature to not discard candidates after connection is established In P2PTransportChannel::OnConnectionStateChange there is code that stop port allocation sessions if the modified connection is stronly connected. This means that local candidates are discarded (they are still gathered, only not surfaced). The implication of this is that if e.g doing a TURN allocation slower than P2P is established, the TURN allocation will not be added to list of local candidates => no TURN connection will be created. NOTE: If first connecting kRelay (only RELAY ONLY) then this patch does matter that much...until an ICE restart happens :) I discovered this when adding the emulated TURN server to tests, and being surprised that the TURN allocations never got used. These test does not (currently) use kRelay as start. Bug: webrtc:12210 Change-Id: I78a67201cf421b0e6fdd2ea684a00d740e063f5e Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/194141 Reviewed-by: Taylor Commit-Queue: Jonas Oreland Cr-Commit-Position: refs/heads/master@{#32647} --- p2p/base/p2p_transport_channel.cc | 17 ++- .../p2p_transport_channel_ice_field_trials.h | 3 + p2p/base/p2p_transport_channel_unittest.cc | 130 ++++++++++++++++++ 3 files changed, 144 insertions(+), 6 deletions(-) diff --git a/p2p/base/p2p_transport_channel.cc b/p2p/base/p2p_transport_channel.cc index 9bf0b23db6..998f4a9f6f 100644 --- a/p2p/base/p2p_transport_channel.cc +++ b/p2p/base/p2p_transport_channel.cc @@ -703,7 +703,10 @@ void P2PTransportChannel::SetIceConfig(const IceConfig& config) { "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) + "dead_connection_timeout_ms", &field_trials_.dead_connection_timeout_ms, + // Stop gathering on strongly connected. + "stop_gather_on_strongly_connected", + &field_trials_.stop_gather_on_strongly_connected) ->Parse(webrtc::field_trial::FindFullName("WebRTC-IceFieldTrials")); if (field_trials_.dead_connection_timeout_ms < 30000) { @@ -2015,11 +2018,13 @@ void P2PTransportChannel::OnConnectionStateChange(Connection* connection) { // the connection is at the latest generation. It is not enough to check // that the connection becomes weakly connected because the connection may be // changing from (writable, receiving) to (writable, not receiving). - bool strongly_connected = !connection->weak(); - bool latest_generation = connection->local_candidate().generation() >= - allocator_session()->generation(); - if (strongly_connected && latest_generation) { - MaybeStopPortAllocatorSessions(); + if (field_trials_.stop_gather_on_strongly_connected) { + bool strongly_connected = !connection->weak(); + bool latest_generation = connection->local_candidate().generation() >= + allocator_session()->generation(); + if (strongly_connected && latest_generation) { + MaybeStopPortAllocatorSessions(); + } } // We have to unroll the stack before doing this because we may be changing // the state of connections while sorting. diff --git a/p2p/base/p2p_transport_channel_ice_field_trials.h b/p2p/base/p2p_transport_channel_ice_field_trials.h index 00e1151ba3..82dc580c1e 100644 --- a/p2p/base/p2p_transport_channel_ice_field_trials.h +++ b/p2p/base/p2p_transport_channel_ice_field_trials.h @@ -58,6 +58,9 @@ struct IceFieldTrials { // The timeout after which the connection will be considered dead if no // traffic is received. int dead_connection_timeout_ms = 30000; + + // Stop gathering when having a strong connection. + bool stop_gather_on_strongly_connected = true; }; } // namespace cricket diff --git a/p2p/base/p2p_transport_channel_unittest.cc b/p2p/base/p2p_transport_channel_unittest.cc index 0e3a009bca..2af2a7412e 100644 --- a/p2p/base/p2p_transport_channel_unittest.cc +++ b/p2p/base/p2p_transport_channel_unittest.cc @@ -5978,4 +5978,134 @@ TEST_F(P2PTransportChannelTest, EnableDnsLookupsWithTransportPolicyNoHost) { DestroyChannels(); } +class GatherAfterConnectedTest : public P2PTransportChannelTest, + public ::testing::WithParamInterface {}; + +TEST_P(GatherAfterConnectedTest, GatherAfterConnected) { + const bool stop_gather_on_strongly_connected = GetParam(); + const std::string field_trial = + std::string("WebRTC-IceFieldTrials/stop_gather_on_strongly_connected:") + + (stop_gather_on_strongly_connected ? "true/" : "false/"); + webrtc::test::ScopedFieldTrials field_trials(field_trial); + + rtc::ScopedFakeClock clock; + // Use local + relay + constexpr uint32_t flags = + kDefaultPortAllocatorFlags | PORTALLOCATOR_ENABLE_SHARED_SOCKET | + PORTALLOCATOR_DISABLE_STUN | PORTALLOCATOR_DISABLE_TCP; + ConfigureEndpoints(OPEN, OPEN, flags, flags); + auto* ep1 = GetEndpoint(0); + auto* ep2 = GetEndpoint(1); + ep1->allocator_->SetCandidateFilter(CF_ALL); + ep2->allocator_->SetCandidateFilter(CF_ALL); + + // Use step delay 3s which is long enough for + // connection to be established before managing to gather relay candidates. + int delay = 3000; + SetAllocationStepDelay(0, delay); + SetAllocationStepDelay(1, delay); + IceConfig ice_config = CreateIceConfig(1000, GATHER_CONTINUALLY); + CreateChannels(ice_config, ice_config); + + PauseCandidates(0); + PauseCandidates(1); + + // We have gathered host candidates but not relay. + ASSERT_TRUE_SIMULATED_WAIT(ep1->saved_candidates_.size() == 1u && + ep2->saved_candidates_.size() == 1u, + kDefaultTimeout, clock); + + ResumeCandidates(0); + ResumeCandidates(1); + + PauseCandidates(0); + PauseCandidates(1); + + ASSERT_TRUE_SIMULATED_WAIT(ep1_ch1()->remote_candidates().size() == 1 && + ep2_ch1()->remote_candidates().size() == 1, + kDefaultTimeout, clock); + + ASSERT_TRUE_SIMULATED_WAIT( + ep1_ch1()->selected_connection() && ep2_ch1()->selected_connection(), + kDefaultTimeout, clock); + + clock.AdvanceTime(webrtc::TimeDelta::Millis(10 * delay)); + + if (stop_gather_on_strongly_connected) { + // The relay candiates gathered has not been propagated to channel. + EXPECT_EQ(ep1->saved_candidates_.size(), 0u); + EXPECT_EQ(ep2->saved_candidates_.size(), 0u); + } else { + // The relay candiates gathered has been propagated to channel. + EXPECT_EQ(ep1->saved_candidates_.size(), 1u); + EXPECT_EQ(ep2->saved_candidates_.size(), 1u); + } +} + +TEST_P(GatherAfterConnectedTest, GatherAfterConnectedMultiHomed) { + const bool stop_gather_on_strongly_connected = GetParam(); + const std::string field_trial = + std::string("WebRTC-IceFieldTrials/stop_gather_on_strongly_connected:") + + (stop_gather_on_strongly_connected ? "true/" : "false/"); + webrtc::test::ScopedFieldTrials field_trials(field_trial); + + rtc::ScopedFakeClock clock; + // Use local + relay + constexpr uint32_t flags = + kDefaultPortAllocatorFlags | PORTALLOCATOR_ENABLE_SHARED_SOCKET | + PORTALLOCATOR_DISABLE_STUN | PORTALLOCATOR_DISABLE_TCP; + AddAddress(0, kAlternateAddrs[0]); + ConfigureEndpoints(OPEN, OPEN, flags, flags); + auto* ep1 = GetEndpoint(0); + auto* ep2 = GetEndpoint(1); + ep1->allocator_->SetCandidateFilter(CF_ALL); + ep2->allocator_->SetCandidateFilter(CF_ALL); + + // Use step delay 3s which is long enough for + // connection to be established before managing to gather relay candidates. + int delay = 3000; + SetAllocationStepDelay(0, delay); + SetAllocationStepDelay(1, delay); + IceConfig ice_config = CreateIceConfig(1000, GATHER_CONTINUALLY); + CreateChannels(ice_config, ice_config); + + PauseCandidates(0); + PauseCandidates(1); + + // We have gathered host candidates but not relay. + ASSERT_TRUE_SIMULATED_WAIT(ep1->saved_candidates_.size() == 2u && + ep2->saved_candidates_.size() == 1u, + kDefaultTimeout, clock); + + ResumeCandidates(0); + ResumeCandidates(1); + + PauseCandidates(0); + PauseCandidates(1); + + ASSERT_TRUE_SIMULATED_WAIT(ep1_ch1()->remote_candidates().size() == 1 && + ep2_ch1()->remote_candidates().size() == 2, + kDefaultTimeout, clock); + + ASSERT_TRUE_SIMULATED_WAIT( + ep1_ch1()->selected_connection() && ep2_ch1()->selected_connection(), + kDefaultTimeout, clock); + + clock.AdvanceTime(webrtc::TimeDelta::Millis(10 * delay)); + + if (stop_gather_on_strongly_connected) { + // The relay candiates gathered has not been propagated to channel. + EXPECT_EQ(ep1->saved_candidates_.size(), 0u); + EXPECT_EQ(ep2->saved_candidates_.size(), 0u); + } else { + // The relay candiates gathered has been propagated. + EXPECT_EQ(ep1->saved_candidates_.size(), 2u); + EXPECT_EQ(ep2->saved_candidates_.size(), 1u); + } +} + +INSTANTIATE_TEST_SUITE_P(GatherAfterConnectedTest, + GatherAfterConnectedTest, + ::testing::Values(true, false)); + } // namespace cricket