Add config to prune TURN ports

webrtc/p2p/client/basicportallocator.cc

 /*
  *  Copyright 2004 The WebRTC Project Authors. All rights reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 29 matching lines @@
   MSG_CONFIG_STOP,
 };
 
 const int PHASE_UDP = 0;
 const int PHASE_RELAY = 1;
 const int PHASE_TCP = 2;
 const int PHASE_SSLTCP = 3;
 
 const int kNumPhases = 4;
 
+// Gets address family priority:  Unspecified < IPv4 < IPv6.
+int GetAddressFamilyPriority(int ip_family) {
+  switch (ip_family) {
+    case AF_UNSPEC:
+      return 0;
+    case AF_INET:
+      return 1;
+    case AF_INET6:
+      return 2;
+    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;
+  }

[pthatcher1, 2016/06/28 21:01:41]

I think it makes sense to prefer UDP > TCP, but I still don't see what we gain
by preferring ipv4 over ipv6 (or vice versa).

[honghaiz3, 2016/06/28 23:36:00]

keep this based on our discussion. 

+
+  int a_family = GetAddressFamilyPriority(a->Network()->GetBestIP().family());
+  int b_family = GetAddressFamilyPriority(b->Network()->GetBestIP().family());
+  return a_family - b_family;
+}
+
 }  // namespace
 
 namespace cricket {
 const uint32_t DISABLE_ALL_PHASES =
     PORTALLOCATOR_DISABLE_UDP | PORTALLOCATOR_DISABLE_TCP |
     PORTALLOCATOR_DISABLE_STUN | PORTALLOCATOR_DISABLE_RELAY;
 
 // BasicPortAllocator
 BasicPortAllocator::BasicPortAllocator(rtc::NetworkManager* network_manager,
                                        rtc::PacketSocketFactory* socket_factory)
     : network_manager_(network_manager), socket_factory_(socket_factory) {
   ASSERT(network_manager_ != nullptr);
   ASSERT(socket_factory_ != nullptr);
   Construct();
 }
 
 BasicPortAllocator::BasicPortAllocator(rtc::NetworkManager* network_manager)
     : network_manager_(network_manager), socket_factory_(nullptr) {
   ASSERT(network_manager_ != nullptr);
   Construct();
 }
 
 BasicPortAllocator::BasicPortAllocator(rtc::NetworkManager* network_manager,
                                        rtc::PacketSocketFactory* socket_factory,
                                        const ServerAddresses& stun_servers)
     : network_manager_(network_manager), socket_factory_(socket_factory) {
   ASSERT(socket_factory_ != NULL);
-  SetConfiguration(stun_servers, std::vector<RelayServerConfig>(), 0);
+  SetConfiguration(stun_servers, std::vector<RelayServerConfig>(), 0, false);
   Construct();
 }
 
 BasicPortAllocator::BasicPortAllocator(
     rtc::NetworkManager* network_manager,
     const ServerAddresses& stun_servers,
     const rtc::SocketAddress& relay_address_udp,
     const rtc::SocketAddress& relay_address_tcp,
     const rtc::SocketAddress& relay_address_ssl)
     : network_manager_(network_manager), socket_factory_(NULL) {
   std::vector<RelayServerConfig> turn_servers;
   RelayServerConfig config(RELAY_GTURN);
   if (!relay_address_udp.IsNil()) {
     config.ports.push_back(ProtocolAddress(relay_address_udp, PROTO_UDP));
   }
   if (!relay_address_tcp.IsNil()) {
     config.ports.push_back(ProtocolAddress(relay_address_tcp, PROTO_TCP));
   }
   if (!relay_address_ssl.IsNil()) {
     config.ports.push_back(ProtocolAddress(relay_address_ssl, PROTO_SSLTCP));
   }
 
   if (!config.ports.empty()) {
     turn_servers.push_back(config);
   }
 
-  SetConfiguration(stun_servers, turn_servers, 0);
+  SetConfiguration(stun_servers, turn_servers, 0, false);
   Construct();
 }
 
 void BasicPortAllocator::Construct() {
   allow_tcp_listen_ = true;
 }
 
 BasicPortAllocator::~BasicPortAllocator() {
 }
 
 PortAllocatorSession* BasicPortAllocator::CreateSessionInternal(
     const std::string& content_name, int component,
     const std::string& ice_ufrag, const std::string& ice_pwd) {
   return new BasicPortAllocatorSession(
       this, content_name, component, ice_ufrag, ice_pwd);
 }
 
 void BasicPortAllocator::AddTurnServer(const RelayServerConfig& turn_server) {
   std::vector<RelayServerConfig> new_turn_servers = turn_servers();
   new_turn_servers.push_back(turn_server);
-  SetConfiguration(stun_servers(), new_turn_servers, candidate_pool_size());
+  SetConfiguration(stun_servers(), new_turn_servers, candidate_pool_size(),
+                   prune_turn_ports());
 }
 
 // BasicPortAllocatorSession
 BasicPortAllocatorSession::BasicPortAllocatorSession(
     BasicPortAllocator *allocator,
     const std::string& content_name,
     int component,
     const std::string& ice_ufrag,
     const std::string& ice_pwd)
     : PortAllocatorSession(content_name, component,
@@ skipping 74 matching lines @@
 }
 
 void BasicPortAllocatorSession::ClearGettingPorts() {
   network_thread_->Clear(this, MSG_ALLOCATE);
   for (uint32_t i = 0; i < sequences_.size(); ++i)
     sequences_[i]->Stop();
 }
 
 std::vector<PortInterface*> BasicPortAllocatorSession::ReadyPorts() const {
   std::vector<PortInterface*> ret;
-  for (const PortData& port : ports_) {
-    if (port.has_pairable_candidate() && !port.error()) {
-      ret.push_back(port.port());
+  for (const PortData& data : ports_) {
+    if (IsPortInUse(data)) {

[pthatcher1, 2016/06/28 21:01:41]

Since the existing port is ReadyPorts, why not calls this PortIsReady()?  And,
in fact, why not just put it on PortData as .ready()?

[honghaiz3, 2016/06/28 23:36:00]

Done.

+      ret.push_back(data.port());
     }
   }
   return ret;
 }
 
 std::vector<Candidate> BasicPortAllocatorSession::ReadyCandidates() const {
   std::vector<Candidate> candidates;
   for (const PortData& data : ports_) {
+    if (!IsPortInUse(data)) {
+      continue;
+    }
+
     for (const Candidate& candidate : data.port()->Candidates()) {
       if (!CheckCandidateFilter(candidate)) {
         continue;
       }
       ProtocolType pvalue;
       if (!StringToProto(candidate.protocol().c_str(), &pvalue) ||
           !data.sequence()->ProtocolEnabled(pvalue)) {
         continue;
       }
       candidates.push_back(SanitizeRelatedAddress(candidate));
@@ skipping 30 matching lines @@
   }
 
   // Check that all port allocation sequences are complete (not running).
   if (std::any_of(sequences_.begin(), sequences_.end(),
                   [](const AllocationSequence* sequence) {
                     return sequence->state() == AllocationSequence::kRunning;
                   })) {
     return false;
   }
 
-  // If all allocated ports are in complete state, session must have got all
-  // expected candidates. Session will trigger candidates allocation complete
-  // signal.
-  if (!std::all_of(ports_.begin(), ports_.end(), [](const PortData& port) {
-        return (port.complete() || port.error());
-      })) {
-    return false;
+  // If all allocated ports are in one of the terminal states, session must have

[pthatcher1, 2016/06/28 21:01:41]

Could say "if all allocated ports are no longer gathering".

[honghaiz3, 2016/06/28 23:36:00]

Done.

+  // got all expected candidates. Session will trigger candidates allocation
+  // complete signal.
+  if (std::all_of(ports_.begin(), ports_.end(),

[pthatcher1, 2016/06/28 21:01:41]

Could be "std::none_of(ports_.... port.gathering();} )"

[honghaiz3, 2016/06/28 23:36:00]

Done used any_of ... May be more readable. 

+                  [](const PortData& port) { return port.terminated(); })) {
+    return true;
   }
 
-  return true;
+  return false;

[pthatcher1, 2016/06/28 21:01:41]

Instead of 

if(foo)
  return true;
return false;

Why not

return foo;

?

[honghaiz3, 2016/06/28 23:36:00]

Done.

 }
 
 void BasicPortAllocatorSession::OnMessage(rtc::Message *message) {
   switch (message->message_id) {
   case MSG_CONFIG_START:
     ASSERT(rtc::Thread::Current() == network_thread_);
     GetPortConfigurations();
     break;
   case MSG_CONFIG_READY:
     ASSERT(rtc::Thread::Current() == network_thread_);
@@ skipping 49 matching lines @@
 
 void BasicPortAllocatorSession::OnConfigStop() {
   ASSERT(rtc::Thread::Current() == network_thread_);
 
   // If any of the allocated ports have not completed the candidates allocation,
   // mark those as error. Since session doesn't need any new candidates
   // at this stage of the allocation, it's safe to discard any new candidates.
   bool send_signal = false;
   for (std::vector<PortData>::iterator it = ports_.begin();
        it != ports_.end(); ++it) {
-    if (!it->complete() && !it->error()) {
+    if (!it->terminated()) {

[pthatcher1, 2016/06/28 21:01:41]

it->gathering()

?

[honghaiz3, 2016/06/28 23:36:00]

Done.

       // Updating port state to error, which didn't finish allocating candidates
       // yet.
       it->set_error();
       send_signal = true;
     }
   }
 
   // Did we stop any running sequences?
   for (std::vector<AllocationSequence*>::iterator it = sequences_.begin();
        it != sequences_.end() && !send_signal; ++it) {
@@ skipping 65 matching lines @@
 // create a new sequence to create the appropriate ports.
 void BasicPortAllocatorSession::DoAllocate() {
   bool done_signal_needed = false;
   std::vector<rtc::Network*> networks;
   GetNetworks(&networks);
 
   if (networks.empty()) {
     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
         // done event and return.
         done_signal_needed = true;
         break;
       }
 
       if (!config || config->relays.empty()) {
         // No relay ports specified in this config.
@@ skipping 101 matching lines @@
   MaybeSignalCandidatesAllocationDone();
 }
 
 void BasicPortAllocatorSession::OnCandidateReady(
     Port* port, const Candidate& c) {
   ASSERT(rtc::Thread::Current() == network_thread_);
   PortData* data = FindPort(port);
   ASSERT(data != NULL);
   // Discarding any candidate signal if port allocation status is
   // already in completed state.
-  if (data->complete() || data->error()) {
+  if (data->terminated()) {
     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: Need to do MaybeSignalCandidateReady after this because we will
+  // check whether the candidate is generated by the port in use.
+  if (!data->has_pairable_candidate() && CandidatePairable(c, port)) {
+    OnPortPairable(data);

[pthatcher1, 2016/06/28 21:01:41]

We should keep has_pairable_candidate and set_has_pairable_candidate(true) next
to each other.  So perhaps make this:

if (CandidatePairable(c, port)) {
  OnPortPairable(data);
}

And then:

void BasicPortAllocatorSession::OnPortPairable(PortData* port_data) {
  if (port_data->has_pairable_candidate()) {
    return;
  }
  port_data->set_has_pairable_candidate(true);

[honghaiz3, 2016/06/28 23:36:00]

Done.

+  }
+
   ProtocolType pvalue;
   bool candidate_protocol_enabled =
       StringToProto(c.protocol().c_str(), &pvalue) &&
       data->sequence()->ProtocolEnabled(pvalue);
 
-  if (CheckCandidateFilter(c) && candidate_protocol_enabled) {
+  if (CheckCandidateFilter(c) && candidate_protocol_enabled &&
+      IsPortInUse(*data)) {
     std::vector<Candidate> candidates;
     candidates.push_back(SanitizeRelatedAddress(c));
     SignalCandidatesReady(this, candidates);
   }
+}
 
-  // Port has already been marked as having a pairable candidate.
-  // Nothing to do here.
-  if (data->has_pairable_candidate()) {
+void BasicPortAllocatorSession::OnPortPairable(PortData* port_data) {

[pthatcher1, 2016/06/28 21:01:41]

We could call it "data" or "port_data" consistenly throughout the file.  I'm
fine either way, but please make it consistent.

[honghaiz3, 2016/06/28 23:36:00]

Now that I have used helper methods below, it is possible to just use data
without getting the name shadowed. 

+  port_data->set_has_pairable_candidate(true);
+  if (port_data->pruned()) {
+    // A port may have been pruned before it becomes ready. In that case,
+    // do not need to do anything here.
     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 (CandidatePairable(c, port)) {
-    data->set_has_pairable_candidate(true);
+  Port* port = port_data->port();
+  if (!allocator_->prune_turn_ports() || port->Type() != RELAY_PORT_TYPE) {
     SignalPortReady(this, port);
+    return;
+  }
+
+  // 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.
+  Port* best_turn_port_on_same_network = nullptr;
+  for (const PortData& data : ports_) {
+    if (data.port()->Network()->name() == port->Network()->name() &&
+        data.port()->Type() == RELAY_PORT_TYPE && IsPortInUse(data) &&
+        (!best_turn_port_on_same_network ||
+         ComparePort(data.port(), best_turn_port_on_same_network) > 0)) {
+      best_turn_port_on_same_network = data.port();
+    }
+  }

[pthatcher1, 2016/06/28 21:01:41]

Can you move this into a helper method that can be called like this?

Port* best_turn_port_on_same_network =
GetBestTurnPortForNetwork(port->Network()->name());

[honghaiz3, 2016/06/28 23:36:00]

Done.

+  // |port| is already in the list of ports, so the best port cannot be nullptr.
+  RTC_DCHECK(best_turn_port_on_same_network != nullptr);
+  if (ComparePort(port, best_turn_port_on_same_network) >= 0) {
+    SignalPortReady(this, port);
+  }
+  // Prune existing ports that have lower priorities.
+  bool pruned_port = false;
+  for (PortData& data : ports_) {
+    if (data.port()->Network()->name() == port->Network()->name() &&
+        data.port()->Type() == RELAY_PORT_TYPE && !data.pruned() &&
+        ComparePort(data.port(), best_turn_port_on_same_network) < 0) {
+      data.set_pruned();
+      pruned_port = true;
+      // Don't need to send SignalPortPruned for the current port because it was
+      // not signaled for being ready.
+      if (data.port() != port) {
+        SignalPortPruned(this, data.port());
+      }
+    }
+  }

[pthatcher1, 2016/06/28 21:01:41]

Can you move this into a helper method that can be called like this?

PrunePortsWorseThan(best_turn_port_on_same_network);

[honghaiz3, 2016/06/28 23:36:00]

Done.

+  if (pruned_port) {
+    MaybeSignalCandidatesAllocationDone();

[pthatcher1, 2016/06/28 21:01:41]

Shouldn't these just be part of the if block above?

if (ComparePort(port, best_turn_port_on_same_network) >= 0) {
  SignalPortReady(this, port);
} else {
  port_data.set_pruned();
  SignalPortPruned(this, port);
  MaybeSignalCandidatesAllocationDone();
}

Also, shouldn't we "MaybeSignalCandidatesAllocationDone()" when we
data.set_pruned() in the for loop?

[honghaiz3, 2016/06/28 23:36:00]

Yes we can do some optimization like that, although we don't need to signal if
the current port is pruned. 
If we do that way, We may end up calling MaybeSignalCandidatesAllocationDone
multiple times, which is not necessary, I think. 

   }
 }
 
+bool BasicPortAllocatorSession::IsPortInUse(const PortData& data) const {
+  return data.has_pairable_candidate() && !data.pruned() && !data.error();
+}
+
 void BasicPortAllocatorSession::OnPortComplete(Port* port) {
   ASSERT(rtc::Thread::Current() == network_thread_);
   PortData* data = FindPort(port);
   ASSERT(data != NULL);
 
   // Ignore any late signals.
-  if (data->complete() || data->error()) {
+  if (data->terminated()) {
     return;
   }
 
   // Moving to COMPLETE state.
   data->set_complete();
   // Send candidate allocation complete signal if this was the last port.
   MaybeSignalCandidatesAllocationDone();
 }
 
 void BasicPortAllocatorSession::OnPortError(Port* port) {
   ASSERT(rtc::Thread::Current() == network_thread_);
   PortData* data = FindPort(port);
   ASSERT(data != NULL);
   // We might have already given up on this port and stopped it.
-  if (data->complete() || data->error()) {
+  if (data->terminated()) {
     return;
   }
 
   // SignalAddressError is currently sent from StunPort/TurnPort.
   // But this signal itself is generic.
   data->set_error();
   // Send candidate allocation complete signal if this was the last port.
   MaybeSignalCandidatesAllocationDone();
 }
 
@@ skipping 385 matching lines @@
 
   // If BasicPortAllocatorSession::OnAllocate left relay ports enabled then we
   // ought to have a relay list for them here.
   ASSERT(config_ && !config_->relays.empty());
   if (!(config_ && !config_->relays.empty())) {
     LOG(LS_WARNING)
         << "AllocationSequence: No relay server configured, skipping.";
     return;
   }
 
-  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);
+  for (RelayServerConfig& relay : config_->relays) {
+    if (relay.type == RELAY_GTURN) {
+      CreateGturnPort(relay);
+    } else if (relay.type == RELAY_TURN) {
+      CreateTurnPort(relay);
     } else {
       ASSERT(false);
     }
   }
 }
 
 void AllocationSequence::CreateGturnPort(const RelayServerConfig& config) {
   // TODO(mallinath) - Rename RelayPort to GTurnPort.
   RelayPort* port = RelayPort::Create(session_->network_thread(),
                                       session_->socket_factory(),
@@ skipping 187 matching lines @@
   ServerAddresses servers;
   for (size_t i = 0; i < relays.size(); ++i) {
     if (relays[i].type == turn_type && SupportsProtocol(relays[i], type)) {
       servers.insert(relays[i].ports.front().address);
     }
   }
   return servers;
 }
 
 }  // namespace cricket