Fix for HTTP2 request hanging bug.

net/socket/client_socket_pool_base.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/socket/client_socket_pool_base.h"
 
 #include <algorithm>
 #include <utility>
 
 #include "base/compiler_specific.h"
@@ skipping 884 matching lines @@
     OnAvailableSocketSlot(group_name, group);
   } else {
     socket.reset();
     RecordIdleSocketFate(IDLE_SOCKET_FATE_RELEASE_UNUSABLE);
   }
 
   CheckForStalledSocketGroups();
 }
 
 void ClientSocketPoolBaseHelper::CheckForStalledSocketGroups() {
-  // If we have idle sockets, see if we can give one to the top-stalled group.
-  std::string top_group_name;
-  Group* top_group = NULL;
-  if (!FindTopStalledGroup(&top_group, &top_group_name)) {
-    // There may still be a stalled group in a lower level pool.
-    for (std::set<LowerLayeredPool*>::iterator it = lower_pools_.begin();
-         it != lower_pools_.end();
-         ++it) {
-       if ((*it)->IsStalled()) {
-         CloseOneIdleSocket();
-         break;
-       }
-    }
-    return;
-  }
-
-  if (ReachedMaxSocketsLimit()) {
-    if (idle_socket_count() > 0) {
-      CloseOneIdleSocket();
-    } else {
-      // We can't activate more sockets since we're already at our global
-      // limit.
+  // Loop until there's nothing more to do.
+  while (true) {
+    // If we have idle sockets, see if we can give one to the top-stalled group.
+    std::string top_group_name;
+    Group* top_group = NULL;
+    if (!FindTopStalledGroup(&top_group, &top_group_name)) {
+      // There may still be a stalled group in a lower level pool.
+      for (std::set<LowerLayeredPool*>::iterator it = lower_pools_.begin();
+           it != lower_pools_.end(); ++it) {
+        if ((*it)->IsStalled()) {
+          CloseOneIdleSocket();
+          break;

[davidben, 2017/05/18 20:22:51]

What's causing the socket pools to react to the free socket slot in this code?

[mmenke, 2017/05/18 20:44:57]

Destroying the socket will do that.

So suppose this is the SSL pool, and the client socket pool is full. 
SSLClientSockets own a ClientSocketHandle, which owns a TCPClientSocket.  So
deleting the idle SSLClientSocket will delete the ClientSocketHandle, which will
free up a slot in the lower layer pool.  Which will then call its own
ProcessPendingRequest and CheckForStalledSocketGroups functions.

Hrm...there's another question:  Should we also do a loop for checking for
stalled lower layer pools?  Think it's beyond the scope of this CL, but worth
figuring out.

[davidben, 2017/05/18 20:52:31]

I guess the vague analogy to our sync-return || ERR_IO_PENDING calling
convention would be that all of these functions should return a signal whether
they synchronously did something and expect you to loop or whether they'll
handle all their obligations asynchornously.

[mmenke, 2017/05/18 21:00:43]

Ideally, we really should only have at most one (new) idle socket when there's a
higher layer stalled group, so only one call is needed.  I'm skeptical that's
currently the case, though, so yea, that's one approach.

We could also just look at idle_socket_count(), like we do below, and then do:

if (idle_socket_count() == 0)
  return;
for (std::set<LowerLayeredPool*>::iterator it = lower_pools_.begin();
     it != lower_pools_.end(); ++it) {
  while ((*it)->IsStalled()) {
    CloseOneIdleSocket();
    if (idle_socket_count() == 0)
      return;
  }
}
    

+        }
+      }
       return;
     }
+
+    if (ReachedMaxSocketsLimit()) {
+      if (idle_socket_count() > 0) {
+        CloseOneIdleSocket();
+      } else {
+        // We can't activate more sockets since we're already at our global
+        // limit.
+        return;
+      }
+    }
+
+    // Note that this may delete top_group.
+    OnAvailableSocketSlot(top_group_name, top_group);
   }
-
-  // Note:  we don't loop on waking stalled groups.  If the stalled group is at
-  //        its limit, may be left with other stalled groups that could be
-  //        woken.  This isn't optimal, but there is no starvation, so to avoid
-  //        the looping we leave it at this.

[davidben, 2017/05/18 20:22:51]

Previously we tried to avoid infinite looping here. Should that be a concern? I
guess the assumption is that, since we invoke all user callbacks here on a
PostTask, we can only drain through our existing set of requests and not grow
new ones in processing?

[mmenke, 2017/05/18 20:44:57]

Correct.  I can't see how we'd get into an infinite loop, or have re-entrancy
issues here because of the PostTask.  Also note that we're already potentially
creating two ConnectJobs in a row, since we always call this method after
another OnAvailableSocketSlot call.

I'm also not seeing how we'd end up looping more than once, in general, other
than on sync failure/success.

-  OnAvailableSocketSlot(top_group_name, top_group);
 }
 
 // Search for the highest priority pending request, amongst the groups that
 // are not at the |max_sockets_per_group_| limit. Note: for requests with
 // the same priority, the winner is based on group hash ordering (and not
 // insertion order).
 bool ClientSocketPoolBaseHelper::FindTopStalledGroup(
     Group** group,
     std::string* group_name) const {
   CHECK((group && group_name) || (!group && !group_name));
@@ skipping 127 matching lines @@
 
   // If the group has no idle sockets, and can't make use of an additional slot,
   // either because it's at the limit or because it's at the socket per group
   // limit, then there's nothing to do.
   if (group->idle_sockets().empty() &&
       !group->CanUseAdditionalSocketSlot(max_sockets_per_group_)) {
     return;
   }
 
   int rv = RequestSocketInternal(group_name, *next_request);
   if (rv != ERR_IO_PENDING) {

[mmenke, 2017/05/18 20:00:19]

Could add a loop here, too, but:

1)  group may be deleted if it becomes empty.
2)  Every call to OnAvailableSocketSlot is followed by a call to
CheckForStalledSocketGroups() (Except the call in CheckForStalledSocketGroups
itself).

Since user callbacks aren't invoked synchronously here, 1) isn't a blocker, it
just makes things a bit messy.  But I'm also not sure if there's any benefit to
doing looping here instead/additionally, other than perhaps a marginal
performance benefit, if we don't look through all groups for one in need of
another socket request quite as often.

     std::unique_ptr<Request> request = group->PopNextPendingRequest();
     DCHECK(request);
     if (group->IsEmpty())
       RemoveGroup(group_name);
 
     request->net_log().EndEventWithNetErrorCode(NetLogEventType::SOCKET_POOL,
                                                 rv);
     InvokeUserCallbackLater(request->handle(), request->callback(), rv);
   }
 }
@@ skipping 386 matching lines @@
   // If there are no more requests, kill the backup timer.
   if (pending_requests_.empty())
     backup_job_timer_.Stop();
   request->CrashIfInvalid();
   return request;
 }
 
 }  // namespace internal
 
 }  // namespace net