Use continual gathering to restore backup connections

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 fc1f0a4e6769ee96ce28f851d0f7cbc368fbf69c..4b16015d975a60fb2a4a91cd6d46df8cf9abc23f 100644
--- a/webrtc/p2p/base/p2ptransportchannel.cc
+++ b/webrtc/p2p/base/p2ptransportchannel.cc
@@ -27,10 +27,15 @@
 namespace {
 
 // messages for queuing up work for ourselves
-enum { MSG_SORT = 1, MSG_CHECK_AND_PING };
+enum {
+  MSG_SORT = 1,
+  MSG_CHECK_AND_PING,
+  MSG_CHECK_RESTORE_BACKUP_CONNECTION,
+  MSG_SIGNAL_CANDIDATES_REMOVED
+};
 
 // The minimum improvement in RTT that justifies a switch.
-static const double kMinImprovement = 10;
+const double kMinImprovement = 10;
 
 bool IsRelayRelay(cricket::Connection* conn) {
   return conn->local_candidate().type() == cricket::RELAY_PORT_TYPE &&
@@ -224,6 +229,10 @@ static const int MAX_CURRENT_STRONG_INTERVAL = 900;  // ms
 
 static const int MIN_CHECK_RECEIVING_INTERVAL = 50;  // ms
 
+// We periodically check and restore backup connections on existing networks
+// that do not have any connection at this interval.
+static const int CHECK_RESTORE_BACKUP_CONNECTION_INTERVAL = 5 * 60 * 1000;
+
 P2PTransportChannel::P2PTransportChannel(const std::string& transport_name,
                                          int component,
                                          P2PTransport* transport,
@@ -246,6 +255,8 @@ P2PTransportChannel::P2PTransportChannel(const std::string& transport_name,
       tiebreaker_(0),
       gathering_state_(kIceGatheringNew),
       check_receiving_interval_(MIN_CHECK_RECEIVING_INTERVAL * 5),
+      check_restore_backup_connection_interval_(
+          CHECK_RESTORE_BACKUP_CONNECTION_INTERVAL),
       config_(MIN_CHECK_RECEIVING_INTERVAL * 50 /* receiving_timeout */,
               0 /* backup_connection_ping_interval */,
               false /* gather_continually */,
@@ -280,6 +291,10 @@ void P2PTransportChannel::AddAllocatorSession(
   // created by this new session because these are replacing those of the
   // previous sessions.
   ports_.clear();
+  // We do not need to signal the candidate removals because a new allocator
+  // session indicates an ICE restart, which will remove all old remote
+  // candidates on the other side.
+  candidates_removed_.clear();
 
   allocator_sessions_.push_back(std::move(session));
 }
@@ -340,7 +355,10 @@ TransportChannelState P2PTransportChannel::ComputeState() const {
       active_connections.push_back(connection);
     }
   }
-  if (active_connections.empty()) {
+  // TODO(honghaiz): We should expose one transport channel state to the
+  // the external world (e.g. TransportController), so that we won't need to
+  // make both writable and channel state depend on IsRecoveringConnectivity.
+  if (active_connections.empty() && !IsRecoveringConnectivity()) {
     return TransportChannelState::STATE_FAILED;
   }
 
@@ -456,6 +474,8 @@ void P2PTransportChannel::Connect() {
 
   // Start checking and pinging as the ports come in.
   thread()->Post(this, MSG_CHECK_AND_PING);
+  thread()->PostDelayed(check_restore_backup_connection_interval_, this,
+                        MSG_CHECK_RESTORE_BACKUP_CONNECTION);
 }
 
 void P2PTransportChannel::MaybeStartGathering() {
@@ -1237,7 +1257,10 @@ void P2PTransportChannel::UpdateState() {
     SignalStateChanged(this);
   }
 
-  bool writable = best_connection_ && best_connection_->writable();
+  // If it is recovering connectivity, it is considered writable to the
+  // external world.

[Taylor Brandstetter, 2016/06/06 18:18:37]

When you say "external world", do you mean applications (so they see ICE
"connected" instead of "disconnected"), or do you mean the audio/video engines?
I think in this situation, an application *should* see "disconnected". If
they've turned continual gathering on, they should expect this, and just wait
for it to resolve itself, and maybe display a message to the user in the
meantime.

[honghaiz3, 2016/06/07 16:42:06]

I meant the TransportController and the layers above. 
If the application see the "disconnected", it will try to kick off "full ICE
restart", which we don't want it to happen when a continual gathering may
resolve the issue. I added a TODO to expose the recovering state the app,
although the app won't do much about it except that "it maybe display a message
to the user". 

[Taylor Brandstetter, 2016/06/07 17:13:26]

If the application is using continual gathering, it should know that
"disconnected" can resolve itself. It should expect a sequence of these states:

connected/not gathering ->
disconnected/gathering ->
checking/not gathering ->
connected/not gathering

If at the end, the state is "disconnected/not gathering", *then* the application
may want to do a full ICE restart.

However, we don't even switch to "checking" properly right now. So I'm ok with
the code as it is now. Just be aware that we may need to change it later.

+  bool writable = (best_connection_ && best_connection_->writable()) ||
+                  IsRecoveringConnectivity();
   set_writable(writable);
 
   bool receiving = false;
@@ -1306,6 +1329,17 @@ void P2PTransportChannel::OnMessage(rtc::Message *pmsg) {
     case MSG_CHECK_AND_PING:
       OnCheckAndPing();
       break;
+    case MSG_CHECK_RESTORE_BACKUP_CONNECTION:
+      OnCheckAndRestoreBackupConnection();
+      break;
+    case MSG_SIGNAL_CANDIDATES_REMOVED:
+      if (!candidates_removed_.empty()) {
+        LOG(LS_INFO) << "Signal removing " << candidates_removed_.size()
+                     << " candidates";
+        SignalCandidatesRemoved(this, candidates_removed_);
+        candidates_removed_.clear();
+      }
+      break;
     default:
       ASSERT(false);
       break;
@@ -1344,6 +1378,41 @@ void P2PTransportChannel::OnCheckAndPing() {
   thread()->PostDelayed(delay, this, MSG_CHECK_AND_PING);
 }
 
+void P2PTransportChannel::OnCheckAndRestoreBackupConnection() {
+  if (!config_.gather_continually) {
+    return;
+  }
+  rtc::NetworkManager::NetworkList networks;
+  allocator_->GetNetworks(&networks);
+  // Filter out loopback networks.
+  auto network_end = std::remove_if(
+      networks.begin(), networks.end(), [](rtc::Network* network) {
+        return network->type() == rtc::ADAPTER_TYPE_LOOPBACK;
+      });
+
+  // Filter out networks having connections in non-failed state.
+  for (Connection* conn : connections_) {
+    if (conn->state() == Connection::STATE_FAILED) {
+      continue;
+    }
+    const std::string& interface_name = conn->port()->Network()->name();
+    network_end = std::remove_if(networks.begin(), network_end,
+                                 [interface_name](rtc::Network* net) {
+                                   return net->name() == interface_name;
+                                 });
+    if (networks.begin() == network_end) {
+      break;
+    }
+  }
+  networks.erase(network_end, networks.end());
+  if (!networks.empty()) {
+    StartRegathering(&networks);
+  }
+
+  thread()->PostDelayed(check_restore_backup_connection_interval_, this,
+                        MSG_CHECK_RESTORE_BACKUP_CONNECTION);
+}
+
 // A connection is considered a backup connection if the channel state
 // is completed, the connection is not the best connection and it is active.
 bool P2PTransportChannel::IsBackupConnection(Connection* conn) const {
@@ -1411,6 +1480,14 @@ void P2PTransportChannel::MarkConnectionPinged(Connection* conn) {
   }
 }
 
+void P2PTransportChannel::set_check_restore_backup_connection_interval(
+    int interval) {
+  check_restore_backup_connection_interval_ = interval;
+  thread()->Clear(this, MSG_CHECK_RESTORE_BACKUP_CONNECTION, nullptr);
+  thread()->PostDelayed(check_restore_backup_connection_interval_, this,
+                        MSG_CHECK_RESTORE_BACKUP_CONNECTION);
+}
+
 // Apart from sending ping from |conn| this method also updates
 // |use_candidate_attr| flag. The criteria to update this flag is
 // explained below.
@@ -1462,6 +1539,9 @@ void P2PTransportChannel::OnConnectionStateChange(Connection* connection) {
   bool latest_generation = connection->local_candidate().generation() >=
                            allocator_session()->generation();
   if (strongly_connected && latest_generation) {
+    if (recovering_start_time_) {
+      recovering_start_time_ = 0;
+    }
     MaybeStopPortAllocatorSessions();
   }
 
@@ -1500,6 +1580,21 @@ void P2PTransportChannel::OnConnectionDestroyed(Connection* connection) {
   // and re-choose a best assuming that there was no best connection.
   if (best_connection_ == connection) {
     LOG(LS_INFO) << "Best connection destroyed.  Will choose a new one.";
+    // When continual gathering is enabled, we will attempt re-gathering only
+    // if the best connecion was removed and it was writable. If it was not

[Taylor Brandstetter, 2016/06/06 18:18:37]

"connecion" => "connection"

[honghaiz3, 2016/06/07 16:42:06]

Done.

+    // writable, most likely it is in the process of gathering on the current
+    // session.
+    if (config_.gather_continually && writable()) {
+      bool all_connections_failed = std::all_of(
+          connections_.begin(), connections_.end(), [](Connection* conn) {
+            return conn->state() == Connection::STATE_FAILED;
+          });
+      // Start regathering on all active networks if all connections failed.
+      if (all_connections_failed && !IsRecoveringConnectivity()) {
+        recovering_start_time_ = rtc::TimeMillis();
+        StartRegathering(nullptr);
+      }
+    }
     SwitchBestConnectionTo(NULL);
     RequestSort();
   }
@@ -1524,7 +1619,7 @@ void P2PTransportChannel::OnPortDestroyed(PortInterface* port) {
 
 void P2PTransportChannel::OnPortNetworkInactive(PortInterface* port) {
   // If it does not gather continually, the port will be removed from the list
-  // when ICE restarts.
+  // when ICE restarts, so don't need to remove it here.
   if (!config_.gather_continually) {
     return;
   }
@@ -1536,11 +1631,47 @@ void P2PTransportChannel::OnPortNetworkInactive(PortInterface* port) {
   ports_.erase(it);
   LOG(INFO) << "Removed port due to inactive networks: " << ports_.size()
             << " remaining";
-  std::vector<Candidate> candidates = port->Candidates();
-  for (Candidate& candidate : candidates) {
+  port->FailAndDestroyConnections();
+
+  for (Candidate candidate : port->Candidates()) {
     candidate.set_transport_name(transport_name());
+    candidates_removed_.push_back(candidate);

[Taylor Brandstetter, 2016/06/06 18:18:37]

Something I didn't notice until now: This needs to sanitize candidates just like
BasicPortAllocator does. By which I mean: don't signal candidates that don't
match the candidate filter, and zero out the "related address" if necessary.
See:
https://cs.chromium.org/chromium/src/third_party/webrtc/p2p/client/basicporta...

If we don't do this, then we'll end up leaking IPs in the "candidate removed"
event.

[honghaiz3, 2016/06/07 16:42:06]

Done.

   }
-  SignalCandidatesRemoved(this, candidates);
+  thread()->Post(this, MSG_SIGNAL_CANDIDATES_REMOVED);
+}
+
+void P2PTransportChannel::StartRegathering(
+    std::vector<rtc::Network*>* networks) {
+  std::string log_msg;
+  std::vector<PortInterface*> ports_to_remove;
+  if (networks == nullptr) {
+    log_msg = "Regathering on all networks";
+    ports_to_remove = ports_;
+  } else {
+    log_msg = "Regathering on networks:";
+    for (rtc::Network* network : *networks) {
+      log_msg += " " + network->ToString();
+    }
+    // Remove all ports using |networks|.
+    for (PortInterface* port : ports_) {
+      if (std::find(networks->begin(), networks->end(), port->Network()) !=
+          networks->end()) {
+        ports_to_remove.push_back(port);
+      }
+    }
+  }
+  LOG(LS_INFO) << log_msg;
+  for (PortInterface* port : ports_to_remove) {
+    // Remove the port and signal the other side to remove the respective remote
+    // candidates.
+    OnPortNetworkInactive(port);
+  }
+
+  PortAllocatorSession* session = allocator_session();
+  // By deactivating all networks in existing allocation sequences,
+  // it will force re-gathering on all active networks.
+  session->InactivateNetworksInExistingSequences(networks);
+  session->StartGettingPorts();
 }
 
 // We data is available, let listeners know