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)) {
+    // Start up again if timed out sending writability checks, or
+    // this is nominated on the controlled side because on the controlled side,
+    // because receiving data will increase its priority in the pruning process.

[pthatcher1, 2016/06/22 06:36:34]

I don't understand this comment.

[honghaiz3, 2016/06/22 08:03:17]

Revised.

+    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 06:36:34]

So the change is to unprune when we received media on a nominated pair?  Why
would we need to do that?   Is that just so that we unprune backup candidate
pairs on the controlled side when the controlling side switches to one?  If so,
is this just here until we're fully switched over to "renomination"?   Also, can
the comment reflect that such logic is only for backup candidate pairs of
aggressively controlling peers?  Finally, why did we not need this logic before?
 

[honghaiz3, 2016/06/22 08:03:17]

I think there is a bug in the existing code that sometimes a connection used by
the controlling side may be pruned before it is selected by the controlled side.
Consider the case that two connections having the same candidate priority.
Whichever first becomes writable will make the other one pruned in the
controlled side but the other one may be the one chosen by the controlling side.
Yes we do have the logic on the controlled side that it only start to prune only
if it selected connection is nominated but the condition may not be so much
useful because with aggressive nomination, it nominated every connection before
the best connection becomes writable. 

     }
   } 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);
-  }
-
+  // Whether it is nominated in this request but not nominated before.
+  bool just_nominated = false;
   if (port_->GetIceRole() == ICEROLE_CONTROLLED) {
-    const StunByteStringAttribute* use_candidate_attr =
-        msg->GetByteString(STUN_ATTR_USE_CANDIDATE);
-    if (use_candidate_attr) {
+    bool use_candidate = msg->GetByteString(STUN_ATTR_USE_CANDIDATE) != nullptr;
+    if (use_candidate) {
+      if (!nominated()) {
+        just_nominated = true;
+      }
       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 && (just_nominated || !pruned_)) {

[Taylor Brandstetter, 2016/06/21 18:33:26]

Why is the connection only restored if it's "just_nominated"? The controlling
agent could switch connections back and forth over the course of a session due
to RTT, right?

[pthatcher1, 2016/06/22 06:36:34]

Yeah, I don't understand the "just nominated" logic either.  If we don't prune
until a candidate pair is nominated and writable, then the fact that the
candidate pair was pruned means a better candidate pair was writable and
nominated.  So why would be unprune this candidate pair, which we know to be
worse?  It seems contrary to the concept of pruning when the controlling side is
aggressive.  We're just going to prune it again once the better candidate pair
is nominated again.

[honghaiz3, 2016/06/22 08:03:17]

I think you are right. I may have over-thought this. 

[honghaiz3, 2016/06/22 08:03:17]

See my reply in the above comments. 

+    ResetWriteStateAndUnprune();
+  }
+
   // 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 447 matching lines @@
     ASSERT(sent < 0);
     error_ = port_->GetError();
     stats_.sent_discarded_packets++;
   } else {
     send_rate_tracker_.AddSamples(sent);
   }
   return sent;
 }
 
 }  // namespace cricket