Do not switch best connection on the controlled side too frequently

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 938 matching lines @@
   std::string remote_ufrag;
   const rtc::SocketAddress& addr(remote_candidate_.address());
   if (!port_->GetStunMessage(data, size, addr, &msg, &remote_ufrag)) {
     // The packet did not parse as a valid STUN message
     // This is a data packet, pass it along.
     set_receiving(true);
     last_data_received_ = rtc::TimeMillis();
     recv_rate_tracker_.AddSamples(size);
     SignalReadPacket(this, data, size, packet_time);
 
-    // If timed out sending writability checks, start up again
-    if (!pruned_ && (write_state_ == STATE_WRITE_TIMEOUT)) {
+    // If the connection has timed out, it might be because it was pruned or
+    // it failed the writability check. Start it up again if it was not pruned
+    // (so it is due to writability check failure), or it was pruned but it has
+    // been nominated by the controlling side.

[pthatcher1, 2016/06/22 19:18:37]

Perhaps the wording could be thus:

If writability timed out, it could be either because the connection was pruned
or because checks are failing.  If it's the later (not pruned), then reset the
connection state to the INIT state since clearly we can receive data on this
connection.  If it's the former (pruned) and the connection was nominated, do
the same, but also unprune the connection.  

[honghaiz3, 2016/06/22 20:01:24]

All changes here are removed. 
Acknowledged.

+    if (write_state_ == STATE_WRITE_TIMEOUT && (nominated() || !pruned_)) {
       LOG(LS_WARNING) << "Received a data packet on a timed-out Connection. "
                       << "Resetting state to STATE_WRITE_INIT.";
-      set_write_state(STATE_WRITE_INIT);
+      ResetWriteStateAndUnprune();

[pthatcher1, 2016/06/22 19:18:37]

Even though I corrected the comment, I don't understand why this is the behavior
that we want.  With aggressive nomination *all* of the connections will be
nominated, which means reset the write state for *all* connections that receive
media.  So basically the rule would be "unprune if we receive media from the
controlling side".   Do we really want that?  If so, can we save it for a future
CL?  I'd rather not block this CL on trying to get this part right.

And even if we do want this behavior, it would be more clear as this:
if (pruned_ && ice_role_ == ICE_CONTROLLED) {
  // If controlling side is sending on pruned candidate pair;  we should
probably unprune.
  Unprune();
}
if (!pruned_ && (write_state_ == STATE_WRITE_TIMEOUT)) {
  set_write_state(STATE_WRITE_INIT);
}


But even then, what prevents the candidate pair from getting re-pruned shortly
after this?  

[honghaiz3, 2016/06/22 20:01:24]

Acknowledged.

     }
   } else if (!msg) {
     // The packet was STUN, but failed a check and was handled internally.
   } else {
     // The packet is STUN and passed the Port checks.
     // Perform our own checks to ensure this packet is valid.
     // If this is a STUN request, then update the receiving bit and respond.
     // If this is a STUN response, then update the writable bit.
     // Log at LS_INFO if we receive a ping on an unwritable connection.
     rtc::LoggingSeverity sev = (!writable() ? rtc::LS_INFO : rtc::LS_VERBOSE);
@@ skipping 54 matching lines @@
     // Received conflicting role from the peer.
     LOG(LS_INFO) << "Received conflicting role from the peer.";
     return;
   }
 
   stats_.recv_ping_requests++;
 
   // This is a validated stun request from remote peer.
   port_->SendBindingResponse(msg, remote_addr);
 
-  // If it timed out on writing check, start up again
-  if (!pruned_ && write_state_ == STATE_WRITE_TIMEOUT) {
-    set_write_state(STATE_WRITE_INIT);
-  }
-
+  bool nominated = false;
   if (port_->GetIceRole() == ICEROLE_CONTROLLED) {
-    const StunByteStringAttribute* use_candidate_attr =
-        msg->GetByteString(STUN_ATTR_USE_CANDIDATE);
-    if (use_candidate_attr) {
+    bool nominated = msg->GetByteString(STUN_ATTR_USE_CANDIDATE) != nullptr;

[pthatcher1, 2016/06/22 19:18:37]

You are making the variable, so the outer |nominated| will always be false.

[honghaiz3, 2016/06/22 20:01:24]

Sorry for the mistake. 
That means the Unprune is indeed not so much useful (the issue will still happen
in some corner cases). 

+    if (nominated) {
       set_nominated(true);
       SignalNominated(this);
     }
   }
+  // If it timed out on writing check, start up again if it is not pruned or
+  // it is just nominated now, because the nomination may increase its priority
+  // in the pruning process.
+  if (write_state_ == STATE_WRITE_TIMEOUT && (nominated || !pruned_)) {
+    ResetWriteStateAndUnprune();
+  }

[pthatcher1, 2016/06/22 19:18:37]

When the controlling side is aggressive *all* the candidate pairs will be
nominated, so we'd basically be unpruning any candidate pairs that the
controlling is pinging, which means we never prune until we stop getting pings. 
Again, this change doesn't seem necessary for this CL, so could we leave it out
and discuss it later?

Also, with the current code, nomianted is always false, so it's never triggered
anyway.  And it's still passing the unit tests :).

[honghaiz3, 2016/06/22 20:01:24]

Acknowledged.

+
   // Set the remote cost if the network_info attribute is available.
   // Note: If packets are re-ordered, we may get incorrect network cost
   // temporarily, but it should get the correct value shortly after that.
   const StunUInt32Attribute* network_attr =
       msg->GetUInt32(STUN_ATTR_NETWORK_INFO);
   if (network_attr) {
     uint32_t network_info = network_attr->value();
     uint16_t network_cost = static_cast<uint16_t>(network_info);
     if (network_cost != remote_candidate_.network_cost()) {
       remote_candidate_.set_network_cost(network_cost);
@@ skipping 13 matching lines @@
 void Connection::Prune() {
   if (!pruned_ || active()) {
     LOG_J(LS_INFO, this) << "Connection pruned";
     pruned_ = true;
     requests_.Clear();
     set_write_state(STATE_WRITE_TIMEOUT);
   }
 }
 
 void Connection::Destroy() {
-  LOG_J(LS_VERBOSE, this) << "Connection destroyed";
   port_->thread()->Post(RTC_FROM_HERE, this, MSG_DELETE);
 }
 
 void Connection::FailAndDestroy() {
   set_state(Connection::STATE_FAILED);
   Destroy();
 }
 
 void Connection::PrintPingsSinceLastResponse(std::string* s, size_t max) {
   std::ostringstream oss;
@@ skipping 322 matching lines @@
                << num_pings_sent_;
   SignalDestroyed(this);
   delete this;
 }
 
 int64_t Connection::last_received() const {
   return std::max(last_data_received_,
              std::max(last_ping_received_, last_ping_response_received_));
 }
 
+void Connection::ResetWriteStateAndUnprune() {
+  write_state_ = STATE_WRITE_INIT;
+  LOG_J(INFO, this) << "Connection unpruned";
+  if (pruned_) {
+    pruned_ = false;
+  }
+}
+
 ConnectionInfo Connection::stats() {
   stats_.recv_bytes_second = round(recv_rate_tracker_.ComputeRate());
   stats_.recv_total_bytes = recv_rate_tracker_.TotalSampleCount();
   stats_.sent_bytes_second = round(send_rate_tracker_.ComputeRate());
   stats_.sent_total_bytes = send_rate_tracker_.TotalSampleCount();
   return stats_;
 }
 
 void Connection::MaybeAddPrflxCandidate(ConnectionRequest* request,
                                         StunMessage* response) {
@@ skipping 93 matching lines @@
     ASSERT(sent < 0);
     error_ = port_->GetError();
     stats_.sent_discarded_packets++;
   } else {
     send_rate_tracker_.AddSamples(sent);
   }
   return sent;
 }
 
 }  // namespace cricket