Fix bug that would create unnecessary ConnectJobs in some cases.

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 "base/compiler_specific.h"
 #include "base/format_macros.h"
 #include "base/logging.h"
 #include "base/message_loop/message_loop.h"
@@ skipping 208 matching lines @@
     return false;
   // So in order to be stalled, |this| must be using at least |max_sockets_| AND
   // |this| must have a request that is actually stalled on the global socket
   // limit.  To find such a request, look for a group that has more requests
   // than jobs AND where the number of sockets is less than
   // |max_sockets_per_group_|.  (If the number of sockets is equal to
   // |max_sockets_per_group_|, then the request is stalled on the group limit,
   // which does not count.)
   for (GroupMap::const_iterator it = group_map_.begin();
        it != group_map_.end(); ++it) {
-    if (it->second->IsStalledOnPoolMaxSockets(max_sockets_per_group_))
+    if (it->second->CanUseAdditionalSocketSlot(max_sockets_per_group_))
       return true;
   }
   return false;
 }
 
 void ClientSocketPoolBaseHelper::AddLowerLayeredPool(
     LowerLayeredPool* lower_pool) {
   DCHECK(pool_);
   CHECK(!ContainsKey(lower_pools_, lower_pool));
   lower_pools_.insert(lower_pool);
@@ skipping 31 matching lines @@
   if (rv != ERR_IO_PENDING) {
     request->net_log().EndEventWithNetErrorCode(NetLog::TYPE_SOCKET_POOL, rv);
     CHECK(!request->handle()->is_initialized());
     request.reset();
   } else {
     group->InsertPendingRequest(request.Pass());
     // Have to do this asynchronously, as closing sockets in higher level pools
     // call back in to |this|, which will cause all sorts of fun and exciting
     // re-entrancy issues if the socket pool is doing something else at the
     // time.
-    if (group->IsStalledOnPoolMaxSockets(max_sockets_per_group_)) {
+    if (group->CanUseAdditionalSocketSlot(max_sockets_per_group_)) {
       base::MessageLoop::current()->PostTask(
           FROM_HERE,
           base::Bind(
               &ClientSocketPoolBaseHelper::TryToCloseSocketsInLayeredPools,
               weak_factory_.GetWeakPtr()));
     }
   }
   return rv;
 }
 
@@ skipping 279 matching lines @@
   if (group.HasConnectJobForHandle(handle)) {
     // Just return the state of the farthest along ConnectJob for the first
     // group.jobs().size() pending requests.
     LoadState max_state = LOAD_STATE_IDLE;
     for (const auto& job : group.jobs()) {
       max_state = std::max(max_state, job->GetLoadState());
     }
     return max_state;
   }
 
-  if (group.IsStalledOnPoolMaxSockets(max_sockets_per_group_))
+  if (group.CanUseAdditionalSocketSlot(max_sockets_per_group_))
     return LOAD_STATE_WAITING_FOR_STALLED_SOCKET_POOL;
   return LOAD_STATE_WAITING_FOR_AVAILABLE_SOCKET;
 }
 
 base::DictionaryValue* ClientSocketPoolBaseHelper::GetInfoAsValue(
     const std::string& name, const std::string& type) const {
   base::DictionaryValue* dict = new base::DictionaryValue();
   dict->SetString("name", name);
   dict->SetString("type", type);
   dict->SetInteger("handed_out_socket_count", handed_out_socket_count_);
@@ skipping 33 matching lines @@
     group_dict->Set("idle_sockets", idle_socket_list);
 
     base::ListValue* connect_jobs_list = new base::ListValue();
     std::set<ConnectJob*>::const_iterator job = group->jobs().begin();
     for (job = group->jobs().begin(); job != group->jobs().end(); job++) {
       int source_id = (*job)->net_log().source().id;
       connect_jobs_list->Append(new base::FundamentalValue(source_id));
     }
     group_dict->Set("connect_jobs", connect_jobs_list);
 
-    group_dict->SetBoolean("is_stalled",
-                           group->IsStalledOnPoolMaxSockets(
-                               max_sockets_per_group_));
+    group_dict->SetBoolean("is_stalled", group->CanUseAdditionalSocketSlot(
+                                             max_sockets_per_group_));
     group_dict->SetBoolean("backup_job_timer_is_running",
                            group->BackupJobTimerIsRunning());
 
     all_groups_dict->SetWithoutPathExpansion(it->first, group_dict);
   }
   dict->Set("groups", all_groups_dict);
   return dict;
 }
 
 bool ClientSocketPoolBaseHelper::IdleSocket::IsUsable() const {
@@ skipping 181 matching lines @@
     std::string* group_name) const {
   CHECK((group && group_name) || (!group && !group_name));
   Group* top_group = NULL;
   const std::string* top_group_name = NULL;
   bool has_stalled_group = false;
   for (GroupMap::const_iterator i = group_map_.begin();
        i != group_map_.end(); ++i) {
     Group* curr_group = i->second;
     if (!curr_group->has_pending_requests())
       continue;
-    if (curr_group->IsStalledOnPoolMaxSockets(max_sockets_per_group_)) {
+    if (curr_group->CanUseAdditionalSocketSlot(max_sockets_per_group_)) {
       if (!group)
         return true;
       has_stalled_group = true;
       bool has_higher_priority = !top_group ||
           curr_group->TopPendingPriority() > top_group->TopPendingPriority();
       if (has_higher_priority) {
         top_group = curr_group;
         top_group_name = &i->first;
       }
     }
@@ skipping 63 matching lines @@
                       connect_timing, request->handle(), base::TimeDelta(),
                       group, request->net_log());
       }
       request->net_log().EndEventWithNetErrorCode(
           NetLog::TYPE_SOCKET_POOL, result);
       InvokeUserCallbackLater(request->handle(), request->callback(), result);
     } else {
       RemoveConnectJob(job, group);
     }
     if (!handed_out_socket) {
       OnAvailableSocketSlot(group_name, group);

[mmenke, 2015/03/04 22:48:43]

The issue is this was resulting in always making a new connection, as long as
there was a pending request, even if there were as many or more ConnectJobs
already.

Could actually get rid of this call, and just delete the group if it were empty.
 That means we'd look for the highest priority group, which would probably be
better...  But that seems a bit far afield from just fixing this bug.

       CheckForStalledSocketGroups();
     }
   }
 }
 
 void ClientSocketPoolBaseHelper::OnIPAddressChanged() {
   FlushWithError(ERR_NETWORK_CHANGED);
 }
 
 void ClientSocketPoolBaseHelper::FlushWithError(int error) {
@@ skipping 19 matching lines @@
     RemoveGroup(group_name);
   } else if (group->has_pending_requests()) {
     ProcessPendingRequest(group_name, group);
   }
 }
 
 void ClientSocketPoolBaseHelper::ProcessPendingRequest(
     const std::string& group_name, Group* group) {
   const Request* next_request = group->GetNextPendingRequest();
   DCHECK(next_request);
+
+  // 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) {
     scoped_ptr<const Request> request = group->PopNextPendingRequest();
     DCHECK(request);
     if (group->IsEmpty())
       RemoveGroup(group_name);
 
     request->net_log().EndEventWithNetErrorCode(NetLog::TYPE_SOCKET_POOL, rv);
     InvokeUserCallbackLater(request->handle(), request->callback(), rv);
   }
@@ skipping 349 matching lines @@
   pending_requests_.Erase(pointer);
   // If there are no more requests, kill the backup timer.
   if (pending_requests_.empty())
     backup_job_timer_.Stop();
   return request.Pass();
 }
 
 }  // namespace internal
 
 }  // namespace net