From f4e8cf0d5b68fd16fe3af1e3d83dd8c624677b28 Mon Sep 17 00:00:00 2001 From: danilchap Date: Thu, 30 Jun 2016 01:55:03 -0700 Subject: [PATCH] Revert of Add config to prune TURN ports (patchset #12 id:360001 of https://codereview.webrtc.org/2093623004/ ) Reason for revert: Breaks Win32/Win64 Debug bots in client.webrtc waterfall Original issue's description: > Add config to prune low-priority TURN ports for creating connections > When the flag prune_turn_ports is set, When a high-priority turn port becomes available, it will prune low-priority ones. The pruned port will not be used for creating connections locally and its candidates will not be sent over to the remove side (unless they have been sent before being pruned). > > This effectively reduces the number of TURN candidates and connections created by TURN ports. > > BUG= > R=deadbeef@webrtc.org, pthatcher@webrtc.org > > Committed: https://crrev.com/17aac053f585e892114974d2eb248e05ad37f973 > Cr-Commit-Position: refs/heads/master@{#13335} TBR=pthatcher@webrtc.org,deadbeef@webrtc.org,honghaiz@webrtc.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= Review-Url: https://codereview.webrtc.org/2111663003 Cr-Commit-Position: refs/heads/master@{#13342} --- webrtc/api/peerconnection.cc | 6 +- webrtc/api/peerconnectioninterface.h | 1 - webrtc/p2p/base/p2ptransportchannel.cc | 32 +-- webrtc/p2p/base/p2ptransportchannel.h | 3 - .../p2p/base/p2ptransportchannel_unittest.cc | 8 +- webrtc/p2p/base/port_unittest.cc | 2 - webrtc/p2p/base/portallocator.cc | 4 +- webrtc/p2p/base/portallocator.h | 11 +- webrtc/p2p/base/portallocator_unittest.cc | 10 +- webrtc/p2p/base/portinterface.h | 2 - webrtc/p2p/base/relayport.h | 6 - webrtc/p2p/base/stunport.h | 5 +- webrtc/p2p/base/tcpport.h | 2 - webrtc/p2p/base/turnport.h | 2 - webrtc/p2p/client/basicportallocator.cc | 174 +++++----------- webrtc/p2p/client/basicportallocator.h | 18 +- .../p2p/client/basicportallocator_unittest.cc | 185 +----------------- 17 files changed, 79 insertions(+), 392 deletions(-) diff --git a/webrtc/api/peerconnection.cc b/webrtc/api/peerconnection.cc index cc33aa88ce..e0b4fca48d 100644 --- a/webrtc/api/peerconnection.cc +++ b/webrtc/api/peerconnection.cc @@ -2212,8 +2212,7 @@ bool PeerConnection::InitializePortAllocator_n( // Call this last since it may create pooled allocator sessions using the // properties set above. port_allocator_->SetConfiguration(stun_servers, turn_servers, - configuration.ice_candidate_pool_size, - configuration.prune_turn_ports); + configuration.ice_candidate_pool_size); return true; } @@ -2229,8 +2228,7 @@ bool PeerConnection::ReconfigurePortAllocator_n( // Call this last since it may create pooled allocator sessions using the // candidate filter set above. port_allocator_->SetConfiguration(stun_servers, turn_servers, - configuration.ice_candidate_pool_size, - configuration.prune_turn_ports); + configuration.ice_candidate_pool_size); return true; } diff --git a/webrtc/api/peerconnectioninterface.h b/webrtc/api/peerconnectioninterface.h index 3757dad8bb..0ac37e1564 100644 --- a/webrtc/api/peerconnectioninterface.h +++ b/webrtc/api/peerconnectioninterface.h @@ -300,7 +300,6 @@ class PeerConnectionInterface : public rtc::RefCountInterface { rtc::Optional combined_audio_video_bwe; rtc::Optional enable_dtls_srtp; int ice_candidate_pool_size = 0; - bool prune_turn_ports = false; }; struct RTCOfferAnswerOptions { diff --git a/webrtc/p2p/base/p2ptransportchannel.cc b/webrtc/p2p/base/p2ptransportchannel.cc index ebbc0eabbe..b5d6755cee 100644 --- a/webrtc/p2p/base/p2ptransportchannel.cc +++ b/webrtc/p2p/base/p2ptransportchannel.cc @@ -125,7 +125,6 @@ void P2PTransportChannel::AddAllocatorSession( session->set_generation(static_cast(allocator_sessions_.size())); session->SignalPortReady.connect(this, &P2PTransportChannel::OnPortReady); - session->SignalPortPruned.connect(this, &P2PTransportChannel::OnPortPruned); session->SignalCandidatesReady.connect( this, &P2PTransportChannel::OnCandidatesReady); session->SignalCandidatesAllocationDone.connect( @@ -1617,7 +1616,7 @@ void P2PTransportChannel::OnPortDestroyed(PortInterface* port) { std::remove(removed_ports_.begin(), removed_ports_.end(), port), removed_ports_.end()); - LOG(INFO) << "Removed port because it is destroyed: " + LOG(INFO) << "Removed port from p2p socket: " << static_cast(ports_.size()) << " remaining"; } @@ -1627,11 +1626,15 @@ void P2PTransportChannel::OnPortNetworkInactive(PortInterface* port) { if (!config_.gather_continually) { return; } - if (!RemovePort(port)) { + auto it = std::find(ports_.begin(), ports_.end(), port); + // Don't need to do anything if the port has been deleted from the port list. + if (it == ports_.end()) { return; } - LOG(INFO) << "Removed port because its network is inactive : " - << port->ToString() << " " << ports_.size() << " remaining"; + removed_ports_.push_back(*it); + ports_.erase(it); + LOG(INFO) << "Removed port due to inactive networks: " << ports_.size() + << " remaining"; std::vector candidates = port->Candidates(); for (Candidate& candidate : candidates) { candidate.set_transport_name(transport_name()); @@ -1639,25 +1642,6 @@ void P2PTransportChannel::OnPortNetworkInactive(PortInterface* port) { SignalCandidatesRemoved(this, candidates); } -void P2PTransportChannel::OnPortPruned(PortAllocatorSession* session, - PortInterface* port) { - if (RemovePort(port)) { - LOG(INFO) << "Removed port because it is pruned: " << port->ToString() - << " " << ports_.size() << " remaining"; - } -} - -bool P2PTransportChannel::RemovePort(PortInterface* port) { - auto it = std::find(ports_.begin(), ports_.end(), port); - // Don't need to do anything if the port has been deleted from the port list. - if (it == ports_.end()) { - return false; - } - ports_.erase(it); - removed_ports_.push_back(port); - return true; -} - // We data is available, let listeners know void P2PTransportChannel::OnReadPacket(Connection* connection, const char* data, diff --git a/webrtc/p2p/base/p2ptransportchannel.h b/webrtc/p2p/base/p2ptransportchannel.h index 3d32f31565..d90cc61ebc 100644 --- a/webrtc/p2p/base/p2ptransportchannel.h +++ b/webrtc/p2p/base/p2ptransportchannel.h @@ -268,9 +268,6 @@ class P2PTransportChannel : public TransportChannelImpl, void AddConnection(Connection* connection); void OnPortReady(PortAllocatorSession *session, PortInterface* port); - void OnPortPruned(PortAllocatorSession* session, PortInterface* port); - // Returns true if the port is found and removed from |ports_|. - bool RemovePort(PortInterface* port); void OnCandidatesReady(PortAllocatorSession *session, const std::vector& candidates); void OnCandidatesAllocationDone(PortAllocatorSession* session); diff --git a/webrtc/p2p/base/p2ptransportchannel_unittest.cc b/webrtc/p2p/base/p2ptransportchannel_unittest.cc index 0408552848..b5645007c3 100644 --- a/webrtc/p2p/base/p2ptransportchannel_unittest.cc +++ b/webrtc/p2p/base/p2ptransportchannel_unittest.cc @@ -1572,9 +1572,9 @@ TEST_F(P2PTransportChannelTest, TestUsingPooledSessionBeforeDoneGathering) { auto& allocator_2 = GetEndpoint(1)->allocator_; int pool_size = 1; allocator_1->SetConfiguration(allocator_1->stun_servers(), - allocator_1->turn_servers(), pool_size, false); + allocator_1->turn_servers(), pool_size); allocator_2->SetConfiguration(allocator_2->stun_servers(), - allocator_2->turn_servers(), pool_size, false); + allocator_2->turn_servers(), pool_size); const PortAllocatorSession* pooled_session_1 = allocator_1->GetPooledSession(); const PortAllocatorSession* pooled_session_2 = @@ -1615,9 +1615,9 @@ TEST_F(P2PTransportChannelTest, TestUsingPooledSessionAfterDoneGathering) { auto& allocator_2 = GetEndpoint(1)->allocator_; int pool_size = 1; allocator_1->SetConfiguration(allocator_1->stun_servers(), - allocator_1->turn_servers(), pool_size, false); + allocator_1->turn_servers(), pool_size); allocator_2->SetConfiguration(allocator_2->stun_servers(), - allocator_2->turn_servers(), pool_size, false); + allocator_2->turn_servers(), pool_size); const PortAllocatorSession* pooled_session_1 = allocator_1->GetPooledSession(); const PortAllocatorSession* pooled_session_2 = diff --git a/webrtc/p2p/base/port_unittest.cc b/webrtc/p2p/base/port_unittest.cc index e156e017fe..743cddacdb 100644 --- a/webrtc/p2p/base/port_unittest.cc +++ b/webrtc/p2p/base/port_unittest.cc @@ -152,8 +152,6 @@ class TestPort : public Port { return true; } - virtual ProtocolType GetProtocol() const { return PROTO_UDP; } - // Exposed for testing candidate building. void AddCandidateAddress(const rtc::SocketAddress& addr) { AddAddress(addr, addr, rtc::SocketAddress(), "udp", "", "", Type(), diff --git a/webrtc/p2p/base/portallocator.cc b/webrtc/p2p/base/portallocator.cc index 9ee08a1476..f9f87b007f 100644 --- a/webrtc/p2p/base/portallocator.cc +++ b/webrtc/p2p/base/portallocator.cc @@ -32,13 +32,11 @@ PortAllocatorSession::PortAllocatorSession(const std::string& content_name, void PortAllocator::SetConfiguration( const ServerAddresses& stun_servers, const std::vector& turn_servers, - int candidate_pool_size, - bool prune_turn_ports) { + int candidate_pool_size) { bool ice_servers_changed = (stun_servers != stun_servers_ || turn_servers != turn_servers_); stun_servers_ = stun_servers; turn_servers_ = turn_servers; - prune_turn_ports_ = prune_turn_ports; // If ICE servers changed, throw away any existing pooled sessions and create // new ones. diff --git a/webrtc/p2p/base/portallocator.h b/webrtc/p2p/base/portallocator.h index 9b4465d3de..6a32b9668f 100644 --- a/webrtc/p2p/base/portallocator.h +++ b/webrtc/p2p/base/portallocator.h @@ -178,11 +178,6 @@ class PortAllocatorSession : public sigslot::has_slots<> { sigslot::signal2&> SignalCandidatesReady; sigslot::signal1 SignalCandidatesAllocationDone; - // A TURN port is pruned if a higher-priority TURN port becomes ready - // (pairable). When it is pruned, it will not be used for creating - // connections and its candidates will not be sent to the remote side - // if they have not been sent. - sigslot::signal2 SignalPortPruned; virtual uint32_t generation() { return generation_; } virtual void set_generation(uint32_t generation) { generation_ = generation; } @@ -258,8 +253,7 @@ class PortAllocator : public sigslot::has_slots<> { // pooled sessions will be either created or destroyed as necessary. void SetConfiguration(const ServerAddresses& stun_servers, const std::vector& turn_servers, - int candidate_pool_size, - bool prune_turn_ports); + int candidate_pool_size); const ServerAddresses& stun_servers() const { return stun_servers_; } @@ -333,8 +327,6 @@ class PortAllocator : public sigslot::has_slots<> { candidate_filter_ = filter; } - bool prune_turn_ports() const { return prune_turn_ports_; } - // Gets/Sets the Origin value used for WebRTC STUN requests. const std::string& origin() const { return origin_; } void set_origin(const std::string& origin) { origin_ = origin; } @@ -365,7 +357,6 @@ class PortAllocator : public sigslot::has_slots<> { // both owned by this class and taken by TakePooledSession. int allocated_pooled_session_count_ = 0; std::deque> pooled_sessions_; - bool prune_turn_ports_ = false; }; } // namespace cricket diff --git a/webrtc/p2p/base/portallocator_unittest.cc b/webrtc/p2p/base/portallocator_unittest.cc index 06abaad2a0..6a34a25c13 100644 --- a/webrtc/p2p/base/portallocator_unittest.cc +++ b/webrtc/p2p/base/portallocator_unittest.cc @@ -35,7 +35,7 @@ class PortAllocatorTest : public testing::Test, public sigslot::has_slots<> { void SetConfigurationWithPoolSize(int candidate_pool_size) { allocator_->SetConfiguration(cricket::ServerAddresses(), std::vector(), - candidate_pool_size, false); + candidate_pool_size); } std::unique_ptr CreateSession( @@ -107,14 +107,14 @@ TEST_F(PortAllocatorTest, CreateSession) { TEST_F(PortAllocatorTest, SetConfigurationUpdatesIceServers) { cricket::ServerAddresses stun_servers_1 = {stun_server_1}; std::vector turn_servers_1 = {turn_server_1}; - allocator_->SetConfiguration(stun_servers_1, turn_servers_1, 0, false); + allocator_->SetConfiguration(stun_servers_1, turn_servers_1, 0); EXPECT_EQ(stun_servers_1, allocator_->stun_servers()); EXPECT_EQ(turn_servers_1, allocator_->turn_servers()); // Update with a different set of servers. cricket::ServerAddresses stun_servers_2 = {stun_server_2}; std::vector turn_servers_2 = {turn_server_2}; - allocator_->SetConfiguration(stun_servers_2, turn_servers_2, 0, false); + allocator_->SetConfiguration(stun_servers_2, turn_servers_2, 0); EXPECT_EQ(stun_servers_2, allocator_->stun_servers()); EXPECT_EQ(turn_servers_2, allocator_->turn_servers()); } @@ -182,14 +182,14 @@ TEST_F(PortAllocatorTest, SetConfigurationRecreatesPooledSessionsWhenIceServersChange) { cricket::ServerAddresses stun_servers_1 = {stun_server_1}; std::vector turn_servers_1 = {turn_server_1}; - allocator_->SetConfiguration(stun_servers_1, turn_servers_1, 1, false); + allocator_->SetConfiguration(stun_servers_1, turn_servers_1, 1); EXPECT_EQ(stun_servers_1, allocator_->stun_servers()); EXPECT_EQ(turn_servers_1, allocator_->turn_servers()); // Update with a different set of servers (and also change pool size). cricket::ServerAddresses stun_servers_2 = {stun_server_2}; std::vector turn_servers_2 = {turn_server_2}; - allocator_->SetConfiguration(stun_servers_2, turn_servers_2, 2, false); + allocator_->SetConfiguration(stun_servers_2, turn_servers_2, 2); EXPECT_EQ(stun_servers_2, allocator_->stun_servers()); EXPECT_EQ(turn_servers_2, allocator_->turn_servers()); auto session_1 = TakePooledSession(); diff --git a/webrtc/p2p/base/portinterface.h b/webrtc/p2p/base/portinterface.h index 987c3fc1ee..e73861965e 100644 --- a/webrtc/p2p/base/portinterface.h +++ b/webrtc/p2p/base/portinterface.h @@ -76,8 +76,6 @@ class PortInterface { virtual int GetOption(rtc::Socket::Option opt, int* value) = 0; virtual int GetError() = 0; - virtual ProtocolType GetProtocol() const = 0; - virtual const std::vector& Candidates() const = 0; // Sends the given packet to the given address, provided that the address is diff --git a/webrtc/p2p/base/relayport.h b/webrtc/p2p/base/relayport.h index 8fa2235043..402736c34d 100644 --- a/webrtc/p2p/base/relayport.h +++ b/webrtc/p2p/base/relayport.h @@ -68,12 +68,6 @@ class RelayPort : public Port { const ProtocolAddress * ServerAddress(size_t index) const; bool IsReady() { return ready_; } - ProtocolType GetProtocol() const override { - // We shouldn't be using RelayPort, but we need to provide an - // implementation here. - return PROTO_UDP; - } - // Used for testing. sigslot::signal1 SignalConnectFailure; sigslot::signal1 SignalSoftTimeout; diff --git a/webrtc/p2p/base/stunport.h b/webrtc/p2p/base/stunport.h index 1bbe25f0e8..82be6fae2a 100644 --- a/webrtc/p2p/base/stunport.h +++ b/webrtc/p2p/base/stunport.h @@ -80,7 +80,8 @@ class UDPPort : public Port { const ServerAddresses& server_addresses() const { return server_addresses_; } - void set_server_addresses(const ServerAddresses& addresses) { + void + set_server_addresses(const ServerAddresses& addresses) { server_addresses_ = addresses; } @@ -104,8 +105,6 @@ class UDPPort : public Port { return protocol == UDP_PROTOCOL_NAME; } - virtual ProtocolType GetProtocol() const { return PROTO_UDP; } - void set_stun_keepalive_delay(int delay) { stun_keepalive_delay_ = delay; } diff --git a/webrtc/p2p/base/tcpport.h b/webrtc/p2p/base/tcpport.h index 76a73f8f47..77bbd09eab 100644 --- a/webrtc/p2p/base/tcpport.h +++ b/webrtc/p2p/base/tcpport.h @@ -61,8 +61,6 @@ class TCPPort : public Port { return protocol == TCP_PROTOCOL_NAME || protocol == SSLTCP_PROTOCOL_NAME; } - ProtocolType GetProtocol() const override { return PROTO_TCP; } - protected: TCPPort(rtc::Thread* thread, rtc::PacketSocketFactory* factory, diff --git a/webrtc/p2p/base/turnport.h b/webrtc/p2p/base/turnport.h index 3bb09bc062..6e528aaf69 100644 --- a/webrtc/p2p/base/turnport.h +++ b/webrtc/p2p/base/turnport.h @@ -85,8 +85,6 @@ class TurnPort : public Port { } const RelayCredentials& credentials() const { return credentials_; } - virtual ProtocolType GetProtocol() const { return server_address_.proto; } - virtual void PrepareAddress(); virtual Connection* CreateConnection( const Candidate& c, PortInterface::CandidateOrigin origin); diff --git a/webrtc/p2p/client/basicportallocator.cc b/webrtc/p2p/client/basicportallocator.cc index 19f23b3f00..e39a44013c 100644 --- a/webrtc/p2p/client/basicportallocator.cc +++ b/webrtc/p2p/client/basicportallocator.cc @@ -47,36 +47,6 @@ const int PHASE_SSLTCP = 3; const int kNumPhases = 4; -// Gets address family priority: IPv6 > IPv4 > Unspecified. -int GetAddressFamilyPriority(int ip_family) { - switch (ip_family) { - case AF_INET6: - return 2; - case AF_INET: - return 1; - default: - RTC_DCHECK(false); - return 0; - } -} - -// Returns positive if a is better, negative if b is better, and 0 otherwise. -int ComparePort(const cricket::Port* a, const cricket::Port* b) { - static constexpr int a_is_better = 1; - static constexpr int b_is_better = -1; - // Protocol type is defined as UDP = 0, TCP = 1, SSLTCP = 2. - if (a->GetProtocol() < b->GetProtocol()) { - return a_is_better; - } - if (a->GetProtocol() > b->GetProtocol()) { - return b_is_better; - } - - int a_family = GetAddressFamilyPriority(a->Network()->GetBestIP().family()); - int b_family = GetAddressFamilyPriority(b->Network()->GetBestIP().family()); - return a_family - b_family; -} - } // namespace namespace cricket { @@ -104,7 +74,7 @@ BasicPortAllocator::BasicPortAllocator(rtc::NetworkManager* network_manager, const ServerAddresses& stun_servers) : network_manager_(network_manager), socket_factory_(socket_factory) { ASSERT(socket_factory_ != NULL); - SetConfiguration(stun_servers, std::vector(), 0, false); + SetConfiguration(stun_servers, std::vector(), 0); Construct(); } @@ -131,7 +101,7 @@ BasicPortAllocator::BasicPortAllocator( turn_servers.push_back(config); } - SetConfiguration(stun_servers, turn_servers, 0, false); + SetConfiguration(stun_servers, turn_servers, 0); Construct(); } @@ -152,30 +122,24 @@ PortAllocatorSession* BasicPortAllocator::CreateSessionInternal( void BasicPortAllocator::AddTurnServer(const RelayServerConfig& turn_server) { std::vector new_turn_servers = turn_servers(); new_turn_servers.push_back(turn_server); - SetConfiguration(stun_servers(), new_turn_servers, candidate_pool_size(), - prune_turn_ports()); + SetConfiguration(stun_servers(), new_turn_servers, candidate_pool_size()); } // BasicPortAllocatorSession BasicPortAllocatorSession::BasicPortAllocatorSession( - BasicPortAllocator* allocator, + BasicPortAllocator *allocator, const std::string& content_name, int component, const std::string& ice_ufrag, const std::string& ice_pwd) - : PortAllocatorSession(content_name, - component, - ice_ufrag, - ice_pwd, - allocator->flags()), - allocator_(allocator), - network_thread_(NULL), + : PortAllocatorSession(content_name, component, + ice_ufrag, ice_pwd, allocator->flags()), + allocator_(allocator), network_thread_(NULL), socket_factory_(allocator->socket_factory()), allocation_started_(false), network_manager_started_(false), running_(false), - allocation_sequences_created_(false), - prune_turn_ports_(allocator->prune_turn_ports()) { + allocation_sequences_created_(false) { allocator_->network_manager()->SignalNetworksChanged.connect( this, &BasicPortAllocatorSession::OnNetworksChanged); allocator_->network_manager()->StartUpdating(); @@ -253,9 +217,9 @@ void BasicPortAllocatorSession::ClearGettingPorts() { std::vector BasicPortAllocatorSession::ReadyPorts() const { std::vector ret; - for (const PortData& data : ports_) { - if (data.ready()) { - ret.push_back(data.port()); + for (const PortData& port : ports_) { + if (port.has_pairable_candidate() && !port.error()) { + ret.push_back(port.port()); } } return ret; @@ -264,10 +228,6 @@ std::vector BasicPortAllocatorSession::ReadyPorts() const { std::vector BasicPortAllocatorSession::ReadyCandidates() const { std::vector candidates; for (const PortData& data : ports_) { - if (!data.ready()) { - continue; - } - for (const Candidate& candidate : data.port()->Candidates()) { if (!CheckCandidateFilter(candidate)) { continue; @@ -318,11 +278,16 @@ bool BasicPortAllocatorSession::CandidatesAllocationDone() const { return false; } - // If all allocated ports are no longer gathering, session must have got all + // If all allocated ports are in complete state, session must have got all // expected candidates. Session will trigger candidates allocation complete // signal. - return std::none_of(ports_.begin(), ports_.end(), - [](const PortData& port) { return port.inprogress(); }); + if (!std::all_of(ports_.begin(), ports_.end(), [](const PortData& port) { + return (port.complete() || port.error()); + })) { + return false; + } + + return true; } void BasicPortAllocatorSession::OnMessage(rtc::Message *message) { @@ -392,7 +357,7 @@ void BasicPortAllocatorSession::OnConfigStop() { bool send_signal = false; for (std::vector::iterator it = ports_.begin(); it != ports_.end(); ++it) { - if (it->inprogress()) { + if (!it->complete() && !it->error()) { // Updating port state to error, which didn't finish allocating candidates // yet. it->set_error(); @@ -478,8 +443,11 @@ void BasicPortAllocatorSession::DoAllocate() { LOG(LS_WARNING) << "Machine has no networks; no ports will be allocated"; done_signal_needed = true; } else { - PortConfiguration* config = configs_.empty() ? nullptr : configs_.back(); for (uint32_t i = 0; i < networks.size(); ++i) { + PortConfiguration* config = NULL; + if (configs_.size() > 0) + config = configs_.back(); + uint32_t sequence_flags = flags(); if ((sequence_flags & DISABLE_ALL_PHASES) == DISABLE_ALL_PHASES) { // If all the ports are disabled we should just fire the allocation @@ -600,85 +568,37 @@ void BasicPortAllocatorSession::OnCandidateReady( PortData* data = FindPort(port); ASSERT(data != NULL); // Discarding any candidate signal if port allocation status is - // already done with gathering. - if (!data->inprogress()) { + // already in completed state. + if (data->complete() || data->error()) { return; } - // Mark that the port has a pairable candidate, either because we have a - // usable candidate from the port, or simply because the port is bound to the - // any address and therefore has no host candidate. This will trigger the port - // to start creating candidate pairs (connections) and issue connectivity - // checks. If port has already been marked as having a pairable candidate, - // do nothing here. - // Note: We should check whether any candidates may become ready after this - // because there we will check whether the candidate is generated by the ready - // ports, which may include this port. - bool pruned_port = false; - if (CandidatePairable(c, port) && !data->has_pairable_candidate()) { - data->set_has_pairable_candidate(true); - - if (prune_turn_ports_ && port->Type() == RELAY_PORT_TYPE) { - pruned_port = PruneTurnPorts(port); - } - // If the current port is not pruned yet, SignalPortReady. - if (!data->pruned()) { - SignalPortReady(this, port); - } - } - ProtocolType pvalue; bool candidate_protocol_enabled = StringToProto(c.protocol().c_str(), &pvalue) && data->sequence()->ProtocolEnabled(pvalue); - if (data->ready() && CheckCandidateFilter(c) && candidate_protocol_enabled) { + if (CheckCandidateFilter(c) && candidate_protocol_enabled) { std::vector candidates; candidates.push_back(SanitizeRelatedAddress(c)); SignalCandidatesReady(this, candidates); } - // If we have pruned any port, maybe need to signal port allocation done. - if (pruned_port) { - MaybeSignalCandidatesAllocationDone(); + // Port has already been marked as having a pairable candidate. + // Nothing to do here. + if (data->has_pairable_candidate()) { + return; } -} -Port* BasicPortAllocatorSession::GetBestTurnPortForNetwork( - const std::string& network_name) const { - Port* best_turn_port = nullptr; - for (const PortData& data : ports_) { - if (data.port()->Network()->name() == network_name && - data.port()->Type() == RELAY_PORT_TYPE && data.ready() && - (!best_turn_port || ComparePort(data.port(), best_turn_port) > 0)) { - best_turn_port = data.port(); - } + // Mark that the port has a pairable candidate, either because we have a + // usable candidate from the port, or simply because the port is bound to the + // any address and therefore has no host candidate. This will trigger the port + // to start creating candidate pairs (connections) and issue connectivity + // checks. + if (CandidatePairable(c, port)) { + data->set_has_pairable_candidate(true); + SignalPortReady(this, port); } - return best_turn_port; -} - -bool BasicPortAllocatorSession::PruneTurnPorts(Port* newly_pairable_turn_port) { - bool pruned_port = false; - // Note: We determine the same network based only on their network names. So - // if an IPv4 address and an IPv6 address have the same network name, they - // are considered the same network here. - const std::string& network_name = newly_pairable_turn_port->Network()->name(); - Port* best_turn_port = GetBestTurnPortForNetwork(network_name); - // |port| is already in the list of ports, so the best port cannot be nullptr. - RTC_CHECK(best_turn_port != nullptr); - - for (PortData& data : ports_) { - if (data.port()->Network()->name() == network_name && - data.port()->Type() == RELAY_PORT_TYPE && !data.pruned() && - ComparePort(data.port(), best_turn_port) < 0) { - data.set_pruned(); - pruned_port = true; - if (data.port() != newly_pairable_turn_port) { - SignalPortPruned(this, data.port()); - } - } - } - return pruned_port; } void BasicPortAllocatorSession::OnPortComplete(Port* port) { @@ -687,7 +607,7 @@ void BasicPortAllocatorSession::OnPortComplete(Port* port) { ASSERT(data != NULL); // Ignore any late signals. - if (!data->inprogress()) { + if (data->complete() || data->error()) { return; } @@ -702,7 +622,7 @@ void BasicPortAllocatorSession::OnPortError(Port* port) { PortData* data = FindPort(port); ASSERT(data != NULL); // We might have already given up on this port and stopped it. - if (!data->inprogress()) { + if (data->complete() || data->error()) { return; } @@ -1108,11 +1028,13 @@ void AllocationSequence::CreateRelayPorts() { return; } - for (RelayServerConfig& relay : config_->relays) { - if (relay.type == RELAY_GTURN) { - CreateGturnPort(relay); - } else if (relay.type == RELAY_TURN) { - CreateTurnPort(relay); + PortConfiguration::RelayList::const_iterator relay; + for (relay = config_->relays.begin(); + relay != config_->relays.end(); ++relay) { + if (relay->type == RELAY_GTURN) { + CreateGturnPort(*relay); + } else if (relay->type == RELAY_TURN) { + CreateTurnPort(*relay); } else { ASSERT(false); } diff --git a/webrtc/p2p/client/basicportallocator.h b/webrtc/p2p/client/basicportallocator.h index bddb3967e5..8ea66c431e 100644 --- a/webrtc/p2p/client/basicportallocator.h +++ b/webrtc/p2p/client/basicportallocator.h @@ -123,15 +123,7 @@ class BasicPortAllocatorSession : public PortAllocatorSession, bool has_pairable_candidate() const { return has_pairable_candidate_; } bool complete() const { return state_ == STATE_COMPLETE; } bool error() const { return state_ == STATE_ERROR; } - bool pruned() const { return state_ == STATE_PRUNED; } - bool inprogress() const { return state_ == STATE_INPROGRESS; } - // Returns true if this port is ready to be used. - bool ready() const { - return has_pairable_candidate_ && state_ != STATE_ERROR && - state_ != STATE_PRUNED; - } - void set_pruned() { state_ = STATE_PRUNED; } void set_has_pairable_candidate(bool has_pairable_candidate) { if (has_pairable_candidate) { ASSERT(state_ == STATE_INPROGRESS); @@ -150,9 +142,7 @@ class BasicPortAllocatorSession : public PortAllocatorSession, enum State { STATE_INPROGRESS, // Still gathering candidates. STATE_COMPLETE, // All candidates allocated and ready for process. - STATE_ERROR, // Error in gathering candidates. - STATE_PRUNED // Pruned by higher priority ports on the same network - // interface. Only TURN ports may be pruned. + STATE_ERROR // Error in gathering candidates. }; Port* port_ = nullptr; AllocationSequence* sequence_ = nullptr; @@ -188,10 +178,6 @@ class BasicPortAllocatorSession : public PortAllocatorSession, // in order to avoid leaking any information. Candidate SanitizeRelatedAddress(const Candidate& c) const; - Port* GetBestTurnPortForNetwork(const std::string& network_name) const; - // Returns true if at least one TURN port is pruned. - bool PruneTurnPorts(Port* newly_pairable_turn_port); - BasicPortAllocator* allocator_; rtc::Thread* network_thread_; std::unique_ptr owned_socket_factory_; @@ -204,8 +190,6 @@ class BasicPortAllocatorSession : public PortAllocatorSession, std::vector sequences_; std::vector ports_; uint32_t candidate_filter_ = CF_ALL; - // Whether to prune low-priority ports, taken from the port allocator. - bool prune_turn_ports_; friend class AllocationSequence; }; diff --git a/webrtc/p2p/client/basicportallocator_unittest.cc b/webrtc/p2p/client/basicportallocator_unittest.cc index 28a522fcd2..6d5d1086a9 100644 --- a/webrtc/p2p/client/basicportallocator_unittest.cc +++ b/webrtc/p2p/client/basicportallocator_unittest.cc @@ -39,15 +39,12 @@ using rtc::SocketAddress; using rtc::Thread; static const SocketAddress kClientAddr("11.11.11.11", 0); -static const SocketAddress kClientAddr2("22.22.22.22", 0); static const SocketAddress kLoopbackAddr("127.0.0.1", 0); static const SocketAddress kPrivateAddr("192.168.1.11", 0); static const SocketAddress kPrivateAddr2("192.168.1.12", 0); static const SocketAddress kClientIPv6Addr("2401:fa00:4:1000:be30:5bff:fee5:c3", 0); -static const SocketAddress kClientIPv6Addr2( - "2401:fa00:4:2000:be30:5bff:fee5:c3", - 0); +static const SocketAddress kClientAddr2("22.22.22.22", 0); static const SocketAddress kNatUdpAddr("77.77.77.77", rtc::NAT_SERVER_UDP_PORT); static const SocketAddress kNatTcpAddr("77.77.77.77", rtc::NAT_SERVER_TCP_PORT); static const SocketAddress kRemoteClientAddr("22.22.22.22", 0); @@ -59,13 +56,7 @@ static const SocketAddress kRelayTcpExtAddr("99.99.99.3", 5003); static const SocketAddress kRelaySslTcpIntAddr("99.99.99.2", 5004); static const SocketAddress kRelaySslTcpExtAddr("99.99.99.3", 5005); static const SocketAddress kTurnUdpIntAddr("99.99.99.4", 3478); -static const SocketAddress kTurnUdpIntIPv6Addr( - "2402:fb00:4:1000:be30:5bff:fee5:c3", - 3479); static const SocketAddress kTurnTcpIntAddr("99.99.99.5", 3478); -static const SocketAddress kTurnTcpIntIPv6Addr( - "2402:fb00:4:2000:be30:5bff:fee5:c3", - 3479); static const SocketAddress kTurnUdpExtAddr("99.99.99.6", 0); // Minimum and maximum port for port range tests. @@ -193,10 +184,12 @@ class BasicPortAllocatorTest : public testing::Test, turn_server.credentials = credentials; if (!udp_turn.IsNil()) { - turn_server.ports.push_back(ProtocolAddress(udp_turn, PROTO_UDP, false)); + turn_server.ports.push_back( + ProtocolAddress(kTurnUdpIntAddr, PROTO_UDP, false)); } if (!tcp_turn.IsNil()) { - turn_server.ports.push_back(ProtocolAddress(tcp_turn, PROTO_TCP, false)); + turn_server.ports.push_back( + ProtocolAddress(kTurnTcpIntAddr, PROTO_TCP, false)); } allocator_->AddTurnServer(turn_server); } @@ -239,8 +232,6 @@ class BasicPortAllocatorTest : public testing::Test, sid, content_name, component, ice_ufrag, ice_pwd); session->SignalPortReady.connect(this, &BasicPortAllocatorTest::OnPortReady); - session->SignalPortPruned.connect(this, - &BasicPortAllocatorTest::OnPortPruned); session->SignalCandidatesReady.connect( this, &BasicPortAllocatorTest::OnCandidatesReady); session->SignalCandidatesAllocationDone.connect( @@ -260,20 +251,6 @@ class BasicPortAllocatorTest : public testing::Test, (pattern.port() != 0 && address.port() == pattern.port())); } - // Returns the number of ports that have matching type, protocol and - // address. - static int CountPorts(const std::vector& ports, - const std::string& type, - ProtocolType protocol, - const SocketAddress& client_addr) { - return std::count_if( - ports.begin(), ports.end(), - [type, protocol, client_addr](PortInterface* port) { - return port->Type() == type && port->GetProtocol() == protocol && - port->Network()->GetBestIP() == client_addr.ipaddr(); - }); - } - // Find a candidate and return it. static bool FindCandidate(const std::vector& candidates, const std::string& type, @@ -415,15 +392,6 @@ class BasicPortAllocatorTest : public testing::Test, EXPECT_NE(ready_ports.end(), std::find(ready_ports.begin(), ready_ports.end(), port)); } - void OnPortPruned(PortAllocatorSession* ses, PortInterface* port) { - LOG(LS_INFO) << "OnPortPruned: " << port->ToString(); - ports_.erase(std::remove(ports_.begin(), ports_.end(), port), ports_.end()); - // Make sure the pruned port is not in ReadyPorts. - auto ready_ports = ses->ReadyPorts(); - EXPECT_EQ(ready_ports.end(), - std::find(ready_ports.begin(), ready_ports.end(), port)); - } - void OnCandidatesReady(PortAllocatorSession* ses, const std::vector& candidates) { for (const Candidate& candidate : candidates) { @@ -1206,145 +1174,6 @@ TEST_F(BasicPortAllocatorTest, TestSharedSocketWithoutNatUsingTurn) { EXPECT_EQ(3U, candidates_.size()); } -// Test that if prune_turn_ports is set, TCP TurnPort will not -// be used if UDP TurnPort is used. -TEST_F(BasicPortAllocatorTest, TestUdpTurnPortDisablesTcpTurnPorts) { - turn_server_.AddInternalSocket(kTurnTcpIntAddr, PROTO_TCP); - AddInterface(kClientAddr); - allocator_.reset(new BasicPortAllocator(&network_manager_)); - allocator_->SetConfiguration(allocator_->stun_servers(), - allocator_->turn_servers(), 0, true); - AddTurnServers(kTurnUdpIntAddr, kTurnTcpIntAddr); - allocator_->set_step_delay(kMinimumStepDelay); - allocator_->set_flags(allocator().flags() | - PORTALLOCATOR_ENABLE_SHARED_SOCKET | - PORTALLOCATOR_DISABLE_TCP); - - EXPECT_TRUE(CreateSession(ICE_CANDIDATE_COMPONENT_RTP)); - session_->StartGettingPorts(); - EXPECT_TRUE_WAIT(candidate_allocation_done_, kDefaultAllocationTimeout); - // Only 2 ports (one STUN and one TURN) are actually being used. - EXPECT_EQ(2U, session_->ReadyPorts().size()); - // We have verified that each port, when it is added to |ports_|, it is found - // in |ready_ports|, and when it is pruned, it is not found in |ready_ports|, - // so we only need to verify the content in one of them. - EXPECT_EQ(2U, ports_.size()); - EXPECT_EQ(1, CountPorts(ports_, "local", PROTO_UDP, kClientAddr)); - EXPECT_EQ(1, CountPorts(ports_, "relay", PROTO_UDP, kClientAddr)); - EXPECT_EQ(0, CountPorts(ports_, "relay", PROTO_TCP, kClientAddr)); - - // We don't remove candidates, so the size of |candidates_| will depend on - // when the TCP TURN port becomes ready. If it is ready after the UDP TURN - // port becomes ready, its candidates will be used there will be 3 candidates. - // Otherwise there will be only 2 candidates. - EXPECT_LE(2U, candidates_.size()); - // There will only be 2 candidates in |ready_candidates| because it only - // includes the candidates in the ready ports. - const std::vector& ready_candidates = session_->ReadyCandidates(); - EXPECT_EQ(2U, ready_candidates.size()); - EXPECT_PRED4(HasCandidate, ready_candidates, "local", "udp", kClientAddr); - EXPECT_PRED4(HasCandidate, ready_candidates, "relay", "udp", - rtc::SocketAddress(kTurnUdpExtAddr.ipaddr(), 0)); -} - -// Tests that if prune_turn_ports is set, IPv4 TurnPort will not -// be used if IPv6 TurnPort is used. -TEST_F(BasicPortAllocatorTest, TestIPv6TurnPortDisablesIPv4TurnPorts) { - turn_server_.AddInternalSocket(kTurnUdpIntIPv6Addr, PROTO_UDP); - // Add two IP addresses on the same interface. - AddInterface(kClientAddr, "net1"); - AddInterface(kClientIPv6Addr, "net1"); - allocator_.reset(new BasicPortAllocator(&network_manager_)); - allocator_->SetConfiguration(allocator_->stun_servers(), - allocator_->turn_servers(), 0, true); - AddTurnServers(kTurnUdpIntIPv6Addr, rtc::SocketAddress()); - - allocator_->set_step_delay(kMinimumStepDelay); - allocator_->set_flags(allocator().flags() | - PORTALLOCATOR_ENABLE_SHARED_SOCKET | - PORTALLOCATOR_ENABLE_IPV6 | PORTALLOCATOR_DISABLE_TCP); - - EXPECT_TRUE(CreateSession(ICE_CANDIDATE_COMPONENT_RTP)); - session_->StartGettingPorts(); - EXPECT_TRUE_WAIT(candidate_allocation_done_, kDefaultAllocationTimeout); - rtc::Thread::Current()->ProcessMessages(1000); - // Three ports (one IPv4 STUN, one IPv6 STUN and one TURN) will be ready. - EXPECT_EQ(3U, session_->ReadyPorts().size()); - EXPECT_EQ(3U, ports_.size()); - EXPECT_EQ(1, CountPorts(ports_, "local", PROTO_UDP, kClientAddr)); - EXPECT_EQ(1, CountPorts(ports_, "local", PROTO_UDP, kClientIPv6Addr)); - EXPECT_EQ(1, CountPorts(ports_, "relay", PROTO_UDP, kClientIPv6Addr)); - EXPECT_EQ(0, CountPorts(ports_, "relay", PROTO_UDP, kClientAddr)); - - // We don't remove candidates, so there may be more than 3 elemenets in - // |candidates_|, although |ready_candidates| only includes the candidates - // in |ready_ports|. - EXPECT_LE(3U, candidates_.size()); - const std::vector& ready_candidates = session_->ReadyCandidates(); - EXPECT_EQ(3U, ready_candidates.size()); - EXPECT_PRED4(HasCandidate, ready_candidates, "local", "udp", kClientAddr); - EXPECT_PRED4(HasCandidate, ready_candidates, "relay", "udp", - rtc::SocketAddress(kTurnUdpExtAddr.ipaddr(), 0)); -} - -// Tests that if prune_turn_ports is set, each network interface -// will has its own set of TurnPorts based on their priorities. -TEST_F(BasicPortAllocatorTest, TestEachInterfaceHasItsOwnTurnPorts) { - turn_server_.AddInternalSocket(kTurnTcpIntAddr, PROTO_TCP); - turn_server_.AddInternalSocket(kTurnUdpIntIPv6Addr, PROTO_UDP); - turn_server_.AddInternalSocket(kTurnTcpIntIPv6Addr, PROTO_TCP); - // Add two interfaces both having IPv4 and IPv6 addresses. - AddInterface(kClientAddr, "net1"); - AddInterface(kClientIPv6Addr, "net1"); - AddInterface(kClientAddr2, "net2"); - AddInterface(kClientIPv6Addr2, "net2"); - allocator_.reset(new BasicPortAllocator(&network_manager_)); - allocator_->SetConfiguration(allocator_->stun_servers(), - allocator_->turn_servers(), 0, true); - // Have both UDP/TCP and IPv4/IPv6 TURN ports. - AddTurnServers(kTurnUdpIntAddr, kTurnTcpIntAddr); - AddTurnServers(kTurnUdpIntIPv6Addr, kTurnTcpIntIPv6Addr); - - allocator_->set_step_delay(kMinimumStepDelay); - allocator_->set_flags(allocator().flags() | - PORTALLOCATOR_ENABLE_SHARED_SOCKET | - PORTALLOCATOR_ENABLE_IPV6); - EXPECT_TRUE(CreateSession(ICE_CANDIDATE_COMPONENT_RTP)); - session_->StartGettingPorts(); - EXPECT_TRUE_WAIT(candidate_allocation_done_, kDefaultAllocationTimeout); - // 10 ports (4 STUN and 1 TURN ports on each interface) will be ready to use. - EXPECT_EQ(10U, session_->ReadyPorts().size()); - EXPECT_EQ(10U, ports_.size()); - EXPECT_EQ(1, CountPorts(ports_, "local", PROTO_UDP, kClientAddr)); - EXPECT_EQ(1, CountPorts(ports_, "local", PROTO_UDP, kClientAddr2)); - EXPECT_EQ(1, CountPorts(ports_, "local", PROTO_UDP, kClientIPv6Addr)); - EXPECT_EQ(1, CountPorts(ports_, "local", PROTO_UDP, kClientIPv6Addr2)); - EXPECT_EQ(1, CountPorts(ports_, "local", PROTO_TCP, kClientAddr)); - EXPECT_EQ(1, CountPorts(ports_, "local", PROTO_TCP, kClientAddr2)); - EXPECT_EQ(1, CountPorts(ports_, "local", PROTO_TCP, kClientIPv6Addr)); - EXPECT_EQ(1, CountPorts(ports_, "local", PROTO_TCP, kClientIPv6Addr2)); - EXPECT_EQ(1, CountPorts(ports_, "relay", PROTO_UDP, kClientIPv6Addr)); - EXPECT_EQ(1, CountPorts(ports_, "relay", PROTO_UDP, kClientIPv6Addr2)); - - // We don't remove candidates, so there may be more than 10 candidates - // in |candidates_|. - EXPECT_LE(10U, candidates_.size()); - const std::vector& ready_candidates = session_->ReadyCandidates(); - EXPECT_EQ(10U, ready_candidates.size()); - EXPECT_PRED4(HasCandidate, ready_candidates, "local", "udp", kClientAddr); - EXPECT_PRED4(HasCandidate, ready_candidates, "local", "udp", kClientAddr2); - EXPECT_PRED4(HasCandidate, ready_candidates, "local", "udp", kClientIPv6Addr); - EXPECT_PRED4(HasCandidate, ready_candidates, "local", "udp", - kClientIPv6Addr2); - EXPECT_PRED4(HasCandidate, ready_candidates, "local", "tcp", kClientAddr); - EXPECT_PRED4(HasCandidate, ready_candidates, "local", "tcp", kClientAddr2); - EXPECT_PRED4(HasCandidate, ready_candidates, "local", "tcp", kClientIPv6Addr); - EXPECT_PRED4(HasCandidate, ready_candidates, "local", "tcp", - kClientIPv6Addr2); - EXPECT_PRED4(HasCandidate, ready_candidates, "relay", "udp", - rtc::SocketAddress(kTurnUdpExtAddr.ipaddr(), 0)); -} - // Testing DNS resolve for the TURN server, this will test AllocationSequence // handling the unresolved address signal from TurnPort. TEST_F(BasicPortAllocatorTest, TestSharedSocketWithServerAddressResolve) { @@ -1640,7 +1469,7 @@ TEST_F(BasicPortAllocatorTest, TestTransportInformationUpdated) { AddInterface(kClientAddr); int pool_size = 1; allocator_->SetConfiguration(allocator_->stun_servers(), - allocator_->turn_servers(), pool_size, false); + allocator_->turn_servers(), pool_size); const PortAllocatorSession* peeked_session = allocator_->GetPooledSession(); ASSERT_NE(nullptr, peeked_session); EXPECT_EQ_WAIT(true, peeked_session->CandidatesAllocationDone(), @@ -1676,7 +1505,7 @@ TEST_F(BasicPortAllocatorTest, TestSetCandidateFilterAfterCandidatesGathered) { AddInterface(kClientAddr); int pool_size = 1; allocator_->SetConfiguration(allocator_->stun_servers(), - allocator_->turn_servers(), pool_size, false); + allocator_->turn_servers(), pool_size); const PortAllocatorSession* peeked_session = allocator_->GetPooledSession(); ASSERT_NE(nullptr, peeked_session); EXPECT_EQ_WAIT(true, peeked_session->CandidatesAllocationDone(),