Do not switch best connection on the controlled side too frequently

webrtc/p2p/base/p2ptransportchannel_unittest.cc

 /*
  *  Copyright 2009 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 1959 matching lines @@
 
   int SendData(cricket::TransportChannel& channel,
                const char* data,
                size_t len,
                int packet_id) {
     rtc::PacketOptions options;
     options.packet_id = packet_id;
     return channel.SendPacket(data, len, options, 0);
   }
 
+  cricket::Connection* CreateConnectionWithCandidate(
+      cricket::P2PTransportChannel& channel,
+      const std::string& ip_addr,
+      int port,
+      int priority,
+      bool writable) {
+    channel.AddRemoteCandidate(CreateHostCandidate(ip_addr, port, priority));
+    cricket::Connection* conn = WaitForConnectionTo(&channel, ip_addr, port);
+    if (conn && writable) {
+      conn->ReceivedPingResponse();  // make it writable
+    }
+    return conn;
+  }
+
+  void NominateConnection(cricket::Connection* conn) {
+    conn->set_nominated(true);
+    conn->SignalNominated(conn);
+  }
+
   void OnSelectedCandidatePairChanged(
       cricket::TransportChannel* transport_channel,
       cricket::CandidatePairInterface* selected_candidate_pair,
       int last_sent_packet_id) {
     last_selected_candidate_pair_ = selected_candidate_pair;
     last_sent_packet_id_ = last_sent_packet_id;
+    ++num_selected_candidate_pair_switch_;

[pthatcher1, 2016/06/15 21:59:53]

switch_ => switches_

And you can probably drop "num_" from the front.

[honghaiz3, 2016/06/16 23:03:49]

Done.

   }
 
   void ReceivePingOnConnection(cricket::Connection* conn,
                                const std::string& remote_ufrag,
                                int priority) {
     cricket::IceMessage msg;
     msg.SetType(cricket::STUN_BINDING_REQUEST);
     msg.AddAttribute(new cricket::StunByteStringAttribute(
         cricket::STUN_ATTR_USERNAME,
         conn->local_candidate().username() + ":" + remote_ufrag));
@@ skipping 15 matching lines @@
     channel_state_ = channel->GetState();
   }
 
   cricket::CandidatePairInterface* last_selected_candidate_pair() {
     return last_selected_candidate_pair_;
   }
   int last_sent_packet_id() { return last_sent_packet_id_; }
   bool channel_ready_to_send() { return channel_ready_to_send_; }
   void reset_channel_ready_to_send() { channel_ready_to_send_ = false; }
   cricket::TransportChannelState channel_state() { return channel_state_; }
+  int num_selected_candidate_pair_switch() {
+    return num_selected_candidate_pair_switch_;
+  }
+  void reset_num_selected_candidate_pair_switch() {
+    num_selected_candidate_pair_switch_ = 0;
+  }
 
  private:
   std::unique_ptr<rtc::PhysicalSocketServer> pss_;
   std::unique_ptr<rtc::VirtualSocketServer> vss_;
   rtc::SocketServerScope ss_scope_;
   cricket::CandidatePairInterface* last_selected_candidate_pair_ = nullptr;
+  int num_selected_candidate_pair_switch_ = 0;
   int last_sent_packet_id_ = -1;
   bool channel_ready_to_send_ = false;
   cricket::TransportChannelState channel_state_ = cricket::STATE_INIT;
 };
 
 TEST_F(P2PTransportChannelPingTest, TestTriggeredChecks) {
   cricket::FakePortAllocator pa(rtc::Thread::Current(), nullptr);
   cricket::P2PTransportChannel ch("trigger checks", 1, &pa);
   PrepareChannel(&ch);
   ch.Connect();
@@ skipping 429 matching lines @@
   EXPECT_EQ(conn3, ch.best_connection());
 
   // Now another data packet will not switch the best connection because the
   // best connection was nominated by the controlling side.
   conn2->ReceivedPing();
   conn2->ReceivedPingResponse();
   conn2->OnReadPacket("XYZ", 3, rtc::CreatePacketTime(0));
   EXPECT_EQ(conn3, ch.best_connection());
 }
 
+TEST_F(P2PTransportChannelPingTest, TestDonotSwitchConnectionTooFrequently) {
+  rtc::ScopedFakeClock clock;
+  cricket::FakePortAllocator pa(rtc::Thread::Current(), nullptr);
+  cricket::P2PTransportChannel ch("receiving state change", 1, &pa);
+  PrepareChannel(&ch);
+  ch.SetIceRole(cricket::ICEROLE_CONTROLLED);
+  ch.Connect();
+  ch.MaybeStartGathering();
+  // The connections have decreasing priority.
+  cricket::Connection* conn1 =
+      CreateConnectionWithCandidate(ch, "1.1.1.1", 1, 10, true);
+  ASSERT_TRUE(conn1 != nullptr);
+  cricket::Connection* conn2 =
+      CreateConnectionWithCandidate(ch, "2.2.2.2", 2, 9, true);
+  ASSERT_TRUE(conn2 != nullptr);
+  cricket::Connection* conn3 =
+      CreateConnectionWithCandidate(ch, "3.3.3.3", 3, 8, true);
+  ASSERT_TRUE(conn3 != nullptr);
+  cricket::Connection* conn4 =
+      CreateConnectionWithCandidate(ch, "4.4.4.4", 4, 7, false);
+  ASSERT_TRUE(conn4 != nullptr);
+
+  // Simulate the aggressive nomination on the controlling side.

[pthatcher1, 2016/06/15 21:59:53]

the aggressive nomination => aggressive nomination

[honghaiz3, 2016/06/16 23:03:49]

Done.

+  reset_num_selected_candidate_pair_switch();
+  NominateConnection(conn2);
+  EXPECT_EQ(1, num_selected_candidate_pair_switch());
+  EXPECT_EQ(conn2, last_selected_candidate_pair());
+  reset_num_selected_candidate_pair_switch();

[pthatcher1, 2016/06/15 21:59:53]

Why not have reset return the last value.  Then you can just do this:

EXPECT_EQ(1, reset_selected_candidate_pair_switches());

[honghaiz3, 2016/06/17 19:18:18]

Done.

+
+  // Selected candidate pair will switch because conn1 has higher priority.
+  NominateConnection(conn1);
+  EXPECT_EQ(1, num_selected_candidate_pair_switch());
+  EXPECT_EQ(conn1, last_selected_candidate_pair());
+  reset_num_selected_candidate_pair_switch();
+
+  // Selected candidate pair will not switch because conn2 has lower priority.
+  NominateConnection(conn2);
+  EXPECT_EQ(0, num_selected_candidate_pair_switch());
+  reset_num_selected_candidate_pair_switch();
+
+  // Selected candidate pair will switch if it is nominated again.

[pthatcher1, 2016/06/15 21:59:53]

I don't think we should do this one, because an aggressively nominating
controlling side will nominate a given candidate pair multiple times, and will
still goes frequent switches on the controlled side.  Maybe not as frequent, but
still frequent.

[honghaiz3, 2016/06/16 01:13:08]

If we do not do this, how do we handle the case that some channel may not
receive data? When there are multiple candidate pairs, the chance that a
connection is nominated twice consecutively should be pretty small. And when it
converge, it should converge to the one selected on the controlling side. 

+  NominateConnection(conn2);
+  EXPECT_EQ(1, num_selected_candidate_pair_switch());
+  EXPECT_EQ(conn2, last_selected_candidate_pair());
+  reset_num_selected_candidate_pair_switch();
+
+  // Selected candidate pair won't switch because conn3 has lower priority

[pthatcher1, 2016/06/15 21:59:53]

Can you add "until conn3 receives data"?

[honghaiz3, 2016/06/16 23:03:49]

Done.

+  NominateConnection(conn3);
+  EXPECT_EQ(0, num_selected_candidate_pair_switch());
+  // Will switch if conn3 receives data.
+  conn3->OnReadPacket("XYZ", 3, rtc::CreatePacketTime(0));
+  EXPECT_EQ(1, num_selected_candidate_pair_switch());
+  EXPECT_EQ(conn3, last_selected_candidate_pair());
+  reset_num_selected_candidate_pair_switch();
+
+  // Selected candidate pair won't switch even if it is nominated and
+  // received data because it is not writable.
+  NominateConnection(conn4);
+  conn4->OnReadPacket("XYZ", 3, rtc::CreatePacketTime(0));
+  EXPECT_EQ(0, num_selected_candidate_pair_switch());
+  // It will switch if it becomes writable
+  conn4->ReceivedPingResponse();
+  EXPECT_EQ(1, num_selected_candidate_pair_switch());
+  EXPECT_EQ(conn4, last_selected_candidate_pair());
+  reset_num_selected_candidate_pair_switch();
+
+  // Make sure sorting won't reselect candidate pair.
+  SIMULATED_WAIT(false, 10, clock);
+  EXPECT_EQ(0, num_selected_candidate_pair_switch());
+}
+
 // Test that if a new remote candidate has the same address and port with
 // an old one, it will be used to create a new connection.
 TEST_F(P2PTransportChannelPingTest, TestAddRemoteCandidateWithAddressReuse) {
   cricket::FakePortAllocator pa(rtc::Thread::Current(), nullptr);
   cricket::P2PTransportChannel ch("candidate reuse", 1, &pa);
   PrepareChannel(&ch);
   ch.Connect();
   ch.MaybeStartGathering();
   const std::string host_address = "1.1.1.1";
   const int port_num = 1;
@@ skipping 430 matching lines @@
 
   // TCP Relay/Relay is the next.
   VerifyNextPingableConnection(cricket::RELAY_PORT_TYPE,
                                cricket::RELAY_PORT_TYPE,
                                cricket::TCP_PROTOCOL_NAME);
 
   // Finally, Local/Relay will be pinged.
   VerifyNextPingableConnection(cricket::LOCAL_PORT_TYPE,
                                cricket::RELAY_PORT_TYPE);
 }