A few updates on connection states

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 8048201754b737dabdfa1901e020521cacc4ef84..be729198c071216ce74c430eee2f3ffd262febe4 100644
--- a/webrtc/p2p/base/port.cc
+++ b/webrtc/p2p/base/port.cc
@@ -782,8 +782,8 @@ Connection::Connection(Port* port,
     : port_(port),
       local_candidate_index_(index),
       remote_candidate_(remote_candidate),
-      read_state_(STATE_READ_INIT),
       write_state_(STATE_WRITE_INIT),
+      receiving_(false),
       connected_(true),
       pruned_(false),
       use_candidate_attr_(false),
@@ -798,7 +798,8 @@ Connection::Connection(Port* port,
       sent_packets_discarded_(0),
       sent_packets_total_(0),
       reported_(false),
-      state_(STATE_WAITING) {
+      state_(STATE_WAITING),
+      receiving_timeout_(CONNECTION_RECEIVE_SHORT_TIMEOUT) {
   // All of our connections start in WAITING state.
   // TODO(mallinath) - Start connections from STATE_FROZEN.
   // Wire up to send stun packets
@@ -839,16 +840,6 @@ uint64 Connection::priority() const {
   return priority;
 }
 
-void Connection::set_read_state(ReadState value) {
-  ReadState old_value = read_state_;
-  read_state_ = value;
-  if (value != old_value) {
-    LOG_J(LS_VERBOSE, this) << "set_read_state";
-    SignalStateChange(this);
-    CheckTimeout();
-  }
-}
-
 void Connection::set_write_state(WriteState value) {
   WriteState old_value = write_state_;
   write_state_ = value;
@@ -856,7 +847,14 @@ void Connection::set_write_state(WriteState value) {
     LOG_J(LS_VERBOSE, this) << "set_write_state from: " << old_value << " to "
                             << value;
     SignalStateChange(this);
-    CheckTimeout();
+  }
+}
+
+void Connection::set_receiving(bool value) {
+  if (value != receiving_) {
+    LOG_J(LS_VERBOSE, this) << "set_receiving to " << value;
+    receiving_ = value;
+    SignalStateChange(this);
   }
 }
 
@@ -901,8 +899,10 @@ void Connection::OnReadPacket(
   if (!port_->GetStunMessage(data, size, addr, msg.accept(), &remote_ufrag)) {
     // The packet did not parse as a valid STUN message
 
-    // If this connection is readable, then pass along the packet.
-    if (read_state_ == STATE_READABLE) {
+    // If this connection received ping before, then pass along the packet.

[pthatcher1, 2015/09/17 05:58:42]

received ping => receive a ping

[honghaiz3, 2015/09/17 19:47:56]

Done.

+    // If the remote side is ICE-LITE, it may never receive ping, but it needs

[pthatcher1, 2015/09/17 05:58:42]

receive ping => receive a ping

[honghaiz3, 2015/09/17 19:47:56]

Done.

+    // to receive ping response before receiving data packet.

[pthatcher1, 2015/09/17 05:58:42]

receive ping => receive a ping
receive data packet => receive a data packet

[honghaiz3, 2015/09/17 19:47:56]

Done.

+    if (last_ping_received_ != 0 || last_ping_response_received_) {

[pthatcher1, 2015/09/17 05:58:42]

I don't understand why we even bother to check these.  If we've receive a data
packet form the remote side, why would we throw it away?

Even if we are making this a refactoring with no behavior change, wouldn't we
make it check only the last_ping_received_, not the
last_ping_response_received_?

[honghaiz3, 2015/09/17 19:47:56]

Yes the primary purpose is to make it have the same or similar behavior. 
For ICE_LITE, it set the read state to READABLE when receiving a ping response.
So adding the last_ping_response_received is trying to have similar behavior. 
I agree the checking may not be necessary. But I guess it tries to enforce the
protocol that other side must send stun ping to check the writability before
sending data. See the bottom of the deleted code in this CL about ICE_LITE. 

       // readable means data from this address is acceptable
       // Send it on!
       last_data_received_ = rtc::Time();
@@ -990,12 +990,7 @@ void Connection::OnReadPacket(
       // Otherwise we can mark connection to read timeout. No response will be
       // sent in this scenario.
       case STUN_BINDING_INDICATION:
-        if (read_state_ == STATE_READABLE) {
-          ReceivedPing();
-        } else {
-          LOG_J(LS_WARNING, this) << "Received STUN binding indication "
-                                  << "from an unreadable connection.";
-        }
+        ReceivedPing();
         break;
 
       default:
@@ -1016,14 +1011,12 @@ void Connection::Prune() {
     LOG_J(LS_VERBOSE, this) << "Connection pruned";
     pruned_ = true;
     requests_.Clear();

[pthatcher1, 2015/09/17 05:58:42]

There's a lot going on in one CL.  Could we break this up into two CLs?

1.  Remove readable state and use receiving instead.
2.  Change behavior of write state, pruning, etc.

I think that would make the diffs easier to understand, and make the CLs less
risky.

[honghaiz3, 2015/09/17 19:47:56]

It is kind of convoluted. I created a new CL only changing the receiving only.
But this CL will depend on that. 

-    set_write_state(STATE_WRITE_TIMEOUT);
   }
 }
 
 void Connection::Destroy() {
   LOG_J(LS_VERBOSE, this) << "Connection destroyed";
-  set_read_state(STATE_READ_TIMEOUT);
-  set_write_state(STATE_WRITE_TIMEOUT);
+  port_->thread()->Post(this, MSG_DELETE);
 }
 
 void Connection::PrintPingsSinceLastResponse(std::string* s, size_t max) {
@@ -1099,6 +1092,17 @@ void Connection::UpdateState(uint32 now) {
                          << ", rtt=" << rtt;
     set_write_state(STATE_WRITE_TIMEOUT);
   }
+
+  // Check the receiving state.
+  uint32 last_recv_time = last_received();

[pthatcher1, 2015/09/17 05:58:42]

Might make sense to write it as:

uint32 time_since_last_received = now - last_received();
if (receiving_ && time_since_last_received > receiving_timeout_) {
  set_receiving(false);
} if (time_since_last_received > DEAD) {
  Destroy();
}

[honghaiz3, 2015/09/17 19:47:56]

This saved an operation. But using subtraction on unsigned number may have
unexpected overflow/underflow. If now < last_received, now - last_received
becomes a big number. Although it should not happen here, it is probably better
to just avoid subtraction at all for unsigned numbers. 

+  if (receiving_) {
+    if (now > last_recv_time + receiving_timeout_) {
+      set_receiving(false);
+    }
+  } else if (now > last_recv_time + CONNECTION_RECEIVE_LONG_TIMEOUT) {
+    // Delete self.
+    Destroy();
+  }
 }
 
 void Connection::Ping(uint32 now) {
@@ -1113,7 +1117,6 @@ void Connection::Ping(uint32 now) {
 
 void Connection::ReceivedPing() {
   last_ping_received_ = rtc::Time();
-  set_read_state(STATE_READABLE);
 }
 
 void Connection::ReceivedPingResponse() {
@@ -1139,10 +1142,9 @@ std::string Connection::ToString() const {
     '-',  // not connected (false)
     'C',  // connected (true)
   };
-  const char READ_STATE_ABBREV[3] = {
-    '-',  // STATE_READ_INIT
-    'R',  // STATE_READABLE
-    'x',  // STATE_READ_TIMEOUT
+  const char RECEIVE_STATE_ABBREV[2] = {
+    '-',  // not receiving (false)
+    'R',  // receiving (true)
   };
   const char WRITE_STATE_ABBREV[4] = {
     'W',  // STATE_WRITABLE
@@ -1170,7 +1172,7 @@ std::string Connection::ToString() const {
      << ":" << remote.type() << ":"
      << remote.protocol() << ":" << remote.address().ToSensitiveString() << "|"
      << CONNECT_STATE_ABBREV[connected()]
-     << READ_STATE_ABBREV[read_state()]
+     << RECEIVE_STATE_ABBREV[receiving()]
      << WRITE_STATE_ABBREV[write_state()]
      << ICESTATE[state()] << "|"
      << priority() << "|";
@@ -1195,11 +1197,6 @@ void Connection::OnConnectionRequestResponse(ConnectionRequest* request,
   uint32 rtt = request->Elapsed();
 
   ReceivedPingResponse();
-  if (remote_ice_mode_ == ICEMODE_LITE) {
-    // A ice-lite end point never initiates ping requests. This will allow
-    // us to move to STATE_READABLE without an incoming ping request.
-    set_read_state(STATE_READABLE);
-  }
 
   if (LOG_CHECK_LEVEL_V(sev)) {
     bool use_candidate = (
@@ -1266,19 +1263,6 @@ void Connection::OnConnectionRequestSent(ConnectionRequest* request) {
                     << ", use_candidate=" << use_candidate;
 }
 
-void Connection::CheckTimeout() {
-  // If both read and write have timed out or read has never initialized, then
-  // this connection can contribute no more to p2p socket unless at some later
-  // date readability were to come back.  However, we gave readability a long
-  // time to timeout, so at this point, it seems fair to get rid of this
-  // connection.
-  if ((read_state_ == STATE_READ_TIMEOUT ||
-       read_state_ == STATE_READ_INIT) &&
-      write_state_ == STATE_WRITE_TIMEOUT) {
-    port_->thread()->Post(this, MSG_DELETE);
-  }
-}
-
 void Connection::HandleRoleConflictFromPeer() {
   port_->SignalRoleConflict(port_);
 }