Fixing inconsistency with behavior of `ClearGettingPorts`.

webrtc/p2p/base/p2ptransportchannel.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 291 matching lines @@
   // Updating the remote ICE candidate generation could change the sort order.
   RequestSortAndStateUpdate();
 }
 
 void P2PTransportChannel::SetRemoteIceMode(IceMode mode) {
   remote_ice_mode_ = mode;
 }
 
 void P2PTransportChannel::SetIceConfig(const IceConfig& config) {
   if (config_.continual_gathering_policy != config.continual_gathering_policy) {
-    LOG(LS_INFO) << "Set continual_gathering_policy to "
-                 << config_.continual_gathering_policy;
-    config_.continual_gathering_policy = config.continual_gathering_policy;
+    if (!allocator_sessions_.empty()) {

[Taylor Brandstetter, 2016/07/07 17:52:37]

This class assumes the continual gathering policy doesn't change during the
course of the session. So we should enforce that assumption.

[honghaiz3, 2016/07/21 04:10:59]

If you set continual gathering policy after a session starts, it will have
effect on the next session.
It will also have effect on the current session unless the current session has
been stopped or cleared.

So I am not sure if we should enforce the assumption. 

[Taylor_Brandstetter, 2016/07/21 16:41:01]

This is the problem in my opinion. From an application's perspective it appears
like a race condition; depending on when the policy is changed, it may or may
not have an effect.

Is there is a compelling use case for changing the gathering policy mid-session,
we can allow it. But the behavior would need to be documented and covered
sufficiently by unit tests.

[honghaiz3, 2016/07/21 17:37:25]

In my opinion, the race condition is still there. You only changed it from
"depending on whether the current session is cleared" to "depending on whether
there is a session". 

We don't expect users to change it after session starts, it is just that I do
not see the benefit of enforcing that assumption. 

It may happen that the user created the transport channel and set the config
right away. If there is a pooled session, then the user may fail to set it. 
We can still print out the error message but don't have to stop the user from
setting it.  

If you really want to enforce it, do so only if the current session is cleared. 

[Taylor Brandstetter, 2016/07/21 18:38:38]

Sessions, including pooled sessions, are only added in MaybeStartGathering,
which is triggered by SetLocalDescription. So this condition should be
equivalent to "don't allow the policy to change after SetLocalDescription is
called". So I believe the behavior should be predictable.

Also: the benefit of enforcing the assumption is that it prevents us from
accidentally using a code path in the future that isn't covered by unit tests or
intentionally supported.

[honghaiz3, 2016/07/21 21:22:05]

Can you please make a comment on the parameter continual_gathering_policy about
the assumption that it must be set before SetLocalDescription and cannot be
changed afterwards?

[pthatcher1, 2016/07/22 18:11:22]

While I agree consistent behavior is good, it would be surprising to someone
that call SetConfiguration with continual gathering to not get continual
gathering and then get no error until they check the native log.  If we're going
to make the rule "you can't change the continual gathering setting", then we
should cause SetConfiguration to fail, if that's even possible.

Better would be to cause the current sessions to go into continual gathering
mode.  How hard would that be?

[Taylor Brandstetter, 2016/07/22 19:58:42]

JSEP already says that SetConfiguration only changes the ICE servers and
filtering policy. Other changes are ignored, or in the case of changing
certificates, an InvalidModificationError is thrown. I assumed we'd do one of
those 2 things for our custom ICE options as well.

As for "How hard would that be?": Like I said earlier, we'd need to document the
various caveats and add unit tests for changing the policy at different points
in time. This isn't trivial and I don't think it's worth the maintenance effort
unless we see a compelling use case for changing the policy post-PC-creation.

[pthatcher1, 2016/08/23 00:55:39]

That's a good point about SetConfiguration only allowing ICE servers.  You've
convinced me.

+      LOG(LS_ERROR) << "Trying to change continual gathering policy "
+                    << "when gathering has already started!";
+    } else {
+      LOG(LS_INFO) << "Set continual_gathering_policy to "
+                   << config_.continual_gathering_policy;
+      config_.continual_gathering_policy = config.continual_gathering_policy;
+    }
   }
 
   if (config.backup_connection_ping_interval >= 0 &&
       config_.backup_connection_ping_interval !=
           config.backup_connection_ping_interval) {
     config_.backup_connection_ping_interval =
         config.backup_connection_ping_interval;
     LOG(LS_INFO) << "Set backup connection ping interval to "
                  << config_.backup_connection_ping_interval << " milliseconds.";
   }
@@ skipping 138 matching lines @@
     const std::vector<Candidate>& candidates) {
   ASSERT(worker_thread_ == rtc::Thread::Current());
   for (size_t i = 0; i < candidates.size(); ++i) {
     SignalCandidateGathered(this, candidates[i]);
   }
 }
 
 void P2PTransportChannel::OnCandidatesAllocationDone(
     PortAllocatorSession* session) {
   ASSERT(worker_thread_ == rtc::Thread::Current());
+  if (config_.gather_continually()) {
+    LOG(LS_INFO) << "P2PTransportChannel: " << transport_name()
+                 << ", component " << component()
+                 << " gathering complete, but using continual "
+                 << "gathering so not changing gathering state.";
+    return;
+  }
   gathering_state_ = kIceGatheringComplete;
   LOG(LS_INFO) << "P2PTransportChannel: " << transport_name() << ", component "
                << component() << " gathering complete";
   SignalGatheringState(this);
 }
 
 // Handle stun packets
 void P2PTransportChannel::OnUnknownAddress(
     PortInterface* port,
     const rtc::SocketAddress& address, ProtocolType proto,
@@ skipping 1374 matching lines @@
 
   // During the initial state when nothing has been pinged yet, return the first
   // one in the ordered |connections_|.
   return *(std::find_if(connections_.begin(), connections_.end(),
                         [conn1, conn2](Connection* conn) {
                           return conn == conn1 || conn == conn2;
                         }));
 }
 
 }  // namespace cricket