Signal to remove remote candidates if ports are pruned.

webrtc/p2p/client/basicportallocator_unittest.cc

Index: webrtc/p2p/client/basicportallocator_unittest.cc
diff --git a/webrtc/p2p/client/basicportallocator_unittest.cc b/webrtc/p2p/client/basicportallocator_unittest.cc
index c9831f525b9b14bd1964b42713256da778b71433..d0d17568786ff26abd28076f782f3ba1572e1eb3 100644
--- a/webrtc/p2p/client/basicportallocator_unittest.cc
+++ b/webrtc/p2p/client/basicportallocator_unittest.cc
@@ -243,6 +243,8 @@ class BasicPortAllocatorTest : public testing::Test,
                                        &BasicPortAllocatorTest::OnPortsPruned);
     session->SignalCandidatesReady.connect(
         this, &BasicPortAllocatorTest::OnCandidatesReady);
+    session->SignalCandidatesRemoved.connect(
+        this, &BasicPortAllocatorTest::OnCandidatesRemoved);
     session->SignalCandidatesAllocationDone.connect(
         this, &BasicPortAllocatorTest::OnCandidatesAllocationDone);
     return session;
@@ -404,6 +406,8 @@ class BasicPortAllocatorTest : public testing::Test,
     EXPECT_EQ(total_ports, ports_.size());
   }
 
+  rtc::VirtualSocketServer* virtual_socket_server() { return vss_.get(); }
+
  protected:
   BasicPortAllocator& allocator() { return *allocator_; }
 
@@ -446,6 +450,21 @@ class BasicPortAllocatorTest : public testing::Test,
     }
   }
 
+  void OnCandidatesRemoved(PortAllocatorSession* session,
+                           const std::vector<Candidate>& removed_candidates) {
+    auto new_end = std::remove_if(
+        candidates_.begin(), candidates_.end(),
+        [removed_candidates](Candidate& candidate) {
+          for (const Candidate& removed_candidate : removed_candidates) {
+            if (candidate.MatchesForRemoval(removed_candidate)) {
+              return true;
+            }
+          }
+          return false;
+        });
+    candidates_.erase(new_end, candidates_.end());
+  }
+
   bool HasRelayAddress(const ProtocolAddress& proto_addr) {
     for (size_t i = 0; i < allocator_->turn_servers().size(); ++i) {
       RelayServerConfig server_config = allocator_->turn_servers()[i];
@@ -1214,6 +1233,15 @@ TEST_F(BasicPortAllocatorTest, TestSharedSocketWithoutNatUsingTurn) {
 // Test that if prune_turn_ports is set, TCP TurnPort will not
 // be used if UDP TurnPort is used.
 TEST_F(BasicPortAllocatorTest, TestUdpTurnPortPrunesTcpTurnPorts) {
+  // Using a large standard deviation for the transit delay so that TCP Turn
+  // ports may become ready sometimes before the UDP Turn port does,
+  // and sometimes after that.
+  // TODO(honghaiz): Change the virtual socket to tell if it is UDP or TCP, so
+  // that we can control the TCP delay independently of the UDP delay.

[Taylor Brandstetter, 2016/09/15 00:43:17]

Since IPv4 vs. IPv6 is already handled in this CL, can UDP vs. TCP be handled
too? I think we should avoid non-deterministic tests.

[honghaiz3, 2016/09/16 01:41:30]

Done.

+  virtual_socket_server()->set_delay_mean(50);
+  virtual_socket_server()->set_delay_stddev(200);
+  virtual_socket_server()->UpdateDelayDistribution();
+
   turn_server_.AddInternalSocket(kTurnTcpIntAddr, PROTO_TCP);
   AddInterface(kClientAddr);
   allocator_.reset(new BasicPortAllocator(&network_manager_));
@@ -1227,7 +1255,7 @@ TEST_F(BasicPortAllocatorTest, TestUdpTurnPortPrunesTcpTurnPorts) {
 
   EXPECT_TRUE(CreateSession(ICE_CANDIDATE_COMPONENT_RTP));
   session_->StartGettingPorts();
-  EXPECT_TRUE_WAIT(candidate_allocation_done_, kDefaultAllocationTimeout);
+  EXPECT_TRUE_WAIT(candidate_allocation_done_, 10000);

[Taylor Brandstetter, 2016/09/15 00:43:17]

nit: May want to use a constant for this timeout. Could even just increase the
default timeout to 10 seconds (I've done that in other places; it reduces
msan/tsan/etc flakes).

[honghaiz3, 2016/09/16 01:41:30]

Done. Now that there is no randomness, I don't think it needs 10 seconds. But
still make it larger to be 3 seconds. If we still see flakiness, we can increase
to 10seconds. 

   // Only 2 ports (one STUN and one TURN) are actually being used.
   EXPECT_EQ(2U, session_->ReadyPorts().size());
   // We have verified that each port, when it is added to |ports_|, it is found
@@ -1238,11 +1266,10 @@ TEST_F(BasicPortAllocatorTest, TestUdpTurnPortPrunesTcpTurnPorts) {
   EXPECT_EQ(1, CountPorts(ports_, "relay", PROTO_UDP, kClientAddr));
   EXPECT_EQ(0, CountPorts(ports_, "relay", PROTO_TCP, kClientAddr));
 
-  // We don't remove candidates, so the size of |candidates_| will depend on
-  // when the TCP TURN port becomes ready. If it is ready after the UDP TURN
-  // port becomes ready, its candidates will be used there will be 3 candidates.
-  // Otherwise there will be only 2 candidates.
-  EXPECT_LE(2U, candidates_.size());
+  // Now that we remove candidates when a Turn port is pruned, |candidates_|
+  // should only contains two candidates regardless whether the TCP Turn port is
+  // created before or after the UDP turn port.

[Taylor Brandstetter, 2016/09/15 00:43:17]

nit: Capitalize "TURN"

[honghaiz3, 2016/09/16 01:41:30]

Done.

+  EXPECT_EQ(2U, candidates_.size());
   // There will only be 2 candidates in |ready_candidates| because it only
   // includes the candidates in the ready ports.
   const std::vector<Candidate>& ready_candidates = session_->ReadyCandidates();
@@ -1255,6 +1282,15 @@ TEST_F(BasicPortAllocatorTest, TestUdpTurnPortPrunesTcpTurnPorts) {
 // Tests that if prune_turn_ports is set, IPv4 TurnPort will not
 // be used if IPv6 TurnPort is used.
 TEST_F(BasicPortAllocatorTest, TestIPv6TurnPortPrunesIPv4TurnPorts) {
+  // Make the mean IPv6 delay longer than the mean IPv4 delay so that it is
+  // more likely that the IPv4 candidates are initially generated and then
+  // removed.
+  virtual_socket_server()->set_delay_stddev(100);

[Taylor Brandstetter, 2016/09/15 00:43:17]

Since you can control the delay for IPv6 vs. IPv4 directly, why is variation
needed?

[honghaiz3, 2016/09/16 01:41:30]

I was hoping to use a random value to test behaviors under different realistic
scenarios. 
vlad made a good argument on using a deterministic value and you also want a
deterministic value. So removed it now with the new approach of setting delays

+  virtual_socket_server()->set_delay_mean(100);
+  virtual_socket_server()->UpdateDelayDistribution();
+  virtual_socket_server()->set_delay_mean(200);
+  virtual_socket_server()->UpdateDelayDistributionForIPv6();
+
   turn_server_.AddInternalSocket(kTurnUdpIntIPv6Addr, PROTO_UDP);
   // Add two IP addresses on the same interface.
   AddInterface(kClientAddr, "net1");
@@ -1263,6 +1299,7 @@ TEST_F(BasicPortAllocatorTest, TestIPv6TurnPortPrunesIPv4TurnPorts) {
   allocator_->SetConfiguration(allocator_->stun_servers(),
                                allocator_->turn_servers(), 0, true);
   AddTurnServers(kTurnUdpIntIPv6Addr, rtc::SocketAddress());
+  AddTurnServers(kTurnUdpIntAddr, rtc::SocketAddress());
 
   allocator_->set_step_delay(kMinimumStepDelay);
   allocator_->set_flags(allocator().flags() |
@@ -1271,7 +1308,7 @@ TEST_F(BasicPortAllocatorTest, TestIPv6TurnPortPrunesIPv4TurnPorts) {
 
   EXPECT_TRUE(CreateSession(ICE_CANDIDATE_COMPONENT_RTP));
   session_->StartGettingPorts();
-  EXPECT_TRUE_WAIT(candidate_allocation_done_, kDefaultAllocationTimeout);
+  EXPECT_TRUE_WAIT(candidate_allocation_done_, 10000);
   // Three ports (one IPv4 STUN, one IPv6 STUN and one TURN) will be ready.
   EXPECT_EQ(3U, session_->ReadyPorts().size());
   EXPECT_EQ(3U, ports_.size());
@@ -1280,10 +1317,9 @@ TEST_F(BasicPortAllocatorTest, TestIPv6TurnPortPrunesIPv4TurnPorts) {
   EXPECT_EQ(1, CountPorts(ports_, "relay", PROTO_UDP, kClientIPv6Addr));
   EXPECT_EQ(0, CountPorts(ports_, "relay", PROTO_UDP, kClientAddr));
 
-  // We don't remove candidates, so there may be more than 3 elemenets in
-  // |candidates_|, although |ready_candidates| only includes the candidates
-  // in |ready_ports|.
-  EXPECT_LE(3U, candidates_.size());
+  // Now that we remove candidates when a Turn port is pruned, there will be
+  // exactly 3 candidates in both |candidates_| and |ready_candidates|.
+  EXPECT_EQ(3U, candidates_.size());
   const std::vector<Candidate>& ready_candidates = session_->ReadyCandidates();
   EXPECT_EQ(3U, ready_candidates.size());
   EXPECT_PRED4(HasCandidate, ready_candidates, "local", "udp", kClientAddr);
@@ -1294,6 +1330,14 @@ TEST_F(BasicPortAllocatorTest, TestIPv6TurnPortPrunesIPv4TurnPorts) {
 // Tests that if prune_turn_ports is set, each network interface
 // will has its own set of TurnPorts based on their priorities.
 TEST_F(BasicPortAllocatorTest, TestEachInterfaceHasItsOwnTurnPorts) {
+  // Make IPv6 sockets have longer delay than IPv4 sockets so that it has
+  // higher chance that IPv4 candidates are generated and then removed.
+  virtual_socket_server()->set_delay_stddev(100);
+  virtual_socket_server()->set_delay_mean(100);
+  virtual_socket_server()->UpdateDelayDistribution();
+  virtual_socket_server()->set_delay_mean(200);
+  virtual_socket_server()->UpdateDelayDistributionForIPv6();
+
   turn_server_.AddInternalSocket(kTurnTcpIntAddr, PROTO_TCP);
   turn_server_.AddInternalSocket(kTurnUdpIntIPv6Addr, PROTO_UDP);
   turn_server_.AddInternalSocket(kTurnTcpIntIPv6Addr, PROTO_TCP);
@@ -1315,7 +1359,7 @@ TEST_F(BasicPortAllocatorTest, TestEachInterfaceHasItsOwnTurnPorts) {
                         PORTALLOCATOR_ENABLE_IPV6);
   EXPECT_TRUE(CreateSession(ICE_CANDIDATE_COMPONENT_RTP));
   session_->StartGettingPorts();
-  EXPECT_TRUE_WAIT(candidate_allocation_done_, kDefaultAllocationTimeout);
+  EXPECT_TRUE_WAIT(candidate_allocation_done_, 10000);
   // 10 ports (4 STUN and 1 TURN ports on each interface) will be ready to use.
   EXPECT_EQ(10U, session_->ReadyPorts().size());
   EXPECT_EQ(10U, ports_.size());
@@ -1330,9 +1374,9 @@ TEST_F(BasicPortAllocatorTest, TestEachInterfaceHasItsOwnTurnPorts) {
   EXPECT_EQ(1, CountPorts(ports_, "relay", PROTO_UDP, kClientIPv6Addr));
   EXPECT_EQ(1, CountPorts(ports_, "relay", PROTO_UDP, kClientIPv6Addr2));
 
-  // We don't remove candidates, so there may be more than 10 candidates
-  // in |candidates_|.
-  EXPECT_LE(10U, candidates_.size());
+  // Now that we remove candidates when Turn ports are pruned, there will be
+  // exactly 10 candidates in |candidates_|.
+  EXPECT_EQ(10U, candidates_.size());
   const std::vector<Candidate>& ready_candidates = session_->ReadyCandidates();
   EXPECT_EQ(10U, ready_candidates.size());
   EXPECT_PRED4(HasCandidate, ready_candidates, "local", "udp", kClientAddr);