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

webrtc/p2p/base/p2ptransportchannel.cc

Index: webrtc/p2p/base/p2ptransportchannel.cc
diff --git a/webrtc/p2p/base/p2ptransportchannel.cc b/webrtc/p2p/base/p2ptransportchannel.cc
index 094a8dcc8f1ffc937a436dc85890633c62d60760..683cbba6c29881ed20af6dc3193dce7415d9546b 100644
--- a/webrtc/p2p/base/p2ptransportchannel.cc
+++ b/webrtc/p2p/base/p2ptransportchannel.cc
@@ -221,6 +221,7 @@ void P2PTransportChannel::AddAllocatorSession(PortAllocatorSession* session) {
 void P2PTransportChannel::AddConnection(Connection* connection) {
   connections_.push_back(connection);
   connection->set_remote_ice_mode(remote_ice_mode_);
+  connection->set_receiving_timeout(receiving_timeout_);
   connection->SignalReadPacket.connect(
       this, &P2PTransportChannel::OnReadPacket);
   connection->SignalReadyToSend.connect(
@@ -340,6 +341,10 @@ void P2PTransportChannel::SetReceivingTimeout(int receiving_timeout_ms) {
   receiving_timeout_ = receiving_timeout_ms;
   check_receiving_delay_ =
       std::max(MIN_CHECK_RECEIVING_DELAY, receiving_timeout_ / 10);
+
+  for (Connection* connection : connections_) {
+    connection->set_receiving_timeout(receiving_timeout_);
+  }
   LOG(LS_VERBOSE) << "Set ICE receiving timeout to " << receiving_timeout_
                   << " milliseconds";
 }
@@ -400,7 +405,7 @@ void P2PTransportChannel::OnPortReady(PortAllocatorSession *session,
   std::vector<RemoteCandidate>::iterator iter;
   for (iter = remote_candidates_.begin(); iter != remote_candidates_.end();
        ++iter) {
-    CreateConnection(port, *iter, iter->origin_port(), false);
+    CreateConnection(port, *iter, iter->origin_port());
   }
 
   SortConnections();
@@ -616,7 +621,7 @@ void P2PTransportChannel::OnCandidate(const Candidate& candidate) {
   }
 
   // Create connections to this remote candidate.
-  CreateConnections(candidate, NULL, false);
+  CreateConnections(candidate, NULL);
 
   // Resort the connections list, which may have new elements.
   SortConnections();
@@ -626,8 +631,7 @@ void P2PTransportChannel::OnCandidate(const Candidate& candidate) {
 // remote candidate.  The return value is true if we created a connection from
 // the origin port.
 bool P2PTransportChannel::CreateConnections(const Candidate& remote_candidate,
-                                            PortInterface* origin_port,
-                                            bool readable) {
+                                            PortInterface* origin_port) {
   ASSERT(worker_thread_ == rtc::Thread::Current());
 
   Candidate new_remote_candidate(remote_candidate);
@@ -665,7 +669,7 @@ bool P2PTransportChannel::CreateConnections(const Candidate& remote_candidate,
   bool created = false;
   std::vector<PortInterface *>::reverse_iterator it;
   for (it = ports_.rbegin(); it != ports_.rend(); ++it) {
-    if (CreateConnection(*it, new_remote_candidate, origin_port, readable)) {
+    if (CreateConnection(*it, new_remote_candidate, origin_port)) {
       if (*it == origin_port)
         created = true;
     }
@@ -673,8 +677,7 @@ bool P2PTransportChannel::CreateConnections(const Candidate& remote_candidate,
 
   if ((origin_port != NULL) &&
       std::find(ports_.begin(), ports_.end(), origin_port) == ports_.end()) {
-    if (CreateConnection(
-        origin_port, new_remote_candidate, origin_port, readable))
+    if (CreateConnection(origin_port, new_remote_candidate, origin_port))
       created = true;
   }
 
@@ -688,8 +691,7 @@ bool P2PTransportChannel::CreateConnections(const Candidate& remote_candidate,
 // And then listen to connection object for changes.
 bool P2PTransportChannel::CreateConnection(PortInterface* port,
                                            const Candidate& remote_candidate,
-                                           PortInterface* origin_port,
-                                           bool readable) {
+                                           PortInterface* origin_port) {
   // Look for an existing connection with this remote address.  If one is not
   // found, then we can create a new connection for this address.
   Connection* connection = port->GetConnection(remote_candidate.address());
@@ -724,11 +726,6 @@ bool P2PTransportChannel::CreateConnection(PortInterface* port,
                          << connections_.size() << " total)";
   }
 
-  // If we are readable, it is because we are creating this in response to a
-  // ping from the other side.  This will cause the state to become readable.
-  if (readable)
-    connection->ReceivedPing();
-
   return true;
 }
 
@@ -851,10 +848,13 @@ bool P2PTransportChannel::GetStats(ConnectionInfos *infos) {
   std::vector<Connection *>::const_iterator it;
   for (it = connections_.begin(); it != connections_.end(); ++it) {
     Connection *connection = *it;
+    // Don't report stats if the connection is pruned and it is not receiving.
+    if (connection->pruned() && !connection->receiving()) {

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

It might be nice to have a method connection->pinging() ==
!connection->pruned().

Then we could write this:

if (!connection->pinging() && !connection->receiving()) {
  continue;
}

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

Having one method just returning the negative of another method looks redundant
(and may cause confusing). Since it will be used only once here, it might be
sufficient to just add some comments.  If you want to replace pruned with
!pinging everywhere, maybe we can do it in the next CL. WDYT?

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

It's OK.  We can just leave it pruned() and remember that pruned() == !pinging. 
Actually, can you just leave a nice comment above the pruned() method that says
"This means we're not pinging."?

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

In fact we don't need this for this CL now since we are deleting them when
pruning them (Previously it kept more connections causing test failures). 

+      continue;
+    }
     ConnectionInfo info;
     info.best_connection = (best_connection_ == connection);
-    info.readable =
-        (connection->read_state() == Connection::STATE_READABLE);
+    info.receiving = connection->receiving();
     info.writable =
         (connection->write_state() == Connection::STATE_WRITABLE);
     info.timeout =
@@ -1046,14 +1046,8 @@ void P2PTransportChannel::UpdateChannelState() {
   if (writable != this->writable())
     LOG(LS_ERROR) << "UpdateChannelState: writable state mismatch";
 
-  bool readable = false;
-  for (uint32 i = 0; i < connections_.size(); ++i) {
-    if (connections_[i]->read_state() == Connection::STATE_READABLE) {
-      readable = true;
-      break;
-    }
-  }
-  set_readable(readable);
+  // TODO(honghaiz): channel receiving state is set in the OnCheckReceiving.

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

channel => The channel
in the OnCheckingReceiving => in OnCheckingReceiving.

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

Done.

+  // Will revisit in a subsequent code change.
 }
 
 // We checked the status of our connections and we had at least one that
@@ -1171,23 +1165,14 @@ bool P2PTransportChannel::IsPingable(Connection* conn) {
   // An never connected connection cannot be written to at all, so pinging is
   // out of the question. However, if it has become WRITABLE, it is in the
   // reconnecting state so ping is needed.
-  if (!conn->connected() && conn->write_state() != Connection::STATE_WRITABLE) {
+  if (!conn->connected() && !conn->writable()) {
     return false;
   }
 
-  if (writable()) {
-    // If we are writable, then we only want to ping connections that could be
-    // better than this one, i.e., the ones that were not pruned.
-    return (conn->write_state() != Connection::STATE_WRITE_TIMEOUT);
-  } else {
-    // If we are not writable, then we need to try everything that might work.
-    // This includes both connections that do not have write timeout as well as
-    // ones that do not have read timeout.  A connection could be readable but
-    // be in write-timeout if we pruned it before.  Since the other side is
-    // still pinging it, it very well might still work.
-    return (conn->write_state() != Connection::STATE_WRITE_TIMEOUT) ||
-           (conn->read_state() != Connection::STATE_READ_TIMEOUT);
-  }
+  // If the channel is not writable, ping all candidates. Otherwise, we only
+  // want to ping connections that could be better than this one,
+  // i.e., the ones that were not pruned.
+  return !writable() || conn->write_state() != Connection::STATE_WRITE_TIMEOUT;

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

The comment seems to indicate the code should be:

return !writable() || !conn->pruned();


Or this:

return conn->pinging() || !writable();

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

The current code and comments tried to be similar to the original code.
When Prune is called, it simply set the connection state to WRITE_TIMEOUT. 
The difference is that the write_state may still come out of the WRITE_TIMEOUT
but pruned will never. Plus, if a high-priority connection has timed out on
writing, it does not need to be pinged unless the channel is not writable. So I
prefer the existing code but change the comments. 
It probably does not matter that much considering we are timing out connections
and deleting them.  

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

How does a connection go from WRITE_TIMEOUT to WRITABLE if it's pruned?  If it's
pruned, then it's not pinging.  If it's not pinging, then it can't get out of
WRITE_TIMEOUT.  

I think your original comment and "!writable() || !conn->pruned();" is more
readable than the new comment and "!writable() || conn->write_state() !=
Connection::STATE_WRITE_TIMEOUT;".

If they are equivalent, then we should go with the more readable one.  If they
aren't, then we should go with the correct one.

So the big question is: Under what conditions are they different, if any?  I
don't see one, but perhaps you do.  If you do, could you please explain it?

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

Plus, write_out may be caused by just writing failures. 
So don't need to ping some high-priority connections which cannot write all the
time even if they are not pruned. 
On 2015/09/18 17:54:42, pthatcher1 wrote:
pruned is not always equal to write_timeout. 
When pruning, it will set write state to write_timeout. 
pruned state will never change back. 
But in the state of write_timeout may change back to writable (if the transport
channel becomes not writable, it will try to ping everything, so a previous
write_timeout connection may become writable again, although the chance is
small. 

 }
 
 // Returns the next pingable connection to ping.  This will be the oldest