Making candidate pool size behave as decided in JSEP.
To simplify things, the candidate pool is only used in the first
offer/answer.
After setting a local description, the size is frozen, and changing ICE
servers won't refresh the pool.
After setting an answer, the pooled candidates are discarded.
BUG=webrtc:5180
Review-Url: https://codereview.webrtc.org/2717893003
Cr-Original-Commit-Position: refs/heads/master@{#17178}
Cr-Mirrored-From: https://chromium.googlesource.com/external/webrtc
Cr-Mirrored-Commit: 42a426372878850160e1e627478335a880cebd9f
diff --git a/p2p/base/portallocator.cc b/p2p/base/portallocator.cc
index dc4166a..a587aa8 100644
--- a/p2p/base/portallocator.cc
+++ b/p2p/base/portallocator.cc
@@ -40,25 +40,21 @@
turn_servers_ = turn_servers;
prune_turn_ports_ = prune_turn_ports;
- bool candidate_pool_drain_began =
- static_cast<int>(pooled_sessions_.size()) != candidate_pool_size_;
- if (candidate_pool_drain_began &&
- candidate_pool_size != candidate_pool_size_) {
- LOG(LS_ERROR) << "Trying to change candidate pool size after pool started "
- "to be drained.";
- return false;
+ if (candidate_pool_frozen_) {
+ if (candidate_pool_size != candidate_pool_size_) {
+ LOG(LS_ERROR) << "Trying to change candidate pool size after pool was "
+ << "frozen.";
+ return false;
+ }
+ return true;
}
+
if (candidate_pool_size < 0) {
LOG(LS_ERROR) << "Can't set negative pool size.";
return false;
}
- candidate_pool_size_ = candidate_pool_size;
- // If sessions need to be recreated, only recreate as many as the current
- // pool size if the pool has begun to be drained.
- int sessions_needed = candidate_pool_drain_began
- ? static_cast<int>(pooled_sessions_.size())
- : candidate_pool_size_;
+ candidate_pool_size_ = candidate_pool_size;
// If ICE servers changed, throw away any existing pooled sessions and create
// new ones.
@@ -66,16 +62,16 @@
pooled_sessions_.clear();
}
- // If |sessions_needed| is less than the number of pooled sessions, get rid
- // of the extras.
- while (sessions_needed < static_cast<int>(pooled_sessions_.size())) {
+ // If |candidate_pool_size_| is less than the number of pooled sessions, get
+ // rid of the extras.
+ while (candidate_pool_size_ < static_cast<int>(pooled_sessions_.size())) {
pooled_sessions_.front().reset(nullptr);
pooled_sessions_.pop_front();
}
- // If |sessions_needed| is greater than the number of pooled sessions,
+ // If |candidate_pool_size_| is greater than the number of pooled sessions,
// create new sessions.
- while (static_cast<int>(pooled_sessions_.size()) < sessions_needed) {
+ while (static_cast<int>(pooled_sessions_.size()) < candidate_pool_size_) {
PortAllocatorSession* pooled_session = CreateSessionInternal("", 0, "", "");
pooled_session->StartGettingPorts();
pooled_sessions_.push_back(
@@ -122,4 +118,12 @@
return pooled_sessions_.front().get();
}
+void PortAllocator::FreezeCandidatePool() {
+ candidate_pool_frozen_ = true;
+}
+
+void PortAllocator::DiscardCandidatePool() {
+ pooled_sessions_.clear();
+}
+
} // namespace cricket
diff --git a/p2p/base/portallocator.h b/p2p/base/portallocator.h
index eb7308c..eec7580 100644
--- a/p2p/base/portallocator.h
+++ b/p2p/base/portallocator.h
@@ -309,6 +309,7 @@
allow_tcp_listen_(true),
candidate_filter_(CF_ALL) {
}
+
virtual ~PortAllocator() {}
// This should be called on the PortAllocator's thread before the
@@ -318,11 +319,13 @@
// Set STUN and TURN servers to be used in future sessions, and set
// candidate pool size, as described in JSEP.
//
- // If the servers are changing and the candidate pool size is nonzero,
- // existing pooled sessions will be destroyed and new ones created.
+ // If the servers are changing, and the candidate pool size is nonzero, and
+ // FreezeCandidatePool hasn't been called, existing pooled sessions will be
+ // destroyed and new ones created.
//
- // If the servers are not changing but the candidate pool size is,
- // pooled sessions will be either created or destroyed as necessary.
+ // If the servers are not changing but the candidate pool size is, and
+ // FreezeCandidatePool hasn't been called, pooled sessions will be either
+ // created or destroyed as necessary.
//
// Returns true if the configuration could successfully be changed.
bool SetConfiguration(const ServerAddresses& stun_servers,
@@ -365,6 +368,18 @@
// Returns the next session that would be returned by TakePooledSession.
const PortAllocatorSession* GetPooledSession() const;
+ // After FreezeCandidatePool is called, changing the candidate pool size will
+ // no longer be allowed, and changing ICE servers will not cause pooled
+ // sessions to be recreated.
+ //
+ // Expected to be called when SetLocalDescription is called on a
+ // PeerConnection. Can be called safely on any thread as long as not
+ // simultaneously with SetConfiguration.
+ void FreezeCandidatePool();
+
+ // Discard any remaining pooled sessions.
+ void DiscardCandidatePool();
+
uint32_t flags() const { return flags_; }
void set_flags(uint32_t flags) { flags_ = flags; }
@@ -432,6 +447,7 @@
std::vector<RelayServerConfig> turn_servers_;
int candidate_pool_size_ = 0; // Last value passed into SetConfiguration.
std::deque<std::unique_ptr<PortAllocatorSession>> pooled_sessions_;
+ bool candidate_pool_frozen_ = false;
bool prune_turn_ports_ = false;
webrtc::MetricsObserverInterface* metrics_observer_ = nullptr;
diff --git a/p2p/base/portallocator_unittest.cc b/p2p/base/portallocator_unittest.cc
index 74e2a3d..ac1c862 100644
--- a/p2p/base/portallocator_unittest.cc
+++ b/p2p/base/portallocator_unittest.cc
@@ -169,15 +169,6 @@
EXPECT_EQ(1, GetAllPooledSessionsReturnCount());
}
-// Test that after the pool starts to be drained, changing the pool size is not
-// allowed.
-TEST_F(PortAllocatorTest, CantChangePoolSizeAfterTakePooledSession) {
- SetConfigurationWithPoolSize(1);
- TakePooledSession();
- SetConfigurationWithPoolSizeExpectFailure(2);
- SetConfigurationWithPoolSizeExpectFailure(0);
-}
-
// According to JSEP, existing pooled sessions should be destroyed and new
// ones created when the ICE servers change.
TEST_F(PortAllocatorTest,
@@ -205,6 +196,31 @@
EXPECT_EQ(0, GetAllPooledSessionsReturnCount());
}
+// According to JSEP, after SetLocalDescription, setting different ICE servers
+// will not cause the pool to be refilled. This is implemented by the
+// PeerConnection calling FreezeCandidatePool when a local description is set.
+TEST_F(PortAllocatorTest,
+ SetConfigurationDoesNotRecreatePooledSessionsAfterFreezeCandidatePool) {
+ cricket::ServerAddresses stun_servers_1 = {stun_server_1};
+ std::vector<cricket::RelayServerConfig> turn_servers_1 = {turn_server_1};
+ allocator_->SetConfiguration(stun_servers_1, turn_servers_1, 1, false);
+ EXPECT_EQ(stun_servers_1, allocator_->stun_servers());
+ EXPECT_EQ(turn_servers_1, allocator_->turn_servers());
+
+ // Update with a different set of servers, but first freeze the pool.
+ allocator_->FreezeCandidatePool();
+ cricket::ServerAddresses stun_servers_2 = {stun_server_2};
+ std::vector<cricket::RelayServerConfig> turn_servers_2 = {turn_server_2};
+ allocator_->SetConfiguration(stun_servers_2, turn_servers_2, 2, false);
+ EXPECT_EQ(stun_servers_2, allocator_->stun_servers());
+ EXPECT_EQ(turn_servers_2, allocator_->turn_servers());
+ auto session = TakePooledSession();
+ ASSERT_NE(nullptr, session.get());
+ EXPECT_EQ(stun_servers_1, session->stun_servers());
+ EXPECT_EQ(turn_servers_1, session->turn_servers());
+ EXPECT_EQ(0, GetAllPooledSessionsReturnCount());
+}
+
TEST_F(PortAllocatorTest, GetPooledSessionReturnsNextSession) {
SetConfigurationWithPoolSize(2);
auto peeked_session_1 = GetPooledSession();
@@ -247,3 +263,11 @@
auto session = TakePooledSession();
EXPECT_EQ(cricket::CF_RELAY, session->candidate_filter());
}
+
+// Verify that after DiscardCandidatePool, TakePooledSession doesn't return
+// anything.
+TEST_F(PortAllocatorTest, DiscardCandidatePool) {
+ SetConfigurationWithPoolSize(1);
+ allocator_->DiscardCandidatePool();
+ EXPECT_EQ(0, GetAllPooledSessionsReturnCount());
+}
diff --git a/p2p/client/basicportallocator.cc b/p2p/client/basicportallocator.cc
index d76ff56..aebe0f2 100644
--- a/p2p/client/basicportallocator.cc
+++ b/p2p/client/basicportallocator.cc
@@ -169,6 +169,9 @@
}
BasicPortAllocator::~BasicPortAllocator() {
+ // Our created port allocator sessions depend on us, so destroy our remaining
+ // pooled sessions before anything else.
+ DiscardCandidatePool();
}
PortAllocatorSession* BasicPortAllocator::CreateSessionInternal(
diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc
index c5d6591..d79868d 100644
--- a/pc/peerconnection.cc
+++ b/pc/peerconnection.cc
@@ -1261,10 +1261,24 @@
signaling_thread()->Post(RTC_FROM_HERE, this,
MSG_SET_SESSIONDESCRIPTION_SUCCESS, msg);
+ // According to JSEP, after setLocalDescription, changing the candidate pool
+ // size is not allowed, and changing the set of ICE servers will not result
+ // in new candidates being gathered.
+ port_allocator_->FreezeCandidatePool();
+
// MaybeStartGathering needs to be called after posting
// MSG_SET_SESSIONDESCRIPTION_SUCCESS, so that we don't signal any candidates
// before signaling that SetLocalDescription completed.
session_->MaybeStartGathering();
+
+ if (desc->type() == SessionDescriptionInterface::kAnswer) {
+ // TODO(deadbeef): We already had to hop to the network thread for
+ // MaybeStartGathering...
+ network_thread()->Invoke<void>(
+ RTC_FROM_HERE,
+ rtc::Bind(&cricket::PortAllocator::DiscardCandidatePool,
+ port_allocator_.get()));
+ }
}
void PeerConnection::SetRemoteDescription(
@@ -1372,6 +1386,15 @@
SetSessionDescriptionMsg* msg = new SetSessionDescriptionMsg(observer);
signaling_thread()->Post(RTC_FROM_HERE, this,
MSG_SET_SESSIONDESCRIPTION_SUCCESS, msg);
+
+ if (desc->type() == SessionDescriptionInterface::kAnswer) {
+ // TODO(deadbeef): We already had to hop to the network thread for
+ // MaybeStartGathering...
+ network_thread()->Invoke<void>(
+ RTC_FROM_HERE,
+ rtc::Bind(&cricket::PortAllocator::DiscardCandidatePool,
+ port_allocator_.get()));
+ }
}
PeerConnectionInterface::RTCConfiguration PeerConnection::GetConfiguration() {
@@ -1537,6 +1560,10 @@
session_->Close();
event_log_.reset();
+ network_thread()->Invoke<void>(
+ RTC_FROM_HERE,
+ rtc::Bind(&cricket::PortAllocator::DiscardCandidatePool,
+ port_allocator_.get()));
}
void PeerConnection::OnSessionStateChange(WebRtcSession* /*session*/,
diff --git a/pc/peerconnectioninterface_unittest.cc b/pc/peerconnectioninterface_unittest.cc
index 02c7932..ccf1a5e 100644
--- a/pc/peerconnectioninterface_unittest.cc
+++ b/pc/peerconnectioninterface_unittest.cc
@@ -2277,6 +2277,43 @@
EXPECT_EQ(RTCErrorType::INVALID_MODIFICATION, error.type());
}
+// Test that after setting an answer, extra pooled sessions are discarded. The
+// ICE candidate pool is only intended to be used for the first offer/answer.
+TEST_F(PeerConnectionInterfaceTest,
+ ExtraPooledSessionsDiscardedAfterApplyingAnswer) {
+ CreatePeerConnection();
+
+ // Set a larger-than-necessary size.
+ PeerConnectionInterface::RTCConfiguration config;
+ config.ice_candidate_pool_size = 4;
+ EXPECT_TRUE(pc_->SetConfiguration(config));
+
+ // Do offer/answer.
+ CreateOfferAsRemoteDescription();
+ CreateAnswerAsLocalDescription();
+
+ // Expect no pooled sessions to be left.
+ const cricket::PortAllocatorSession* session =
+ port_allocator_->GetPooledSession();
+ EXPECT_EQ(nullptr, session);
+}
+
+// After Close is called, pooled candidates should be discarded so as to not
+// waste network resources.
+TEST_F(PeerConnectionInterfaceTest, PooledSessionsDiscardedAfterClose) {
+ CreatePeerConnection();
+
+ PeerConnectionInterface::RTCConfiguration config;
+ config.ice_candidate_pool_size = 3;
+ EXPECT_TRUE(pc_->SetConfiguration(config));
+ pc_->Close();
+
+ // Expect no pooled sessions to be left.
+ const cricket::PortAllocatorSession* session =
+ port_allocator_->GetPooledSession();
+ EXPECT_EQ(nullptr, session);
+}
+
// Test that SetConfiguration returns an invalid modification error if
// modifying a field in the configuration that isn't allowed to be modified.
TEST_F(PeerConnectionInterfaceTest,
