Ping the premier connection on each network with higher priority.

webrtc/p2p/base/p2ptransportchannel.cc

Index: webrtc/p2p/base/p2ptransportchannel.cc
diff --git a/webrtc/p2p/base/p2ptransportchannel.cc b/webrtc/p2p/base/p2ptransportchannel.cc
index 3f68d6d6e078fd635e8f696b3b46ae12bae8ce67..25bcebebcebd8f8407611c086986adb5a5df8fca 100644
--- a/webrtc/p2p/base/p2ptransportchannel.cc
+++ b/webrtc/p2p/base/p2ptransportchannel.cc
@@ -1248,6 +1248,8 @@ void P2PTransportChannel::SortConnectionsAndUpdateState() {
   // have higher priority if it is writable.
   MaybeSwitchSelectedConnection(top_connection, "sorting");
 
+  UpdatePremierConnections();
+
   // The controlled side can prune only if the selected connection has been
   // nominated because otherwise it may prune the connection that will be
   // selected by the controlling side.
@@ -1285,6 +1287,21 @@ void P2PTransportChannel::SortConnectionsAndUpdateState() {
   MaybeStartPinging();
 }
 
+void P2PTransportChannel::UpdatePremierConnections() {
+  // |connections_| has been sorted, so the first one in the list on a given
+  // network is the premier connection, except that the selected connection
+  // is always the premier connection on the network.
+  premier_connection_by_network_name_.clear();
+  for (Connection* conn : connections_) {
+    const std::string& network_name = conn->port()->Network()->name();
+    if (conn == selected_connection_ ||
+        premier_connection_by_network_name_.find(network_name) ==
+            premier_connection_by_network_name_.end()) {
+      premier_connection_by_network_name_[network_name] = conn;
+    }
+  }
+}
+
 void P2PTransportChannel::PruneConnections() {
   // We can prune any connection for which there is a connected, writable
   // connection on the same network with better or equal priority.  We leave
@@ -1295,25 +1312,16 @@ void P2PTransportChannel::PruneConnections() {
   // switch. If the |premier| connection is not connected, we may be
   // reconnecting a TCP connection and temporarily do not prune connections in
   // this network. See the big comment in CompareConnectionStates.
-
-  // Get a list of the networks that we are using.
-  std::set<rtc::Network*> networks;
-  for (const Connection* conn : connections_) {
-    networks.insert(conn->port()->Network());
-  }
-  for (rtc::Network* network : networks) {
-    Connection* premier = GetBestConnectionOnNetwork(network);
-    // Do not prune connections if the current selected connection is weak on
+  for (Connection* conn : connections_) {
+    // Do not prune connections if the current premier connection is weak on
     // this network. Otherwise, it may delete connections prematurely.
-    if (!premier || premier->weak()) {
+    Connection* premier =
+        premier_connection_by_network_name_[conn->port()->Network()->name()];
+    if (!premier || conn == premier || premier->weak()) {
       continue;
     }
-
-    for (Connection* conn : connections_) {
-      if ((conn != premier) && (conn->port()->Network() == network) &&
-          (CompareConnectionCandidates(premier, conn) >= 0)) {
-        conn->Prune();
-      }
+    if (CompareConnectionCandidates(premier, conn) >= 0) {
+      conn->Prune();
     }
   }
 }
@@ -1451,26 +1459,6 @@ bool P2PTransportChannel::ReadyToSend(Connection* connection) const {
           PresumedWritable(connection));
 }
 
-// If we have a selected connection, return it, otherwise return top one in the
-// list (later we will mark it best).
-Connection* P2PTransportChannel::GetBestConnectionOnNetwork(
-    rtc::Network* network) const {
-  // If the selected connection is on this network, then it wins.
-  if (selected_connection_ &&
-      (selected_connection_->port()->Network() == network)) {
-    return selected_connection_;
-  }
-
-  // Otherwise, we return the top-most in sorted order.
-  for (size_t i = 0; i < connections_.size(); ++i) {
-    if (connections_[i]->port()->Network() == network) {
-      return connections_[i];
-    }
-  }
-
-  return NULL;
-}
-
 // Handle any queued up requests
 void P2PTransportChannel::OnMessage(rtc::Message *pmsg) {
   switch (pmsg->message_id) {
@@ -1548,19 +1536,26 @@ bool P2PTransportChannel::IsPingable(const Connection* conn,
     return false;
   }
 
-  // If the channel is weakly connected, ping all connections.
-  if (weak()) {
-    return true;
-  }
-
-  // Always ping active connections regardless whether the channel is completed
-  // or not, but backup connections are pinged at a slower rate.
-  if (IsBackupConnection(conn)) {
+  // Backup connections are pinged at a slower rate if the channel is strongly
+  // connected.
+  bool strongly_connected = !weak();
+  if (strongly_connected && IsBackupConnection(conn)) {
     return conn->rtt_samples() == 0 ||
            (now >= conn->last_ping_response_received() +
                        config_.backup_connection_ping_interval);
   }
-  // Don't ping inactive non-backup connections.
+
+  // If the premier connection is weakly connected, ping all connections on

[Taylor Brandstetter, 2016/10/04 02:08:10]

Suggest changing this to "If the premier connection on a network is weakly
connected..." to make it clear there are multiple premier connections.

[honghaiz3, 2016/10/20 23:47:58]

Done.

+  // the same network. Otherwise, do not ping those with a lower priority
+  // (i.e., those having been pruned) or those which have timed out.
+  auto iter =
+      premier_connection_by_network_name_.find(conn->port()->Network()->name());
+  if (iter == premier_connection_by_network_name_.end() || !iter->second ||
+      iter->second->weak()) {
+    return true;
+  }
+
+  // Don't ping inactive connections.
   if (!conn->active()) {
     return false;
   }
@@ -1576,14 +1571,41 @@ bool P2PTransportChannel::IsPingable(const Connection* conn,
   return (now >= conn->last_ping_sent() + ping_interval);
 }
 
-bool P2PTransportChannel::IsSelectedConnectionPingable(int64_t now) {
-  if (!selected_connection_ || !selected_connection_->connected() ||
-      !selected_connection_->writable()) {
+bool P2PTransportChannel::IsPremierConnectionPingable(
+    Connection* premier_connection,
+    int64_t now) {
+  if (!premier_connection || !premier_connection->connected() ||
+      !premier_connection->writable() ||
+      premier_connection->state() == Connection::STATE_FAILED) {

[Taylor Brandstetter, 2016/10/04 02:08:09]

Why did the "failed" check not exist before?

[honghaiz3, 2016/10/20 23:47:58]

It is logical to include this although it does not matter here
because whenever we set a connection to STATE_FAILED, we also mark it as
WRITE_TIMEOUT. Revert this change to avoid the confusion. 

     return false;
   }
 
-  int interval = CalculateActiveWritablePingInterval(selected_connection_, now);
-  return selected_connection_->last_ping_sent() + interval <= now;
+  int interval = CalculateActiveWritablePingInterval(premier_connection, now);
+  return premier_connection->last_ping_sent() + interval <= now;
+}
+
+Connection* P2PTransportChannel::FindBestPremierConnectionToPing(int64_t now) {
+  // If the selected connection is pingable, select it to ping.
+  if (IsPremierConnectionPingable(selected_connection_, now)) {
+    return selected_connection_;
+  }
+
+  // Otherwise, find the premier connection that has not been pinged for the
+  // longest time.
+  Connection* oldest_premier_connection = nullptr;
+  for (auto kv : premier_connection_by_network_name_) {
+    Connection* conn = kv.second;
+    if (conn == nullptr || conn == selected_connection_) {
+      continue;
+    }
+    if (IsPremierConnectionPingable(conn, now) &&
+        (!oldest_premier_connection ||
+         conn->last_ping_sent() <
+             oldest_premier_connection->last_ping_sent())) {
+      oldest_premier_connection = conn;
+    }
+  }
+  return oldest_premier_connection;
 }
 
 int P2PTransportChannel::CalculateActiveWritablePingInterval(
@@ -1598,8 +1620,9 @@ int P2PTransportChannel::CalculateActiveWritablePingInterval(
   int stable_interval = config_.stable_writable_connection_ping_interval;
   int stablizing_interval =
       std::min(stable_interval, STABILIZING_WRITABLE_CONNECTION_PING_INTERVAL);
-
-  return conn->stable(now) ? stable_interval : stablizing_interval;
+  // If the channel is weak or the connection is not stable yet, use the
+  // stablizing_interval.
+  return (!weak() && conn->stable(now)) ? stable_interval : stablizing_interval;

[Taylor Brandstetter, 2016/10/04 02:08:09]

So this is now the "stabilizing or weak" interval? I think that needs to be made
clear. Maybe a new interval should be introduced.

[honghaiz3, 2016/10/20 23:47:58]

Changed to weak_or_stablizing_interval. 

 }
 
 // Returns the next pingable connection to ping.  This will be the oldest
@@ -1611,10 +1634,8 @@ int P2PTransportChannel::CalculateActiveWritablePingInterval(
 // CompareConnectionStates.
 Connection* P2PTransportChannel::FindNextPingableConnection() {
   int64_t now = rtc::TimeMillis();
-  Connection* conn_to_ping = nullptr;
-  if (IsSelectedConnectionPingable(now)) {
-    conn_to_ping = selected_connection_;
-  } else {
+  Connection* conn_to_ping = FindBestPremierConnectionToPing(now);
+  if (!conn_to_ping) {
     conn_to_ping = FindConnectionToPing(now);
   }
   return conn_to_ping;
@@ -1728,9 +1749,16 @@ void P2PTransportChannel::OnConnectionDestroyed(Connection* connection) {
   pinged_connections_.erase(*iter);
   unpinged_connections_.erase(*iter);
   connections_.erase(iter);
+  for (auto kv : premier_connection_by_network_name_) {
+    if (kv.second == connection) {
+      kv.second = nullptr;
+    }
+  }
 
-  LOG_J(LS_INFO, this) << "Removed connection ("
-    << static_cast<int>(connections_.size()) << " remaining)";
+  LOG_J(LS_INFO, this) << "Removed connection " << std::hex << connection

[Taylor Brandstetter, 2016/10/04 02:08:10]

Why not log connection->ToString()?

[honghaiz3, 2016/10/20 23:47:58]

connection->ToString() needs to access the members in the respective port. There
is potential risk that the port may also be in the process of deleting itself
and the members are partially released. 
Plus, it is probably sufficient to know which connection is removed at this
point.

[Taylor Brandstetter, 2016/10/21 20:18:49]

Does that risk really exist? I thought a Port is only destroyed asynchronously,
after all its connections are destroyed.

[honghaiz3, 2016/10/21 21:27:26]

It probably does not although it would require careful analysis of the code. 
I once got caught when I tried to print out the whole information of a
Connection when it is about to be deleted and it caused some tests flaky after
the CL was landed. 
It is not super important to print out the complete information here.  
It is probably sufficient to know which connection was deleted at this point for
the purpose of debugging.  

+                       << std::dec << " ("
+                       << static_cast<int>(connections_.size())
+                       << " remaining)";
 
   // If this is currently the selected connection, then we need to pick a new
   // one. The call to SortConnectionsAndUpdateState will pick a new one. It
@@ -1858,7 +1886,7 @@ void P2PTransportChannel::OnReadyToSend(Connection* connection) {
 
 // Find "triggered checks".  We ping first those connections that have
 // received a ping but have not sent a ping since receiving it
-// (last_received_ping > last_sent_ping).  But we shouldn't do
+// (last_ping_received > last_ping_sent).  But we shouldn't do
 // triggered checks if the connection is already writable.
 Connection* P2PTransportChannel::FindOldestConnectionNeedingTriggeredCheck(
     int64_t now) {