Replace readable with receiving where receiving means receiving anything (stun ping, response or da…

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..6f185d0238840e4490bf90ac4e06d74e4a54dbfb 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),
@@ -791,6 +791,7 @@ Connection::Connection(Port* port,
       remote_ice_mode_(ICEMODE_FULL),
       requests_(port->thread()),
       rtt_(DEFAULT_RTT),
+      connection_created_(rtc::Time()),
       last_ping_sent_(0),
       last_ping_received_(0),
       last_data_received_(0),
@@ -798,7 +799,8 @@ Connection::Connection(Port* port,
       sent_packets_discarded_(0),
       sent_packets_total_(0),
       reported_(false),
-      state_(STATE_WAITING) {
+      state_(STATE_WAITING),
+      receiving_timeout_(WEAK_CONNECTION_RECEIVE_TIMEOUT) {
   // All of our connections start in WAITING state.
   // TODO(mallinath) - Start connections from STATE_FROZEN.
   // Wire up to send stun packets
@@ -839,16 +841,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 +848,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,10 +900,12 @@ 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) {
-      // readable means data from this address is acceptable
-      // Send it on!
+    // If this connection received a ping or a ping response before, then pass
+    // along the packet. If the remote side is ICE-LITE, it may never receive
+    // a ping, but it needs to receive a ping response before receiving a data
+    // packet.
+    if (last_ping_received_ != 0 || last_ping_response_received_ != 0) {

[pthatcher1, 2015/09/17 21:45:11]

Why do we only do this if we've received a ping or ping response?  Why not
always do it?

[honghaiz3, 2015/09/18 02:35:46]

Basically I am set_receiving at the same time when setting one of the 
 last_data_received, last_ping_received, last_ping_response_received, so that
receiving state will remain consistent. 
If I always do it here (set receiving), but did not update the
last_data_received, then, after a short while, when the channel checks the
connection state, it may become not receiving (because last_data_received was
not set).  

[pthatcher1, 2015/09/18 17:54:42]

Sorry, when I said "why no always do it", I meant the whole block of code, not
just set_receiving(true).  Why don't we just remove this if statement and run
all the code below it every time we get a packet?

[honghaiz3, 2015/09/18 21:39:30]

OK. Just remove the condition. 

+      set_receiving(true);
       last_data_received_ = rtc::Time();
       recv_rate_tracker_.Update(size);
       SignalReadPacket(this, data, size, packet_time);
@@ -990,12 +991,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:
@@ -1022,8 +1018,7 @@ void Connection::Prune() {
 
 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) {
@@ -1087,7 +1082,6 @@ void Connection::UpdateState(uint32 now) {
                          << " rtt=" << rtt;
     set_write_state(STATE_WRITE_UNRELIABLE);
   }
-
   if ((write_state_ == STATE_WRITE_UNRELIABLE ||
        write_state_ == STATE_WRITE_INIT) &&
       TooLongWithoutResponse(pings_since_last_response_,
@@ -1099,6 +1093,20 @@ void Connection::UpdateState(uint32 now) {
                          << ", rtt=" << rtt;
     set_write_state(STATE_WRITE_TIMEOUT);
   }
+
+  // Check the receiving state.
+  uint32 last_recv_time = last_received();
+  bool receiving = now <= last_recv_time + receiving_timeout_;
+  set_receiving(receiving);
+  if (!receiving) {

[pthatcher1, 2015/09/17 21:45:11]

Please use an early return

if (receiving) {
  return;
}

[honghaiz3, 2015/09/18 02:35:47]

Done.

+    // If this connection has never received anything, use the connection
+    // creating time as the starting time for connection timeout.
+    last_recv_time = std::max(last_recv_time, connection_created_);

[pthatcher1, 2015/09/17 21:45:12]

I wouldn't re-assign to last_recv_time.  I would do something like:

uint32 receive_deadline = std::max(last_recv_time, connection_created_) +
DEAD_CONNECTION_RECEIVE_TIMEOUT;
if (now > receive_deadline) {
  Destroy();
}

[honghaiz3, 2015/09/18 02:35:47]

Just remove the temporary var. 

+    if (now > last_recv_time + DEAD_CONNECTION_RECEIVE_TIMEOUT) {
+      // Delete self.
+      Destroy();
+    }
+  }
 }
 
 void Connection::Ping(uint32 now) {
@@ -1112,8 +1120,8 @@ void Connection::Ping(uint32 now) {
 }
 
 void Connection::ReceivedPing() {
+  set_receiving(true);
   last_ping_received_ = rtc::Time();
-  set_read_state(STATE_READABLE);
 }
 
 void Connection::ReceivedPingResponse() {
@@ -1122,6 +1130,7 @@ void Connection::ReceivedPingResponse() {
   // So if we're not already, become writable. We may be bringing a pruned
   // 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();
@@ -1139,10 +1148,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 +1178,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 +1203,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 +1269,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_);
 }