Ping backup connection at a slower rate

webrtc/p2p/base/p2ptransportchannel.cc

 /*
  *  Copyright 2004 The WebRTC Project Authors. All rights reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 183 matching lines @@
 // writable or not receiving.
 const uint32_t WEAK_PING_DELAY = 1000 * PING_PACKET_SIZE / 10000;
 
 // If the current best connection is both writable and receiving, then we will
 // also try hard to make sure it is pinged at this rate (a little less than
 // 2 * STRONG_PING_DELAY).
 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,
                                          P2PTransport* transport,
                                          PortAllocator* allocator)
     : TransportChannelImpl(transport_name, component),
       transport_(transport),
       allocator_(allocator),
       worker_thread_(rtc::Thread::Current()),
       incoming_only_(false),
       error_(0),
       best_connection_(NULL),
       pending_best_connection_(NULL),
       sort_dirty_(false),
       remote_ice_mode_(ICEMODE_FULL),
       ice_role_(ICEROLE_UNKNOWN),
       tiebreaker_(0),
       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);
   if (weak_ping_delay) {
     weak_ping_delay_ =  weak_ping_delay;
   }
 }
 
 P2PTransportChannel::~P2PTransportChannel() {
   ASSERT(worker_thread_ == rtc::Thread::Current());
@@ skipping 54 matching lines @@
   ASSERT(worker_thread_ == rtc::Thread::Current());
   if (!ports_.empty()) {
     LOG(LS_ERROR)
         << "Attempt to change tiebreaker after Port has been allocated.";
     return;
   }
 
   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;
   }
 
   std::vector<Connection*> active_connections;
   for (Connection* connection : connections_) {
     if (connection->active()) {
       active_connections.push_back(connection);
     }
   }
@@ skipping 53 matching lines @@
 }
 
 void P2PTransportChannel::SetRemoteIceMode(IceMode mode) {
   remote_ice_mode_ = mode;
 }
 
 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
 void P2PTransportChannel::Connect() {
   ASSERT(worker_thread_ == rtc::Thread::Current());
   if (ice_ufrag_.empty() || ice_pwd_.empty()) {
     ASSERT(false);
     LOG(LS_ERROR) << "P2PTransportChannel::Connect: The ice_ufrag_ and the "
                   << "ice_pwd_ are not set.";
     return;
@@ skipping 647 matching lines @@
     }
     LOG_J(LS_INFO, this) << "New best connection: "
                          << best_connection_->ToString();
     SignalRouteChange(this, best_connection_->remote_candidate());
   } else {
     LOG_J(LS_INFO, this) << "No best connection";
   }
 }
 
 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);
 
   bool receiving = false;
   for (const Connection* connection : connections_) {
     if (connection->receiving()) {
       receiving = true;
       break;
     }
   }
@@ skipping 82 matching lines @@
       PingConnection(conn);
     }
   }
   int check_delay = std::min(ping_delay, check_receiving_delay_);
   thread()->PostDelayed(check_delay, this, MSG_CHECK_AND_PING);
 }
 
 // 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());
   if (remote.username().empty() || remote.password().empty()) {
     // If we don't have an ICE ufrag and pwd, there's no way we can ping.
     return false;
   }
 
   // An never connected connection cannot be written to at all, so pinging is
   // out of the question. However, if it has become WRITABLE, it is in the
   // reconnecting state so ping is needed.
   if (!conn->connected() && !conn->writable()) {
     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
 // pingable connection unless we have a connected, writable connection that is
 // past the maximum acceptable ping delay. When reconnecting a TCP connection,
 // the best connection is disconnected, although still WRITABLE while
 // reconnecting. The newly created connection should be selected as the ping
 // target to become writable instead. See the big comment in CompareConnections.
 Connection* P2PTransportChannel::FindNextPingableConnection() {
   uint32_t now = rtc::Time();
   if (best_connection_ && best_connection_->connected() &&
       best_connection_->writable() &&
       (best_connection_->last_ping_sent() + MAX_CURRENT_STRONG_DELAY <= now)) {
     return best_connection_;
   }
 
   // First, 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
   // triggered checks if the connection is already writable.
   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 =
         (!conn->writable() &&
          conn->last_ping_received() > conn->last_ping_sent());
     if (needs_triggered_check &&
         (!oldest_needing_triggered_check ||
          (conn->last_ping_received() <
           oldest_needing_triggered_check->last_ping_received()))) {
       oldest_needing_triggered_check = conn;
@@ skipping 91 matching lines @@
   // The call to SortConnections will pick a new one.  It looks at the current
   // best connection in order to avoid switching between fairly similar ones.
   // Since this connection is no longer an option, we can just set best to NULL
   // 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.";
     SwitchBestConnectionTo(NULL);
     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);
 }
 
 // When a port is destroyed remove it from our list of ports to use for
 // connection attempts.
 void P2PTransportChannel::OnPortDestroyed(PortInterface* port) {
   ASSERT(worker_thread_ == rtc::Thread::Current());
 
   // Remove this port from the list (if we didn't drop it already).
   std::vector<PortInterface*>::iterator iter =
@@ skipping 34 matching lines @@
   SignalSentPacket(this, sent_packet);
 }
 
 void P2PTransportChannel::OnReadyToSend(Connection* connection) {
   if (connection == best_connection_ && writable()) {
     SignalReadyToSend(this);
   }
 }
 
 }  // namespace cricket