Stop a session when a new connection becomes writable.

webrtc/p2p/base/p2ptransportchannel.cc

Index: webrtc/p2p/base/p2ptransportchannel.cc
diff --git a/webrtc/p2p/base/p2ptransportchannel.cc b/webrtc/p2p/base/p2ptransportchannel.cc
index 9d598f57f3d7902d7c12ac55f8eee22bbda80edc..2d7c6d6c256311babfeab596cfb033da79dbec3c 100644
--- a/webrtc/p2p/base/p2ptransportchannel.cc
+++ b/webrtc/p2p/base/p2ptransportchannel.cc
@@ -213,7 +213,6 @@ P2PTransportChannel::P2PTransportChannel(const std::string& transport_name,
       best_connection_(NULL),
       pending_best_connection_(NULL),
       sort_dirty_(false),
-      was_writable_(false),
       remote_ice_mode_(ICEMODE_FULL),
       ice_role_(ICEROLE_UNKNOWN),
       tiebreaker_(0),
@@ -232,6 +231,8 @@ P2PTransportChannel::~P2PTransportChannel() {
 // 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);
 
@@ -246,6 +247,7 @@ void P2PTransportChannel::AddAllocatorSession(PortAllocatorSession* session) {
   session->SignalCandidatesAllocationDone.connect(
       this, &P2PTransportChannel::OnCandidatesAllocationDone);
   session->StartGettingPorts();
+  is_getting_ports_ = true;
 }
 
 void P2PTransportChannel::AddConnection(Connection* connection) {
@@ -1069,13 +1071,11 @@ void P2PTransportChannel::UpdateChannelState() {
   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()) {
@@ -1089,16 +1089,20 @@ void P2PTransportChannel::HandleWritable() {
     }
     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);
   }
 }
@@ -1280,6 +1284,12 @@ void P2PTransportChannel::OnConnectionStateChange(Connection* 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();