Delete a connection after it is pruned or becomes write_timeout

webrtc/p2p/base/port.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 1106 matching lines @@
   // connection back to life, but if we don't really want it, we can always
   // prune it again.
   set_receiving(true);
   set_write_state(STATE_WRITABLE);
   set_state(STATE_SUCCEEDED);
   pings_since_last_response_.clear();
   last_ping_response_received_ = rtc::Time();
 }
 
 bool Connection::dead(uint32_t now) const {
-  if (now < (time_created_ms_ + MIN_CONNECTION_LIFETIME)) {
-    // A connection that hasn't passed its minimum lifetime is still alive.
-    // We do this to prevent connections from being pruned too quickly
-    // during a network change event when two networks would be up
-    // simultaneously but only for a brief period.
-    return false;
+  if (last_received() == 0) {
+    // If a connection has never received anything, it will be dead only if
+    // it is inactive and has passed its minimum lifetime. We do this to prevent
+    // a connection may be deleted too quickly because it has not got a chance
+    // to be pinged, or it may be pruned too quickly during a network change
+    // event when two networks would be up simultaneously but only for a
+    // brief period.
+    return !active() && now > (time_created_ms_ + MIN_CONNECTION_LIFETIME);
   }

[pthatcher1, 2015/12/30 17:12:28]

Let me see if I understand the purpose of this CL correctly:  Currently, we are
creating a connection, it's still actively pinging, but it hasn't received
anything yet, and we decide it's dead, perhaps because it never had enough time
to ping and receive something.  

In that case, I think something like this might be more readable:

if (last_received() > 0) {
  // If it has ever received anything, we keep it alive until it hasn't received
anything for DEAD_CONNECTION_RECEIVE_TIMEOUT
  // This covers the normal case of a successfully used connection that stops
working.
  // This also allows a remote peer to continue pinging over a locally inactive
(pruned) connection.
  return (now > (last_received() + DEAD_CONNECTION_RECEIVE_TIMEOUT)); 
}

if (active()) {
  // If it has never received anything, but it is actively pinging (not pruned),

  // it is alive at least as long as it is pinging.  Otherwise, the connection
might be deleted
  // before it has a chance to ping.
  // This is the normal case for a new connection that is pinging but hasn't
received anything yet.
  return false;
}

// if it has never received anything and it not actively pinging (pruned), but
we keep it around at least MIN_CONNECTION_LIFETIME
// to prevent connections from being pruned too quickly during a network change
event when two networks would be up
// simultaneously but only for a brief period.
return now > (time_created_ms_ + MIN_CONNECTION_LIFETIME);

[honghaiz3, 2015/12/30 19:38:18]

Yes. More precisely, having 10 seconds for the minimum lifetime is not enough in
some cases. Some good connection may not have a chance to send a ping before it
is deleted.
This happened sometimes when both sides have two networks (so more connections
are created). 

 
   // It is dead if it has not received anything for
   // DEAD_CONNECTION_RECEIVE_TIMEOUT milliseconds.
   return (now > (last_received() + DEAD_CONNECTION_RECEIVE_TIMEOUT));
 }
 
 std::string Connection::ToDebugId() const {
   std::stringstream ss;
   ss << std::hex << this;
   return ss.str();
@@ skipping 268 matching lines @@
     ASSERT(sent < 0);
     error_ = port_->GetError();
     sent_packets_discarded_++;
   } else {
     send_rate_tracker_.AddSamples(sent);
   }
   return sent;
 }
 
 }  // namespace cricket