Ping backup connection at a slower rate

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 ab7c32277f9f920c667b25c8f977998b5665b83d..b89c8d556f43dbc0efac42874db577b925b95dcc 100644
--- a/webrtc/p2p/base/p2ptransportchannel.cc
+++ b/webrtc/p2p/base/p2ptransportchannel.cc
@@ -201,6 +201,10 @@ static const uint32_t MAX_CURRENT_STRONG_DELAY = 900;
 
 static const int MIN_CHECK_RECEIVING_DELAY = 50;  // ms
 
+// This is 5 seconds shorter than DEAD_CONNECTION_RECEIVE_TIMEOUT in port.h,
+// so there are 5 seconds to retry pinging the backup connection before it
+// becomes dead.
+static const int BACKUP_CONNECTION_PING_INTERVAL = 25000;  // ms
 
 P2PTransportChannel::P2PTransportChannel(const std::string& transport_name,
                                          int component,
@@ -221,7 +225,8 @@ P2PTransportChannel::P2PTransportChannel(const std::string& transport_name,
       remote_candidate_generation_(0),
       gathering_state_(kIceGatheringNew),
       check_receiving_delay_(MIN_CHECK_RECEIVING_DELAY * 5),
-      receiving_timeout_(MIN_CHECK_RECEIVING_DELAY * 50) {
+      receiving_timeout_(MIN_CHECK_RECEIVING_DELAY * 50),
+      backup_connection_ping_interval_(BACKUP_CONNECTION_PING_INTERVAL) {

[pthatcher1, 2015/11/20 05:34:38]

We call one "delay" and the other "interval".  It sounds like it should be
'delay' everywhere or 'interval' everywhere.  I think 'interval' sounds better.

[honghaiz3, 2015/11/20 20:10:06]

I agree that the interval sounds better. I will change the words in a subsequent
CL and put the TODO near the definition of this variable.  

   uint32_t weak_ping_delay = ::strtoul(
       webrtc::field_trial::FindFullName("WebRTC-StunInterPacketDelay").c_str(),
       nullptr, 10);
@@ -296,9 +301,13 @@ void P2PTransportChannel::SetIceTiebreaker(uint64_t tiebreaker) {
   tiebreaker_ = tiebreaker;
 }
 
+TransportChannelState P2PTransportChannel::GetState() const {
+  return channel_state_;

[pthatcher1, 2015/11/20 05:34:37]

Why not just state_?

[honghaiz3, 2015/11/20 20:10:06]

Done.

+}
+
 // A channel is considered ICE completed once there is at most one active
 // connection per network and at least one active connection.
-TransportChannelState P2PTransportChannel::GetState() const {
+TransportChannelState P2PTransportChannel::GetStateInternal() const {
   if (!had_connection_) {
     return TransportChannelState::STATE_INIT;
   }
@@ -372,18 +381,23 @@ void P2PTransportChannel::SetIceConfig(const IceConfig& config) {
   gather_continually_ = config.gather_continually;
   LOG(LS_INFO) << "Set gather_continually to " << gather_continually_;
 
-  if (config.receiving_timeout_ms < 0) {
-    return;
+  if (config.backup_connection_ping_interval >= 0) {
+    backup_connection_ping_interval_ = config.backup_connection_ping_interval;
+    LOG(LS_INFO) << "Set backup connection ping interval to "
+                 << backup_connection_ping_interval_ << " milliseconds.";

[pthatcher1, 2015/11/20 05:34:37]

You probably only want to log this it if it changes.

[honghaiz3, 2015/11/20 20:10:06]

Done. Only set and log this if the value changes.

   }
-  receiving_timeout_ = config.receiving_timeout_ms;
-  check_receiving_delay_ =
-      std::max(MIN_CHECK_RECEIVING_DELAY, receiving_timeout_ / 10);
 
-  for (Connection* connection : connections_) {
-    connection->set_receiving_timeout(receiving_timeout_);
+  if (config.receiving_timeout_ms >= 0) {
+    receiving_timeout_ = config.receiving_timeout_ms;
+    check_receiving_delay_ =
+        std::max(MIN_CHECK_RECEIVING_DELAY, receiving_timeout_ / 10);
+
+    for (Connection* connection : connections_) {
+      connection->set_receiving_timeout(receiving_timeout_);
+    }
+    LOG(LS_INFO) << "Set ICE receiving timeout to " << receiving_timeout_
+                 << " milliseconds";

[pthatcher1, 2015/11/20 05:34:38]

Same here.

[honghaiz3, 2015/11/20 20:10:06]

Done.

   }
-  LOG(LS_INFO) << "Set ICE receiving timeout to " << receiving_timeout_
-               << " milliseconds";
 }
 
 // Go into the state of processing candidates, and running in general
@@ -1051,6 +1065,8 @@ void P2PTransportChannel::SwitchBestConnectionTo(Connection* conn) {
 }
 
 void P2PTransportChannel::UpdateChannelState() {
+  channel_state_ = GetStateInternal();

[pthatcher1, 2015/11/20 05:34:37]

So what's the advantage of cacheing the state and possibly getting out of sync?

If there is one, we should definitely document what it is.

[honghaiz3, 2015/11/20 20:10:06]

Basically I want to avoid too much unnecessary computations to call GetState(). 
In TransportController, there are tons of places that will call this. 
Plus, I am adding more calls on this. we will be calling this once every 500ms,
or 50ms if the channel is weak. It could be more because we are calling this on
every connection (this can be reduced though). 

+
   bool writable = best_connection_ && best_connection_->writable();
   set_writable(writable);
 
@@ -1153,7 +1169,7 @@ void P2PTransportChannel::OnCheckAndPing() {
 // Is the connection in a state for us to even consider pinging the other side?
 // We consider a connection pingable even if it's not connected because that's
 // how a TCP connection is kicked into reconnecting on the active side.
-bool P2PTransportChannel::IsPingable(Connection* conn) {
+bool P2PTransportChannel::IsPingable(Connection* conn, uint32_t now) {
   const Candidate& remote = conn->remote_candidate();
   // We should never get this far with an empty remote ufrag.
   ASSERT(!remote.username().empty());
@@ -1169,9 +1185,21 @@ bool P2PTransportChannel::IsPingable(Connection* conn) {
     return false;
   }
 
-  // If the channel is weak, ping all candidates. Otherwise, we only
-  // want to ping connections that have not timed out on writing.
-  return weak() || conn->write_state() != Connection::STATE_WRITE_TIMEOUT;
+  // If the channel is weakly connected, ping all connections.
+  if (weak()) {
+    return true;
+  }
+
+  // If the channel is not COMPLETED,

[pthatcher1, 2015/11/20 05:34:37]

Might be more readable as "If the channel is strongly connected but not yet
completed, ..."

[honghaiz3, 2015/11/20 20:10:06]

Done.

+  // ping connections that have not timed out on writing.

[pthatcher1, 2015/11/20 05:34:38]

To match the code, you should say "ping all active connections"

[honghaiz3, 2015/11/20 20:10:06]

Done.

+  if (channel_state_ != STATE_COMPLETED) {
+    return conn->active();
+  }
+  // If the channel is strongly connected and it is COMPLETED, ping the backup
+  // connection once every |backup_connection_ping_interval_| milliseconds.

[pthatcher1, 2015/11/20 05:34:38]

The comment says "ping the backup connection", but the code pings both the best
connection and then any connection that hasn't been pinged in the last
backup_connection_ping_interval_ milliseconds.

It seems like this would be more simple with the logic being "always ping all
active connections regardless of whether it is COMPLETED or not, but the
frequency of the ping changes depending on whether it's a backup connection or
not.  So the real logic is IsBackupConnection(connection).

So could we have something like this?

if (weak())) {
  return true;
}

if (IsBackupConnection(connection)) {
  return (conn->last_ping_response_received() &&
backup_connection_ping_interval_);
}

return conn->Active();

bool IsBackupConnection(connection) {
  return ((channel_state_ == STATE_COMPLETED) && (connection !=
best_connection_);
}

[honghaiz3, 2015/11/20 20:10:06]

Thanks. made a small change, requiring that backup connections to be active. 

+  return conn == best_connection_ ||
+         now >= (conn->last_ping_response_received() +
+                 backup_connection_ping_interval_);
 }
 
 // Returns the next pingable connection to ping.  This will be the oldest
@@ -1195,7 +1223,7 @@ Connection* P2PTransportChannel::FindNextPingableConnection() {
   Connection* oldest_needing_triggered_check = nullptr;
   Connection* oldest = nullptr;
   for (Connection* conn : connections_) {
-    if (!IsPingable(conn)) {
+    if (!IsPingable(conn, now)) {
       continue;
     }
     bool needs_triggered_check =
@@ -1307,6 +1335,8 @@ void P2PTransportChannel::OnConnectionDestroyed(Connection* connection) {
     RequestSort();
   }
 
+  UpdateChannelState();

[pthatcher1, 2015/11/20 05:34:38]

This kind of thing makes me worry that we won't remember to call it in all the
right places and end up out sync, introducing bugs.  Are you sure we need to
cache it?  If so, are you sure we call "update" in all of the right places?

[honghaiz3, 2015/11/20 20:10:06]

I am pretty sure this is the only place we need to add to make sure it syncs. 
GetStateInternal could only change the state when a connection is added,
removed, or a connection's write state has changed. Whenever a connection is
created, or a connection state has changed, it will sort connections. We
updateChannelState at the end of sorting. The only thing we miss is that when a
connection is deleted, we do not always UpdateChannelState. 
In the future, I think we should signal the channel state change event to
transportController instead of signal the write state change and connection
deletion event. 

+
   SignalConnectionRemoved(this);
 }