net: Socket pools prioritize requests with ignore_limits

net/socket/client_socket_pool_base.cc

Index: net/socket/client_socket_pool_base.cc
===================================================================
--- net/socket/client_socket_pool_base.cc	(revision 203448)
+++ net/socket/client_socket_pool_base.cc	(working copy)
@@ -187,16 +187,26 @@
 
 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);
 }
 
@@ -494,8 +504,10 @@
       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();
       }