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}
diff --git a/webrtc/api/webrtcsession_unittest.cc b/webrtc/api/webrtcsession_unittest.cc
index c0d8a3b..6c0170e 100644
--- a/webrtc/api/webrtcsession_unittest.cc
+++ b/webrtc/api/webrtcsession_unittest.cc
@@ -2258,6 +2258,10 @@
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 @@
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 @@
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 d4d0e5e..03b0ed4 100644
--- a/webrtc/p2p/base/p2ptransportchannel.cc
+++ b/webrtc/p2p/base/p2ptransportchannel.cc
@@ -351,9 +351,14 @@
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::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 9342a20..26db621 100644
--- a/webrtc/p2p/base/p2ptransportchannel_unittest.cc
+++ b/webrtc/p2p/base/p2ptransportchannel_unittest.cc
@@ -305,7 +305,9 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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<int>(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 8eef714..8befbad 100644
--- a/webrtc/p2p/base/portallocator.h
+++ b/webrtc/p2p/base/portallocator.h
@@ -158,14 +158,22 @@
// 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 6234d77..e94feb4 100644
--- a/webrtc/p2p/client/basicportallocator.cc
+++ b/webrtc/p2p/client/basicportallocator.cc
@@ -253,7 +253,6 @@
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 @@
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 93951ff..c9831f5 100644
--- a/webrtc/p2p/client/basicportallocator_unittest.cc
+++ b/webrtc/p2p/client/basicportallocator_unittest.cc
@@ -1608,6 +1608,7 @@
// 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 @@
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
