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"
 #include "base/metrics/stats_counters.h"
 #include "base/stl_util.h"
 #include "base/string_util.h"
 #include "base/time.h"
 #include "base/values.h"
 #include "net/base/net_log.h"
 #include "net/base/net_errors.h"
 #include "net/socket/client_socket_handle.h"
 
 using base::TimeDelta;
 
+namespace net {
+
 namespace {
 
 // Indicate whether we should enable idle socket cleanup timer. When timer is
 // disabled, sockets are closed next time a socket request is made.
 bool g_cleanup_timer_enabled = true;
 
 // The timeout value, in seconds, used to clean up idle sockets that can't be
 // reused.
 //
 // Note: It's important to close idle sockets that have received data as soon
 // as possible because the received data may cause BSOD on Windows XP under
 // some conditions.  See http://crbug.com/4606.
 const int kCleanupInterval = 10;  // DO NOT INCREASE THIS TIMEOUT.
 
 // Indicate whether or not we should establish a new transport layer connection
 // after a certain timeout has passed without receiving an ACK.
 bool g_connect_backup_jobs_enabled = true;
 
+// Compares the effective priority of two results, and returns 1 if |request1|
+// has greater effective priority than |request2|, 0 if they have the same
+// effective priority, and -1 if |request2| has the greater effective priority.
+// Requests with |ignore_limits| set have higher effective pririoty than those

[eroman, 2013/06/10 17:48:00]

typo priority

[mmenke, 2013/06/10 18:17:26]

Done.

+// without.  If both requests have |ignore_limits| set/unset, then the request
+// with the highest Pririoty has the highest effective priority.  Does not take
+// into account the fact that Requests are serviced in FIFO order if they would
+// otherwise have the same priority.
+int CompareEffectiveRequestPriority(
+    const internal::ClientSocketPoolBaseHelper::Request& request1,
+    const internal::ClientSocketPoolBaseHelper::Request& request2) {
+  if (request1.ignore_limits() && !request2.ignore_limits())
+    return 1;
+  if (!request1.ignore_limits() && request2.ignore_limits())
+    return -1;
+  if (request1.priority() > request2.priority())
+    return 1;
+  if (request1.priority() < request2.priority())
+    return -1;
+  return 0;
+}
+
 }  // namespace
 
-namespace net {
-
 ConnectJob::ConnectJob(const std::string& group_name,
                        base::TimeDelta timeout_duration,
                        Delegate* delegate,
                        const BoundNetLog& net_log)
     : group_name_(group_name),
       timeout_duration_(timeout_duration),
       delegate_(delegate),
       net_log_(net_log),
       idle_(true) {
   DCHECK(!group_name.empty());
@@ skipping 126 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())
+  // TODO(mmenke):  Should the network stack require requests with
+  //                |ignore_limits| have the highest priority?
+  while (it != pending_requests->end() &&
+         CompareEffectiveRequestPriority(*r, *(*it)) <= 0) {

[eroman, 2013/06/10 17:48:00]

Given that the result is only compared <=0, would it be sufficient to define
"CompareEffectiveRequestPriority" as a 'less than' comparator instead?

[mmenke, 2013/06/10 18:17:26]

I could, but to maintain FIFO order, I'd have to either make a less-or-equal
comparator, or I'd have to negate it.  Or I suppose I could walk through the
list backwards.

Negating a comparator seems ugly to me.  Making it less-or-equal seems a bit
confusing.  Walking backwards also doesn't seem as clear as this is.  Thoughts?

[eroman, 2013/06/10 18:31:49]

I guess you could do something like
  !LessThan(b,a)

(both negating, and also flipping the order of |a| and |b|).

However I agree that is harder to read. As is is fine. I would also be in favor
of having a boolean comparator that returns either "greater than" or "less than
equal to" for simplicity.

I haven't thought this through, but <algorithm>'s std::upper_bound() might also
be applicable. (Although probably even harder to read).

LGTM whichever option you choose.

     ++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