Increase the stun ping interval.

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 199 matching lines @@
 // well on a 28.8K modem, which is the slowest connection on which the voice
 // quality is reasonable at all.
 static const int PING_PACKET_SIZE = 60 * 8;
 // STRONG_PING_INTERVAL (480ms) is applied when the best connection is both
 // writable and receiving.
 static const int STRONG_PING_INTERVAL = 1000 * PING_PACKET_SIZE / 1000;
 // WEAK_PING_INTERVAL (48ms) is applied when the best connection is either not
 // writable or not receiving.
 const int WEAK_PING_INTERVAL = 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_INTERVAL).
+// If the current best connection is both writable and receiving, first several

[pthatcher1, 2016/06/01 23:22:02]

first several pings => the first several pings

+// pings will be sent at this rate by default in order for the RTT to converge

[pthatcher1, 2016/06/01 23:22:02]

This is for all connections, not just the best connection, right?  Perhaps we
could say "Writable connections at ping at a higher rate while stabilizing"

+// faster. If it pings too fast, other connections may get starved.

[pthatcher1, 2016/06/01 23:22:02]

Doesn't this apply to all connections, not just the best?  If so, I think the
comment should say something like "Writable connections are pinged at a faster
rate while stabilizing."

 static const int MAX_CURRENT_STRONG_INTERVAL = 900;  // ms

[pthatcher1, 2016/06/01 23:22:02]

This needs to be renamed now.

Perhaps:

STABLIZING_WRITABLE_CONNECTION_PING_INTERVAL

And the other one could be:

STABLE_WRITABLE_CONNECTION_PING_INTERVAL

 
+// Writable connections are pinged at a slower rate.

[pthatcher1, 2016/06/01 23:22:03]

once stablized

+static const int WRITABLE_CONNECTION_PING_INTERVAL = 2500;  // ms
+
 static const int MIN_CHECK_RECEIVING_INTERVAL = 50;  // ms
 
 P2PTransportChannel::P2PTransportChannel(const std::string& transport_name,
                                          int component,
                                          P2PTransport* transport,
                                          PortAllocator* allocator)
     : P2PTransportChannel(transport_name, component, allocator) {}
 
 P2PTransportChannel::P2PTransportChannel(const std::string& transport_name,
                                          int component,
                                          PortAllocator* allocator)
     : TransportChannelImpl(transport_name, component),
       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),
       gathering_state_(kIceGatheringNew),
       check_receiving_interval_(MIN_CHECK_RECEIVING_INTERVAL * 5),
       config_(MIN_CHECK_RECEIVING_INTERVAL * 50 /* receiving_timeout */,
               0 /* backup_connection_ping_interval */,
               false /* gather_continually */,
               false /* prioritize_most_likely_candidate_pairs */,
-              MAX_CURRENT_STRONG_INTERVAL /* max_strong_interval */) {
+              WRITABLE_CONNECTION_PING_INTERVAL) {
   uint32_t weak_ping_interval = ::strtoul(
       webrtc::field_trial::FindFullName("WebRTC-StunInterPacketDelay").c_str(),
       nullptr, 10);
   if (weak_ping_interval) {
     weak_ping_interval_ = static_cast<int>(weak_ping_interval);
   }
 }
 
 P2PTransportChannel::~P2PTransportChannel() {
   ASSERT(worker_thread_ == rtc::Thread::Current());
@@ skipping 161 matching lines @@
     }
     LOG(LS_INFO) << "Set ICE receiving timeout to " << config_.receiving_timeout
                  << " milliseconds";
   }
 
   config_.prioritize_most_likely_candidate_pairs =
       config.prioritize_most_likely_candidate_pairs;
   LOG(LS_INFO) << "Set ping most likely connection to "
                << config_.prioritize_most_likely_candidate_pairs;
 
-  if (config.max_strong_interval >= 0 &&
-      config_.max_strong_interval != config.max_strong_interval) {
-    config_.max_strong_interval = config.max_strong_interval;
-    LOG(LS_INFO) << "Set max strong interval to "
-                 << config_.max_strong_interval;
+  if (config.writable_connection_ping_interval >= 0 &&
+      config_.writable_connection_ping_interval !=
+          config.writable_connection_ping_interval) {
+    config_.writable_connection_ping_interval =
+        config.writable_connection_ping_interval;
+    LOG(LS_INFO) << "Set writable_connection_ping_interval to "
+                 << config_.writable_connection_ping_interval;
   }
 }
 
 const IceConfig& P2PTransportChannel::config() const {
   return config_;
 }
 
 // Go into the state of processing candidates, and running in general
 void P2PTransportChannel::Connect() {
   ASSERT(worker_thread_ == rtc::Thread::Current());
@@ skipping 890 matching lines @@
   int ping_interval = (weak() || need_more_pings_at_weak_interval)
                           ? weak_ping_interval_
                           : STRONG_PING_INTERVAL;
   if (rtc::TimeMillis() >= last_ping_sent_ms_ + ping_interval) {
     Connection* conn = FindNextPingableConnection();
     if (conn) {
       PingConnection(conn);
       MarkConnectionPinged(conn);
     }
   }
+
   int delay = std::min(ping_interval, check_receiving_interval_);
   thread()->PostDelayed(delay, this, MSG_CHECK_AND_PING);
 }
 
 // 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 {
   return state_ == STATE_COMPLETED && conn != best_connection_ &&
          conn->active();
 }
 
+bool P2PTransportChannel::IsBestConnectionPingable() {
+  if (!best_connection_ || !best_connection_->connected() ||
+      !best_connection_->writable()) {
+    return false;
+  }
+
+  int interval = NeedToPingFast(best_connection_)
+                     ? std::min(MAX_CURRENT_STRONG_INTERVAL,
+                                config_.writable_connection_ping_interval)
+                     : config_.writable_connection_ping_interval;
+  return best_connection_->last_ping_sent() + interval <= rtc::TimeMillis();
+}
+
+bool P2PTransportChannel::NeedToPingFast(Connection* conn) {

[pthatcher1, 2016/06/01 23:22:02]

This the code I wrote below, I think we don't need this method.  In fact, I
don't think we need IsBestConnectionPingable.  We could just have:

best_connection_ && IsPingable(best_connection_)

[honghaiz3, 2016/06/02 00:03:57]

Cannot do this. 
If the best_connection becomes unwritable, it will ping the best connection
forever. 

[pthatcher1, 2016/06/02 18:03:02]

Good catch.  How about best_connection_ && best_connection_->connected() &&
best_connection->writable() && IsPingable(best_connection_)?

That's basically what we have now, except replacing
"(best_connection_->last_ping_sent() + config_.max_strong_interval <=	 now)"
with "IsPingable(best_connection_)".

[honghaiz3, 2016/06/02 19:20:25]

That would work. It is more readable although at the price of checking
writable() and connected() twice for the best connection.  

+  return conn->num_pings_sent() <= MIN_PINGS_FOR_RTT_CONVERGENCE;
+}
+
 // 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, int64_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;
@@ skipping 10 matching lines @@
   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)) {
     return (now >= conn->last_ping_response_received() +
                        config_.backup_connection_ping_interval);
   }
+

[pthatcher1, 2016/06/01 23:22:02]

I think instead of having NeedsToPingFast, we can just roll the logic into here.
 From here in this method and below, the logic could be thus:

// Don't ping inactive non-backup connections.
if (!conn->active()) {
  return false;
}

// Do ping unwritable, active connections.
if (!conn->writable()) {
  return true;
}

// Ping writable, active connections if it's been long enough since the last
ping.
int ping_interval = config_.writable_connection_ping_interval;

// A stabilizing connection gets a faster ping interval.
bool stable = conn->num_pings_sent() <= MIN_PINGS_TO_STABILIZE;
if (!stable) {
  ping_interval = std::min(ping_interval,
STABLIZING_WRITABLE_CONNECTION_PING_INTERVAL);
}
return (now >= conn->last_ping_sent() + ping_interval);

[honghaiz3, 2016/06/02 00:03:57]

look good to me. This is just 
int ping_interval = stable ? config_.writable_connection_ping_interval :
STABLIZING_WRITABLE_CONNECTION_PING_INTERVAL; 

[honghaiz3, 2016/06/02 17:06:25]

As I thought more about it, I think it may not be good to set the ping interval
to 2.5 seconds when it is stable and writable, because writable state will be
affected by the ping frequency. If a connection suddenly becomes bad (from a
writable state) and if we ping once every 2.5 seconds since it is writable, the
connection won't change to unwritable after 12.5 seconds because the state
change from writable to unwritable need 5 un-acked stun pings and at least 5
seconds. So we need to check if the last stun ping has been responded and if yes
use the 2.5 seconds interval. Otherwise use the 1.0 seconds interval. 
It would be something like that
int ping_interval = stable && conn->LastPingWasResponded() ?
config_.writable_connection_ping_interval :
 STABLIZING_WRITABLE_CONNECTION_PING_INTERVAL; 
and perhaps change the name of the constant 
STABLIZING_WRITABLE_CONNECTION_PING_INTERVAL

By doing so, we may still wait for up 6.5 seconds before detecting a connection
is not writable (compare to 5 seconds today). This is because we may have waited
for 2.5 seconds to send the next ping  and found it failed and from then on, it
needs to send 4 more stun pings before switching to unwritable state.  

[pthatcher1, 2016/06/02 18:03:02]

Yes, except you still need the std::min in there in case
config_.writable_connection_ping_interval is less than
STABLIZING_WRITABLE_CONNECTION_PING_INTERVAL (you don't want
STABLIZING_WRITABLE_CONNECTION_PING_INTERVAL to slow down pinging in that case).

[pthatcher1, 2016/06/02 18:03:02]

Yes, that's the tradeoff with low-frequency keep-alive, and we have two possible
mitigations:

1.  We could mark a connection as "unstable" if it hasn't received anything for
3 seconds, and then start pinging it at a higher rate.  This would allow us to
get 5 stun pings within 5 seconds if we don't receive anything.

2.  We can move away from using writability as the signal for "still connected"
and use receiving as the signal for "still connected" instead.  


I think what you're saying is that we should not submit this CL without at least
one of this mitigations.  Is that correct?

[honghaiz3, 2016/06/02 19:20:25]

I did not realize that we may have a case of writable_connection_ping_interval
smaller than the STABLIZING_WRITABLE_CONNECTION_PING_INTERVAL. Perhaps we should
enforce the former is always bigger than the latter.

Yes. I think not land the current CL as is. 
Other possible mitigations:
3. Start ping at a higher rate (once per second, not that high) if there are
ping pending and the last ping response was received more than 1 second ago.
This is similar to what I have suggested in the previous comments. 
4. Start ping at a higher rate (once per second) if the connection is not
receiving (the receiving timeout is adjustable). This is similar to your
mitigation 1 but using our receiving state and the timeout.  
I hope we don't introduce new states or new timeout values. 

+  // The connection will ping at a slower rate with two conditions:
+  // 1. The connection is writable.
+  // 2. The connection has sent enough initial pings for RTT to converge.
+  if (conn->writable() && !NeedToPingFast(conn)) {
+    return (now >=
+            conn->last_ping_sent() + config_.writable_connection_ping_interval);
+  }
+
   return conn->active();
 }
 
 // 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 interval. When reconnecting a TCP
+// past the writable ping interval. 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() {
   int64_t now = rtc::TimeMillis();
   Connection* conn_to_ping = nullptr;
-  if (best_connection_ && best_connection_->connected() &&
-      best_connection_->writable() &&
-      (best_connection_->last_ping_sent() + config_.max_strong_interval <=
-       now)) {
+  if (IsBestConnectionPingable()) {
     conn_to_ping = best_connection_;
   } else {
     conn_to_ping = FindConnectionToPing(now);
   }
   return conn_to_ping;
 }
 
 void P2PTransportChannel::MarkConnectionPinged(Connection* conn) {
   if (conn && pinged_connections_.insert(conn).second) {
     unpinged_connections_.erase(conn);
@@ skipping 277 matching lines @@
 
   // During the initial state when nothing has been pinged yet, return the first
   // one in the ordered |connections_|.
   return *(std::find_if(connections_.begin(), connections_.end(),
                         [conn1, conn2](Connection* conn) {
                           return conn == conn1 || conn == conn2;
                         }));
 }
 
 }  // namespace cricket