net: Socket pools prioritize requests with ignore_limits

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.h"
@@ skipping 169 matching lines @@
 }
 
 ClientSocketPoolBaseHelper::CallbackResultPair::CallbackResultPair(
     const CompletionCallback& callback_in, int result_in)
     : callback(callback_in),
       result(result_in) {
 }
 
 ClientSocketPoolBaseHelper::CallbackResultPair::~CallbackResultPair() {}
 
-// InsertRequestIntoQueue inserts the request into the queue based on
-// priority.  Highest priorities are closest to the front.  Older requests are
-// prioritized over requests of equal priority.
-//
 // static
 void ClientSocketPoolBaseHelper::InsertRequestIntoQueue(
     const Request* r, RequestQueue* pending_requests) {
   RequestQueue::iterator it = pending_requests->begin();
-  while (it != pending_requests->end() && r->priority() <= (*it)->priority())
+
+  // If not ignoring the socket pool limits, skip over all requests that do.

[eroman, 2013/06/04 20:42:21]

rather than splitting things up into 2 loops, I suggest combining them into one.
perhaps like this:

================
while (it != pending_requests->end() &&
       r->ignore_limits() <= (*it)->ignore_limits() &&
       r->priority() <= (*it)->priority()) {
  ++it;
}
pending_requests->insert(it, r);
================

...or for better clarity, have a function which compares 2 requests's "effective
priority".

[mmenke, 2013/06/05 04:32:24]

Done, went with the function approach and a 3-value comparison function (Could
use two value, but !RequestHigherEffectivePriority is a little weird).  Think
the bool inequality of the other approach is a little harder to follow, though
my check wasn't much better.

+  if (!r->ignore_limits()) {
+    while (it != pending_requests->end() && (*it)->ignore_limits()) {
+      ++it;
+    }
+  }
+
+  // Skip over all requests with priority <= the new request, with the same
+  // value of |ignore_limits|.
+  // TODO(mmenke):  Should the network stack require requests with
+  //                |ignore_limits| set have the highest priority?

[eroman, 2013/06/04 20:42:21]

nit: wording. i think "have the" --> "to have the"]

[mmenke, 2013/06/05 04:32:24]

It's actually correct:  "(requests with |ignore_limits| set) have the highest
priority".  I've removed the word "set", and it still has the same meaning.

+  while (it != pending_requests->end() && r->priority() <= (*it)->priority() &&
+         r->ignore_limits() == (*it)->ignore_limits()) {
     ++it;
+  }
   pending_requests->insert(it, r);
 }
 
 // static
 const ClientSocketPoolBaseHelper::Request*
 ClientSocketPoolBaseHelper::RemoveRequestFromQueue(
     const RequestQueue::iterator& it, Group* group) {
   const Request* req = *it;
   group->mutable_pending_requests()->erase(it);
   // If there are no more requests, we kill the backup timer.
@@ skipping 277 matching lines @@
   Group* group = GetOrCreateGroup(group_name);
 
   // Search pending_requests for matching handle.
   RequestQueue::iterator it = group->mutable_pending_requests()->begin();
   for (; it != group->pending_requests().end(); ++it) {
     if ((*it)->handle() == handle) {
       scoped_ptr<const Request> req(RemoveRequestFromQueue(it, group));
       req->net_log().AddEvent(NetLog::TYPE_CANCELLED);
       req->net_log().EndEvent(NetLog::TYPE_SOCKET_POOL);
 
-      // We let the job run, unless we're at the socket limit.
-      if (group->jobs().size() && ReachedMaxSocketsLimit()) {
+      // We let the job run, unless we're at the socket limit and there is
+      // not another request waiting on the job.
+      if (group->jobs().size() > group->pending_requests().size() &&
+          ReachedMaxSocketsLimit()) {
         RemoveConnectJob(*group->jobs().begin(), group);
         CheckForStalledSocketGroups();
       }
       break;
     }
   }
 }
 
 bool ClientSocketPoolBaseHelper::HasGroup(const std::string& group_name) const {
   return ContainsKey(group_map_, group_name);
@@ skipping 709 matching lines @@
   STLDeleteElements(&jobs_);
   unassigned_job_count_ = 0;
 
   // Cancel pending backup job.
   weak_factory_.InvalidateWeakPtrs();
 }
 
 }  // namespace internal
 
 }  // namespace net