Delete a connection only if it has timed out on writing and not receiving for 10 seconds.

webrtc/p2p/base/port.cc

Index: webrtc/p2p/base/port.cc
diff --git a/webrtc/p2p/base/port.cc b/webrtc/p2p/base/port.cc
index 842a26e8260df55b740eed7527c13a22698a7309..89bbeebba32b51223aa1b3e89223ba91ae484a00 100644
--- a/webrtc/p2p/base/port.cc
+++ b/webrtc/p2p/base/port.cc
@@ -801,7 +801,8 @@ Connection::Connection(Port* port,
       sent_packets_total_(0),
       reported_(false),
       state_(STATE_WAITING),
-      receiving_timeout_(WEAK_CONNECTION_RECEIVE_TIMEOUT) {
+      receiving_timeout_(WEAK_CONNECTION_RECEIVE_TIMEOUT),
+      time_created_ms_(rtc::Time()) {
   // All of our connections start in WAITING state.
   // TODO(mallinath) - Start connections from STATE_FROZEN.
   // Wire up to send stun packets
@@ -849,7 +850,6 @@ void Connection::set_write_state(WriteState value) {
     LOG_J(LS_VERBOSE, this) << "set_write_state from: " << old_value << " to "
                             << value;
     SignalStateChange(this);
-    CheckTimeout();
   }
 }
 
@@ -858,7 +858,6 @@ void Connection::set_receiving(bool value) {
     LOG_J(LS_VERBOSE, this) << "set_receiving to " << value;
     receiving_ = value;
     SignalStateChange(this);
-    CheckTimeout();
   }
 }
 
@@ -1089,6 +1088,7 @@ void Connection::UpdateState(uint32 now) {
   uint32 last_recv_time = last_received();
   bool receiving = now <= last_recv_time + receiving_timeout_;
   set_receiving(receiving);
+  CheckTimeout(now);
 }
 
 void Connection::Ping(uint32 now) {
@@ -1251,10 +1251,16 @@ void Connection::OnConnectionRequestSent(ConnectionRequest* request) {
                     << ", use_candidate=" << use_candidate;
 }
 
-void Connection::CheckTimeout() {
-  // If write has timed out and it is not receiving, remove the connection.
+void Connection::CheckTimeout(uint32 now) {

[pthatcher1, 2015/09/28 23:07:27]

If this is now called in only one place, why not remove this method and simply
call it in that one place?

[honghaiz3, 2015/09/29 18:47:50]

Done.

   if (!receiving_ && write_state_ == STATE_WRITE_TIMEOUT) {
-    Destroy();
+    // Remove the connection only if it has timed out on writing and it has
+    // not received anything for a long time. If the connection has never
+    // received anything before, |last_received()| will return 0. In this case,
+    // use the connection creation time as the base to compute the timeout.

[pthatcher1, 2015/09/28 23:07:27]

I think this comment ought to say something more like "Don't destroy a
connection until N seconds have passed since it was created.  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."

[honghaiz3, 2015/09/29 18:47:49]

Done.

+    uint32 base_for_timeout = std::max(time_created_ms_, last_received());

[pthatcher1, 2015/09/28 23:07:27]

I think call this "receiving_deadline" would be a bit better:

uint32 receiving_deadline = std::max(time_created_ms_, last_received()) +
DEAD_CONNECTION_RECEIVE_TIMEOUT.

[honghaiz3, 2015/09/29 18:47:50]

Done.

+    if (now > base_for_timeout + DEAD_CONNECTION_RECEIVE_TIMEOUT) {
+      Destroy();
+    }
   }

[pthatcher1, 2015/09/28 23:07:27]

This would be more clear with early returns.  

[honghaiz3, 2015/09/29 18:47:49]

Done.

 }

[pthatcher1, 2015/09/28 23:07:27]

I think this would be more clear with a helper method called something like
"Dead":  I also think we could combine all my other comments into something like
this:


void Connection::UpdateState(uint32 now) {
  ...
  if (Dead(now)) {
    Destroy();
  }
}

void Connection::Active() {
  // TODO: Move from using the write_state_ to using pruned_.
  return write_state_ != STATE_WRITE_TIMEOUT;
}

bool Connection::Dead(uint32 now) {
  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 (receiving_) {
    // A connection that is receiving is alive.
    return false;
  }

  // A connection is alive until it is inactive.
  return !Active();

  // TODO: Move from using the write_state_ to using the receiving state with
something like the following:
  // return (now > (last_received() + DEAD_CONNECTION_RECEIVE_TIMEOUT));
}

[honghaiz3, 2015/09/29 18:47:50]

Done.