Fix for HTTP2 request hanging bug.

net/socket/client_socket_pool_base.cc

Index: net/socket/client_socket_pool_base.cc
diff --git a/net/socket/client_socket_pool_base.cc b/net/socket/client_socket_pool_base.cc
index 912dfeeaff137b4752f896e91eacf779b791966a..f5978625bd2e0ce2ae805ae55f2ff565b5ef1728 100644
--- a/net/socket/client_socket_pool_base.cc
+++ b/net/socket/client_socket_pool_base.cc
@@ -902,37 +902,36 @@ void ClientSocketPoolBaseHelper::ReleaseSocket(
 }
 
 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;
-       }
+  // 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;
     }
-    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;
+    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:  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);
+    // Note that this may delete top_group.
+    OnAvailableSocketSlot(top_group_name, top_group);
+  }
 }
 
 // Search for the highest priority pending request, amongst the groups that