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..ad09cc12582357f2c53a1516721618508fd29d3b 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,

[pthatcher1, 2016/06/07 18:54:33]

It seems like a better name would be MSG_REGATHER_FAILED_NETWORKS

[honghaiz3, 2016/06/29 02:48:22]

MSG_REGATHER_ON_FAILED_NETWORKS better?

+  MSG_SIGNAL_CANDIDATES_REMOVED

[pthatcher1, 2016/06/07 18:54:33]

Why does this require a thread post?  To coalesce candidate removals?

[honghaiz3, 2016/06/29 02:48:22]

Yes. We can merge multiple candidate removals into one. 

+};
 
 // 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;

[pthatcher1, 2016/06/07 18:54:33]

I think a better name would be DEFAULT_REGATHER_FAILED_NETWORKS_INTERVAL.?

+
 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),

[pthatcher1, 2016/06/07 18:54:33]

Can you put this in config_ instead of as its own thing?

       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();

[pthatcher1, 2016/06/07 18:54:33]

This feels like implicit magic to me.  Could we instead know when we're removing
candidates because of an ICE restart and then suppress them from being signaled
in the first place instead of clearing them here?

 
   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()) {

[pthatcher1, 2016/06/07 18:54:33]

I don't like the look of the complexity added to hide the disconnected state
from the application.  I think it would be better instead to keep it simple and
just expose the disconnected state to applications that opt into continual
gathering.

     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,11 @@ 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.
+  // TODO(honghaiz): Expose the recovering state to the application.
+  bool writable = (best_connection_ && best_connection_->writable()) ||
+                  IsRecoveringConnectivity();
   set_writable(writable);
 
   bool receiving = false;
@@ -1306,6 +1330,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 +1379,41 @@ void P2PTransportChannel::OnCheckAndPing() {
   thread()->PostDelayed(delay, this, MSG_CHECK_AND_PING);
 }
 
+void P2PTransportChannel::OnCheckAndRestoreBackupConnection() {
+  if (!config_.gather_continually || !allocator_->network_manager()) {
+    return;
+  }
+  rtc::NetworkManager::NetworkList networks;
+  allocator_->network_manager()->GetNetworks(&networks);

[pthatcher1, 2016/06/07 18:54:33]

I'm not a fan of reaching into the allocator like this.  Would it make sense,
instead, to have a GetFailedNetworks method on the Allocator?  The Allocator has
the Ports.  The Ports have the Connections.  The Connections have the states. 
So the Allocator should know which ports have non-failed connections.  I think
the code would end up being more simple and would have a more clear division of
labor between the P2pTransportChannel and the PortAllocator.

+  // 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());

[pthatcher1, 2016/06/07 18:54:33]

This logic seems a bit complicated.   Would it make more sense to have one
std::remove_if with networks, checking if any non-failed connection has a
matching network name?

[honghaiz3, 2016/06/29 02:48:23]

Done.

+  if (!networks.empty()) {
+    StartRegathering(&networks);
+  }

[pthatcher1, 2016/06/07 18:54:33]

It seems like a lot of this logic should be in a separate methods called
RegatherNetworksWithAllFailedConnections or RegatherFailedNetworks, unless we
move most of it to PortAllocator::GetFailedNetworks(), in which case it would be
StartRegathering(allocator->GetFailedNetworks()).  

In fact, why even have StartRegathering logic in P2pTransportChannel?  Why not
have that in PortAllocator?  Why not have the method be
PortAllocator::RegatherFailedNetworks()?   That would seem even more clear to
me. 

[honghaiz3, 2016/06/29 02:48:23]

Done.

+
+  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 +1481,14 @@ void P2PTransportChannel::MarkConnectionPinged(Connection* conn) {
   }
 }
 
+void P2PTransportChannel::set_check_restore_backup_connection_interval(

[pthatcher1, 2016/06/07 18:54:33]

It sounds like this method should be called RegatherFailedNetworksEvery(int
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 +1540,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;
+    }

[pthatcher1, 2016/06/07 18:54:33]

***

     MaybeStopPortAllocatorSessions();
   }
 
@@ -1500,6 +1581,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 connection was removed and it was writable. If it was not
+    // 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();

[pthatcher1, 2016/06/07 18:54:33]

Should this be last_regather_time_, and should it be set inside of
StartRegathering?

[honghaiz3, 2016/06/29 02:48:23]

Done.

+        StartRegathering(nullptr);
+      }

[pthatcher1, 2016/06/07 18:54:33]

Can we put this in a separate methods called MaybeRegather() or
RegatherIfAllConnectionsJustFailed()?

[honghaiz3, 2016/06/29 02:48:23]

Removed. 

+    }
     SwitchBestConnectionTo(NULL);
     RequestSort();
   }
@@ -1524,7 +1620,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 +1632,45 @@ 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();

[pthatcher1, 2016/06/07 18:54:33]

It feels like this should be Close(), which should then fire
SignalCandidatesRemoved, which we can listen to.

+
+  std::vector<Candidate> candidates =
+      allocator_session()->ReadyCandidates(port);
+  for (Candidate candidate : candidates) {
     candidate.set_transport_name(transport_name());
+    candidates_removed_.push_back(candidate);
   }
-  SignalCandidatesRemoved(this, candidates);
+  thread()->Post(this, MSG_SIGNAL_CANDIDATES_REMOVED);

[pthatcher1, 2016/06/07 18:54:33]

Why does this need a thread hop?

[honghaiz3, 2016/06/29 02:48:23]

We can combine multiple candidate removal signals into one. 

+}
+
+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);

[pthatcher1, 2016/06/07 18:54:33]

I feel like we're confounding two things by triggering OnPortNetworkInactive
directly here.  If we have a different signal and that triggers similar
behavior, I think that's OK.  But confounding the two gets too complex.

For example, we could, here, call port->Close() which would then fire a
port->SignalCandidatesRemoved, which we could listen to.  

+  }
+
+  allocator_session()->GetPortsOnNetworks(networks);
 }
 
 // We data is available, let listeners know