From b60a8198f1bca0f843743837e295f780c721f712 Mon Sep 17 00:00:00 2001 From: deadbeef Date: Wed, 24 Aug 2016 15:15:00 -0700 Subject: [PATCH] Fixing inconsistency with behavior of `ClearGettingPorts`. I found that, depending on when it's called, ClearGettingPorts may or may not signal CandidatesAllocationDone, and may or may not continue to gather more ports/candidates. I'm fixing this inconsistency by having it always signal CandidatesAllocationDone (if needed), and always stop gathering until the next network change event. This makes it equivalent to StopGettingPorts, except that it allows gathering to be restarted if a network change occurs. I also found that P2PTransportChannel was signaling "gathering complete" even when continual gathering was enabled. This wasn't caught by the unit tests due to the inconsistency of ClearGettingPorts as described above. Review-Url: https://codereview.webrtc.org/2124283003 Cr-Commit-Position: refs/heads/master@{#13908} --- webrtc/api/webrtcsession_unittest.cc | 11 +-- webrtc/p2p/base/p2ptransportchannel.cc | 18 ++++- .../p2p/base/p2ptransportchannel_unittest.cc | 78 +++++++++---------- webrtc/p2p/base/portallocator.h | 16 +++- webrtc/p2p/client/basicportallocator.cc | 2 +- .../p2p/client/basicportallocator_unittest.cc | 6 +- 6 files changed, 77 insertions(+), 54 deletions(-) diff --git a/webrtc/api/webrtcsession_unittest.cc b/webrtc/api/webrtcsession_unittest.cc index c0d8a3bbdf..6c0170e704 100644 --- a/webrtc/api/webrtcsession_unittest.cc +++ b/webrtc/api/webrtcsession_unittest.cc @@ -2258,6 +2258,10 @@ TEST_F(WebRtcSessionTest, TestLocalCandidatesAddedAndRemovedIfGatherContinually) { AddInterface(rtc::SocketAddress(kClientAddrHost1, kClientAddrPort)); Init(); + // Enable Continual Gathering. + cricket::IceConfig config; + config.continual_gathering_policy = cricket::GATHER_CONTINUALLY; + session_->SetIceConfig(config); SendAudioVideoStream1(); CreateAndSetRemoteOfferAndLocalAnswer(); @@ -2267,7 +2271,8 @@ TEST_F(WebRtcSessionTest, ASSERT_TRUE(candidates != NULL); EXPECT_EQ(0u, candidates->count()); - EXPECT_TRUE_WAIT(observer_.oncandidatesready_, kIceCandidatesTimeout); + // Since we're using continual gathering, we won't get "gathering done". + EXPECT_EQ_WAIT(2u, candidates->count(), kIceCandidatesTimeout); local_desc = session_->local_description(); candidates = local_desc->candidates(kMediaContentIndex0); @@ -2291,10 +2296,6 @@ TEST_F(WebRtcSessionTest, candidates = local_desc->candidates(kMediaContentIndex0); size_t num_local_candidates = candidates->count(); - // Enable Continual Gathering - cricket::IceConfig config; - config.continual_gathering_policy = cricket::GATHER_CONTINUALLY; - session_->SetIceConfig(config); // Bring down the network interface to trigger candidate removals. RemoveInterface(rtc::SocketAddress(kClientAddrHost1, kClientAddrPort)); // Verify that all local candidates are removed. diff --git a/webrtc/p2p/base/p2ptransportchannel.cc b/webrtc/p2p/base/p2ptransportchannel.cc index d4d0e5e327..03b0ed426a 100644 --- a/webrtc/p2p/base/p2ptransportchannel.cc +++ b/webrtc/p2p/base/p2ptransportchannel.cc @@ -351,9 +351,14 @@ void P2PTransportChannel::SetRemoteIceMode(IceMode mode) { void P2PTransportChannel::SetIceConfig(const IceConfig& config) { if (config_.continual_gathering_policy != config.continual_gathering_policy) { - config_.continual_gathering_policy = config.continual_gathering_policy; - LOG(LS_INFO) << "Set continual_gathering_policy to " - << config_.continual_gathering_policy; + if (!allocator_sessions_.empty()) { + LOG(LS_ERROR) << "Trying to change continual gathering policy " + << "when gathering has already started!"; + } else { + config_.continual_gathering_policy = config.continual_gathering_policy; + LOG(LS_INFO) << "Set continual_gathering_policy to " + << config_.continual_gathering_policy; + } } if (config.backup_connection_ping_interval >= 0 && @@ -524,6 +529,13 @@ void P2PTransportChannel::OnCandidatesReady( void P2PTransportChannel::OnCandidatesAllocationDone( PortAllocatorSession* session) { ASSERT(worker_thread_ == rtc::Thread::Current()); + if (config_.gather_continually()) { + LOG(LS_INFO) << "P2PTransportChannel: " << transport_name() + << ", component " << component() + << " gathering complete, but using continual " + << "gathering so not changing gathering state."; + return; + } gathering_state_ = kIceGatheringComplete; LOG(LS_INFO) << "P2PTransportChannel: " << transport_name() << ", component " << component() << " gathering complete"; diff --git a/webrtc/p2p/base/p2ptransportchannel_unittest.cc b/webrtc/p2p/base/p2ptransportchannel_unittest.cc index 9342a201a1..26db621a9d 100644 --- a/webrtc/p2p/base/p2ptransportchannel_unittest.cc +++ b/webrtc/p2p/base/p2ptransportchannel_unittest.cc @@ -305,7 +305,9 @@ class P2PTransportChannelTestBase : public testing::Test, return ep2_.GetChannelData(channel); } - void CreateChannels(int num) { + void CreateChannels(int num, + const IceConfig& ep1_config, + const IceConfig& ep2_config) { std::string ice_ufrag_ep1_cd1_ch = kIceUfrag[0]; std::string ice_pwd_ep1_cd1_ch = kIcePwd[0]; std::string ice_ufrag_ep2_cd1_ch = kIceUfrag[1]; @@ -316,6 +318,8 @@ class P2PTransportChannelTestBase : public testing::Test, ep2_.cd1_.ch_.reset(CreateChannel( 1, ICE_CANDIDATE_COMPONENT_DEFAULT, ice_ufrag_ep2_cd1_ch, ice_pwd_ep2_cd1_ch, ice_ufrag_ep1_cd1_ch, ice_pwd_ep1_cd1_ch)); + ep1_.cd1_.ch_->SetIceConfig(ep1_config); + ep2_.cd1_.ch_->SetIceConfig(ep2_config); ep1_.cd1_.ch_->MaybeStartGathering(); ep2_.cd1_.ch_->MaybeStartGathering(); if (num == 2) { @@ -329,10 +333,18 @@ class P2PTransportChannelTestBase : public testing::Test, ep2_.cd2_.ch_.reset(CreateChannel( 1, ICE_CANDIDATE_COMPONENT_DEFAULT, ice_ufrag_ep2_cd2_ch, ice_pwd_ep2_cd2_ch, ice_ufrag_ep1_cd2_ch, ice_pwd_ep1_cd2_ch)); + ep1_.cd2_.ch_->SetIceConfig(ep1_config); + ep2_.cd2_.ch_->SetIceConfig(ep2_config); ep1_.cd2_.ch_->MaybeStartGathering(); ep2_.cd2_.ch_->MaybeStartGathering(); } } + + void CreateChannels(int num) { + IceConfig default_config; + CreateChannels(num, default_config, default_config); + } + P2PTransportChannel* CreateChannel(int endpoint, int component, const std::string& local_ice_ufrag, @@ -1573,10 +1585,11 @@ TEST_F(P2PTransportChannelTest, TestContinualGathering) { kDefaultPortAllocatorFlags); SetAllocationStepDelay(0, kDefaultStepDelay); SetAllocationStepDelay(1, kDefaultStepDelay); - CreateChannels(1); - IceConfig config = CreateIceConfig(1000, GATHER_CONTINUALLY); - ep1_ch1()->SetIceConfig(config); + IceConfig continual_gathering_config = + CreateIceConfig(1000, GATHER_CONTINUALLY); // By default, ep2 does not gather continually. + IceConfig default_config; + CreateChannels(1, continual_gathering_config, default_config); EXPECT_TRUE_WAIT_MARGIN(ep1_ch1() != NULL && ep2_ch1() != NULL && ep1_ch1()->receiving() && ep1_ch1()->writable() && @@ -1908,8 +1921,10 @@ TEST_F(P2PTransportChannelMultihomedTest, TestFailoverControlledSide) { SetAllocatorFlags(0, kOnlyLocalPorts); SetAllocatorFlags(1, kOnlyLocalPorts); + // Make the receiving timeout shorter for testing. + IceConfig config = CreateIceConfig(1000, GATHER_ONCE); // Create channels and let them go writable, as usual. - CreateChannels(1); + CreateChannels(1, config, config); EXPECT_TRUE_SIMULATED_WAIT(ep1_ch1()->receiving() && ep1_ch1()->writable() && ep2_ch1()->receiving() && @@ -1920,11 +1935,6 @@ TEST_F(P2PTransportChannelMultihomedTest, TestFailoverControlledSide) { LocalCandidate(ep1_ch1())->address().EqualIPs(kPublicAddrs[0]) && RemoteCandidate(ep1_ch1())->address().EqualIPs(kPublicAddrs[1])); - // Make the receiving timeout shorter for testing. - IceConfig config = CreateIceConfig(1000, GATHER_ONCE); - ep1_ch1()->SetIceConfig(config); - ep2_ch1()->SetIceConfig(config); - // Blackhole any traffic to or from the public addrs. LOG(LS_INFO) << "Failing over..."; fw()->AddRule(false, rtc::FP_ANY, rtc::FD_ANY, kPublicAddrs[1]); @@ -1964,8 +1974,10 @@ TEST_F(P2PTransportChannelMultihomedTest, TestFailoverControllingSide) { SetAllocatorFlags(0, kOnlyLocalPorts); SetAllocatorFlags(1, kOnlyLocalPorts); + // Make the receiving timeout shorter for testing. + IceConfig config = CreateIceConfig(1000, GATHER_ONCE); // Create channels and let them go writable, as usual. - CreateChannels(1); + CreateChannels(1, config, config); EXPECT_TRUE_SIMULATED_WAIT(ep1_ch1()->receiving() && ep1_ch1()->writable() && ep2_ch1()->receiving() && ep2_ch1()->writable(), @@ -1975,11 +1987,6 @@ TEST_F(P2PTransportChannelMultihomedTest, TestFailoverControllingSide) { LocalCandidate(ep1_ch1())->address().EqualIPs(kPublicAddrs[0]) && RemoteCandidate(ep1_ch1())->address().EqualIPs(kPublicAddrs[1])); - // Make the receiving timeout shorter for testing. - IceConfig config = CreateIceConfig(1000, GATHER_ONCE); - ep1_ch1()->SetIceConfig(config); - ep2_ch1()->SetIceConfig(config); - // Blackhole any traffic to or from the public addrs. LOG(LS_INFO) << "Failing over..."; fw()->AddRule(false, rtc::FP_ANY, rtc::FD_ANY, kPublicAddrs[0]); @@ -2020,8 +2027,10 @@ TEST_F(P2PTransportChannelMultihomedTest, TestIceRenomination) { SetAllocatorFlags(0, kOnlyLocalPorts); SetAllocatorFlags(1, kOnlyLocalPorts); + // Make the receiving timeout shorter for testing. + IceConfig config = CreateIceConfig(1000, GATHER_ONCE); // Create channels and let them go writable, as usual. - CreateChannels(1); + CreateChannels(1, config, config); ep1_ch1()->set_remote_supports_renomination(true); EXPECT_TRUE_SIMULATED_WAIT(ep1_ch1()->receiving() && ep1_ch1()->writable() && ep2_ch1()->receiving() && @@ -2041,11 +2050,6 @@ TEST_F(P2PTransportChannelMultihomedTest, TestIceRenomination) { SIMULATED_WAIT(nominated(), 3000, clock); EXPECT_FALSE(nominated()); - // Make the receiving timeout shorter for testing. - IceConfig config = CreateIceConfig(1000, GATHER_ONCE); - ep1_ch1()->SetIceConfig(config); - ep2_ch1()->SetIceConfig(config); - // Blackhole any traffic to or from the public addrs. LOG(LS_INFO) << "Failing over..."; fw()->AddRule(false, rtc::FP_ANY, rtc::FD_ANY, kPublicAddrs[0]); @@ -2362,9 +2366,9 @@ TEST_F(P2PTransportChannelMultihomedTest, TestNetworkBecomesInactive) { AddAddress(0, kPublicAddrs[0]); AddAddress(1, kPublicAddrs[1]); // Create channels and let them go writable, as usual. - CreateChannels(1); - ep1_ch1()->SetIceConfig(CreateIceConfig(2000, GATHER_CONTINUALLY)); - ep2_ch1()->SetIceConfig(CreateIceConfig(2000, GATHER_ONCE)); + IceConfig ep1_config = CreateIceConfig(2000, GATHER_CONTINUALLY); + IceConfig ep2_config = CreateIceConfig(2000, GATHER_ONCE); + CreateChannels(1, ep1_config, ep2_config); SetAllocatorFlags(0, kOnlyLocalPorts); SetAllocatorFlags(1, kOnlyLocalPorts); @@ -2406,10 +2410,10 @@ TEST_F(P2PTransportChannelMultihomedTest, auto& cellular = kPublicAddrs; AddAddress(0, wifi[0], "test_wifi0", rtc::ADAPTER_TYPE_WIFI); AddAddress(1, cellular[1], "test_cell1", rtc::ADAPTER_TYPE_CELLULAR); - CreateChannels(1); // Set continual gathering policy. - ep1_ch1()->SetIceConfig(CreateIceConfig(1000, GATHER_CONTINUALLY)); - ep2_ch1()->SetIceConfig(CreateIceConfig(1000, GATHER_CONTINUALLY)); + IceConfig continual_gathering_config = + CreateIceConfig(1000, GATHER_CONTINUALLY); + CreateChannels(1, continual_gathering_config, continual_gathering_config); SetAllocatorFlags(0, kOnlyLocalPorts); SetAllocatorFlags(1, kOnlyLocalPorts); EXPECT_TRUE_WAIT_MARGIN(ep1_ch1()->receiving() && ep1_ch1()->writable() && @@ -2456,11 +2460,11 @@ TEST_F(P2PTransportChannelMultihomedTest, SetAllocatorFlags(0, kOnlyLocalPorts); SetAllocatorFlags(1, kOnlyLocalPorts); - // Create channels and let them go writable, as usual. - CreateChannels(1); // Set continual gathering policy. - ep1_ch1()->SetIceConfig(CreateIceConfig(1000, GATHER_CONTINUALLY)); - ep2_ch1()->SetIceConfig(CreateIceConfig(1000, GATHER_CONTINUALLY)); + IceConfig continual_gathering_config = + CreateIceConfig(1000, GATHER_CONTINUALLY); + // Create channels and let them go writable, as usual. + CreateChannels(1, continual_gathering_config, continual_gathering_config); EXPECT_TRUE_SIMULATED_WAIT(ep1_ch1()->receiving() && ep1_ch1()->writable() && ep2_ch1()->receiving() && ep2_ch1()->writable(), @@ -2514,12 +2518,10 @@ TEST_F(P2PTransportChannelMultihomedTest, SetAllocatorFlags(0, kOnlyLocalPorts); SetAllocatorFlags(1, kOnlyLocalPorts); - // Create channels and let them go writable, as usual. - CreateChannels(1); // Set continual gathering policy. IceConfig config = CreateIceConfig(1000, GATHER_CONTINUALLY_AND_RECOVER); - ep1_ch1()->SetIceConfig(config); - ep2_ch1()->SetIceConfig(config); + // Create channels and let them go writable, as usual. + CreateChannels(1, config, config); EXPECT_TRUE_SIMULATED_WAIT(ep1_ch1()->receiving() && ep1_ch1()->writable() && ep2_ch1()->receiving() && ep2_ch1()->writable(), 3000, clock); @@ -2558,11 +2560,9 @@ TEST_F(P2PTransportChannelMultihomedTest, TestRestoreBackupConnection) { SetAllocatorFlags(1, kOnlyLocalPorts); // Create channels and let them go writable, as usual. - CreateChannels(1); IceConfig config = CreateIceConfig(1000, GATHER_CONTINUALLY); config.regather_on_failed_networks_interval = rtc::Optional(2000); - ep1_ch1()->SetIceConfig(config); - ep2_ch1()->SetIceConfig(config); + CreateChannels(1, config, config); EXPECT_TRUE_SIMULATED_WAIT(ep1_ch1()->receiving() && ep1_ch1()->writable() && ep2_ch1()->receiving() && ep2_ch1()->writable(), diff --git a/webrtc/p2p/base/portallocator.h b/webrtc/p2p/base/portallocator.h index 8eef714b09..8befbadcc7 100644 --- a/webrtc/p2p/base/portallocator.h +++ b/webrtc/p2p/base/portallocator.h @@ -158,14 +158,22 @@ class PortAllocatorSession : public sigslot::has_slots<> { // Default filter should be CF_ALL. virtual void SetCandidateFilter(uint32_t filter) = 0; - // Starts gathering STUN and Relay configurations. + // Starts gathering ports and ICE candidates. virtual void StartGettingPorts() = 0; - // Completely stops the gathering process and will not start new ones. + // Completely stops gathering. Will not gather again unless StartGettingPorts + // is called again. virtual void StopGettingPorts() = 0; // Whether the session is actively getting ports. virtual bool IsGettingPorts() = 0; - // ClearGettingPorts and IsCleared are used by continual gathering. - // Only stops the existing gathering process but may start new ones if needed. + + // + // NOTE: The group of methods below is only used for continual gathering. + // + + // ClearGettingPorts should have the same immediate effect as + // StopGettingPorts, but if the implementation supports continual gathering, + // ClearGettingPorts allows additional ports/candidates to be gathered if the + // network conditions change. virtual void ClearGettingPorts() = 0; // Whether it is in the state where the existing gathering process is stopped, // but new ones may be started (basically after calling ClearGettingPorts). diff --git a/webrtc/p2p/client/basicportallocator.cc b/webrtc/p2p/client/basicportallocator.cc index 6234d77b75..e94feb4d15 100644 --- a/webrtc/p2p/client/basicportallocator.cc +++ b/webrtc/p2p/client/basicportallocator.cc @@ -253,7 +253,6 @@ void BasicPortAllocatorSession::StartGettingPorts() { void BasicPortAllocatorSession::StopGettingPorts() { ASSERT(rtc::Thread::Current() == network_thread_); - network_thread_->Post(RTC_FROM_HERE, this, MSG_CONFIG_STOP); ClearGettingPorts(); // Note: this must be called after ClearGettingPorts because both may set the // session state and we should set the state to STOPPED. @@ -266,6 +265,7 @@ void BasicPortAllocatorSession::ClearGettingPorts() { for (uint32_t i = 0; i < sequences_.size(); ++i) { sequences_[i]->Stop(); } + network_thread_->Post(RTC_FROM_HERE, this, MSG_CONFIG_STOP); state_ = SessionState::CLEARED; } diff --git a/webrtc/p2p/client/basicportallocator_unittest.cc b/webrtc/p2p/client/basicportallocator_unittest.cc index 93951ff80b..c9831f525b 100644 --- a/webrtc/p2p/client/basicportallocator_unittest.cc +++ b/webrtc/p2p/client/basicportallocator_unittest.cc @@ -1608,6 +1608,7 @@ TEST_F(BasicPortAllocatorTest, TestStopGettingPorts) { // After stopping getting ports, adding a new interface will not start // getting ports again. + allocator_->set_step_delay(kMinimumStepDelay); candidates_.clear(); ports_.clear(); candidate_allocation_done_ = false; @@ -1625,17 +1626,18 @@ TEST_F(BasicPortAllocatorTest, TestClearGettingPorts) { ASSERT_EQ_WAIT(2U, candidates_.size(), 1000); EXPECT_EQ(2U, ports_.size()); session_->ClearGettingPorts(); - WAIT(candidate_allocation_done_, 1000); - EXPECT_FALSE(candidate_allocation_done_); + EXPECT_TRUE_WAIT(candidate_allocation_done_, 1000); // After clearing getting ports, adding a new interface will start getting // ports again. + allocator_->set_step_delay(kMinimumStepDelay); candidates_.clear(); ports_.clear(); candidate_allocation_done_ = false; network_manager_.AddInterface(kClientAddr2); ASSERT_EQ_WAIT(2U, candidates_.size(), 1000); EXPECT_EQ(2U, ports_.size()); + EXPECT_TRUE_WAIT(candidate_allocation_done_, kDefaultAllocationTimeout); } // Test that the ports and candidates are updated with new ufrag/pwd/etc. when