Add config to prune TURN ports

webrtc/p2p/client/basicportallocator.cc

Index: webrtc/p2p/client/basicportallocator.cc
diff --git a/webrtc/p2p/client/basicportallocator.cc b/webrtc/p2p/client/basicportallocator.cc
index e39a44013cdcf4f5b4cf627e225c499afb1e1687..2fc6e89aff3d65a47ebad4ea3e1ae3f3b43d0d8c 100644
--- a/webrtc/p2p/client/basicportallocator.cc
+++ b/webrtc/p2p/client/basicportallocator.cc
@@ -47,6 +47,38 @@ 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 {
@@ -74,7 +106,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<RelayServerConfig>(), 0);
+  SetConfiguration(stun_servers, std::vector<RelayServerConfig>(), 0, false);
   Construct();
 }
 
@@ -101,7 +133,7 @@ BasicPortAllocator::BasicPortAllocator(
     turn_servers.push_back(config);
   }
 
-  SetConfiguration(stun_servers, turn_servers, 0);
+  SetConfiguration(stun_servers, turn_servers, 0, false);
   Construct();
 }
 
@@ -122,7 +154,8 @@ PortAllocatorSession* BasicPortAllocator::CreateSessionInternal(
 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
@@ -217,9 +250,9 @@ void BasicPortAllocatorSession::ClearGettingPorts() {
 
 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;
@@ -228,6 +261,10 @@ std::vector<PortInterface*> BasicPortAllocatorSession::ReadyPorts() const {
 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;
@@ -278,16 +315,15 @@ bool BasicPortAllocatorSession::CandidatesAllocationDone() const {
     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) {
@@ -357,7 +393,7 @@ void BasicPortAllocatorSession::OnConfigStop() {
   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();
@@ -443,11 +479,8 @@ 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
@@ -569,36 +602,88 @@ void BasicPortAllocatorSession::OnCandidateReady(
   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) {
@@ -607,7 +692,7 @@ void BasicPortAllocatorSession::OnPortComplete(Port* port) {
   ASSERT(data != NULL);
 
   // Ignore any late signals.
-  if (data->complete() || data->error()) {
+  if (data->terminated()) {
     return;
   }
 
@@ -622,7 +707,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->complete() || data->error()) {
+  if (data->terminated()) {
     return;
   }
 
@@ -1028,13 +1113,11 @@ void AllocationSequence::CreateRelayPorts() {
     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);
     }