Stop a session when a new connection becomes writable.

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 195 matching lines @@
                                          PortAllocator* allocator)
     : TransportChannelImpl(transport_name, component),
       transport_(transport),
       allocator_(allocator),
       worker_thread_(rtc::Thread::Current()),
       incoming_only_(false),
       error_(0),
       best_connection_(NULL),
       pending_best_connection_(NULL),
       sort_dirty_(false),
-      was_writable_(false),
       remote_ice_mode_(ICEMODE_FULL),
       ice_role_(ICEROLE_UNKNOWN),
       tiebreaker_(0),
       remote_candidate_generation_(0),
       gathering_state_(kIceGatheringNew),
       check_receiving_delay_(MIN_CHECK_RECEIVING_DELAY * 5),
       receiving_timeout_(MIN_CHECK_RECEIVING_DELAY * 50) {}
 
 P2PTransportChannel::~P2PTransportChannel() {
   ASSERT(worker_thread_ == rtc::Thread::Current());
 
   for (size_t i = 0; i < allocator_sessions_.size(); ++i)
     delete allocator_sessions_[i];
 }
 
 // Add the allocator session to our list so that we know which sessions
 // are still active.
 void P2PTransportChannel::AddAllocatorSession(PortAllocatorSession* session) {
+  ASSERT(worker_thread_ == rtc::Thread::Current());
+
   session->set_generation(static_cast<uint32_t>(allocator_sessions_.size()));
   allocator_sessions_.push_back(session);
 
   // We now only want to apply new candidates that we receive to the ports
   // created by this new session because these are replacing those of the
   // previous sessions.
   ports_.clear();
 
   session->SignalPortReady.connect(this, &P2PTransportChannel::OnPortReady);
   session->SignalCandidatesReady.connect(
       this, &P2PTransportChannel::OnCandidatesReady);
   session->SignalCandidatesAllocationDone.connect(
       this, &P2PTransportChannel::OnCandidatesAllocationDone);
   session->StartGettingPorts();
+  is_getting_ports_ = true;
 }
 
 void P2PTransportChannel::AddConnection(Connection* connection) {
   connections_.push_back(connection);
   connection->set_remote_ice_mode(remote_ice_mode_);
   connection->set_receiving_timeout(receiving_timeout_);
   connection->SignalReadPacket.connect(
       this, &P2PTransportChannel::OnReadPacket);
   connection->SignalReadyToSend.connect(
       this, &P2PTransportChannel::OnReadyToSend);
@@ skipping 803 matching lines @@
   bool receiving = false;
   for (const Connection* connection : connections_) {
     if (connection->receiving()) {
       receiving = true;
       break;
     }
   }
   set_receiving(receiving);
 }
 
-// We checked the status of our connections and we had at least one that
-// was writable, go into the writable state.
-void P2PTransportChannel::HandleWritable() {
-  ASSERT(worker_thread_ == rtc::Thread::Current());
-  if (writable()) {
+void P2PTransportChannel::MaybeStopSessions() {

[pthatcher1, 2015/10/24 00:39:04]

Should be MaybeStopPortAllocatorSessions

[honghaiz3, 2015/10/26 16:47:46]

Done.

+  if (!is_getting_ports_) {
     return;
   }
+  is_getting_ports_ = false;

[pthatcher1, 2015/10/24 00:39:04]

How is is_getting_ports_ going to be different than session->IsGettingPorts()?

If it's the same, then this method doesn't do anything because it checks if
!session->IsGettingPorts().  So we wouldn't need is_getting_ports_.

[honghaiz3, 2015/10/26 16:47:46]

It is pretty much the same except for the last session which is not stopped but
cleared. 
For correctness, is_geeting_ports_ is not needed. 
But it stopped the rest from being called multiple times (every time a
connection becomes strong). 
Its effect is similar to the check of (writable()) in the original code. 
If it is removed, it will call ClearGettingPorts on the last session every time
(if continual gathering is enabled), although it does not do much except for
wasting some computing cycles for the current code.

[pthatcher1, 2015/10/27 07:24:04]

OK, could we make this a little cleaner by doing something like this?

bool P2pTransportChannel::IsGettingPorts() {
  for (PortAllocatorSession* session : allocator_sessions_) {
    if (session->IsGettingPorts()) {
      return true;
    }
  }
}

void P2PTransportChannel::MaybeStopSessions() {
  if (!IsGettingPorts()) {
    return;
  }
  ...
}

[honghaiz3, 2015/10/27 16:59:42]

The problem is that whenever Continual Gathering is enabled, IsGettingPorts will
always return true on the last session.
So we will keep on running StopSession() for every session whenever a new
connection becomes writable.  The purpose of the variable is to remember whether
we have already attempted to stop sessions. Changed the variable name and
hopefully make it less confusing. 

 
   for (PortAllocatorSession* session : allocator_sessions_) {
     if (!session->IsGettingPorts()) {
       continue;
     }
     // If gathering continually, keep the last session running so that it
     // will gather candidates if the networks change.
     if (gather_continually_ && session == allocator_sessions_.back()) {
       session->ClearGettingPorts();
       break;
     }
     session->StopGettingPorts();
   }
+}
 
-  was_writable_ = true;
-  set_writable(true);
+// Go into the writable state and notify upper layer if it was not before.
+void P2PTransportChannel::HandleWritable() {
+  ASSERT(worker_thread_ == rtc::Thread::Current());
+  if (!writable()) {
+    set_writable(true);
+  }
 }
 
 // Notify upper layer about channel not writable state, if it was before.
 void P2PTransportChannel::HandleNotWritable() {
   ASSERT(worker_thread_ == rtc::Thread::Current());
-  if (was_writable_) {
-    was_writable_ = false;
+  if (writable()) {
     set_writable(false);
   }
 }
 
 void P2PTransportChannel::HandleAllTimedOut() {
   // Currently we are treating this as channel not writable.
   HandleNotWritable();
 }
 
 bool P2PTransportChannel::weak() const {
@@ skipping 161 matching lines @@
   // connection and role is controlled.
   if (ice_role_ == ICEROLE_CONTROLLED) {
     if (connection == pending_best_connection_ && connection->writable()) {
       pending_best_connection_ = NULL;
       LOG(LS_INFO) << "Switching best connection on controlled side"
                    << " because it's now writable: " << connection->ToString();
       SwitchBestConnectionTo(connection);
     }
   }
 
+  // May stop the session after at least one connection becomes strong
+  // after starting to get ports.
+  if (!connection->weak()) {

[pthatcher1, 2015/10/24 00:39:04]

Can you document more clearly why connection->writable() isn't enough?

[honghaiz3, 2015/10/26 16:47:46]

Done.

+    MaybeStopSessions();
+  }
+
   // We have to unroll the stack before doing this because we may be changing
   // the state of connections while sorting.
   RequestSort();
 }
 
 // When a connection is removed, edit it out, and then update our best
 // connection.
 void P2PTransportChannel::OnConnectionDestroyed(Connection* connection) {
   ASSERT(worker_thread_ == rtc::Thread::Current());
 
@@ skipping 71 matching lines @@
   SignalSentPacket(this, sent_packet);
 }
 
 void P2PTransportChannel::OnReadyToSend(Connection* connection) {
   if (connection == best_connection_ && writable()) {
     SignalReadyToSend(this);
   }
 }
 
 }  // namespace cricket