Add reprioritization to socket pools.

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 <algorithm>
 #include <utility>
 
 #include "base/compiler_specific.h"
@@ skipping 261 matching lines @@
 
 void ClientSocketPoolBaseHelper::RemoveHigherLayeredPool(
     HigherLayeredPool* higher_pool) {
   CHECK(higher_pool);
   CHECK(ContainsKey(higher_pools_, higher_pool));
   higher_pools_.erase(higher_pool);
 }
 
 int ClientSocketPoolBaseHelper::RequestSocket(
     const std::string& group_name,
-    std::unique_ptr<const Request> request) {
+    std::unique_ptr<Request> request) {
   CHECK(!request->callback().is_null());
   CHECK(request->handle());
 
   // Cleanup any timed-out idle sockets if no timer is used.
   if (!use_cleanup_timer_)
     CleanupIdleSockets(false);
 
   request->net_log().BeginEvent(NetLog::TYPE_SOCKET_POOL);
   Group* group = GetOrCreateGroup(group_name);
 
@@ skipping 227 matching lines @@
   return false;
 }
 
 // static
 void ClientSocketPoolBaseHelper::LogBoundConnectJobToRequest(
     const NetLog::Source& connect_job_source, const Request& request) {
   request.net_log().AddEvent(NetLog::TYPE_SOCKET_POOL_BOUND_TO_CONNECT_JOB,
                              connect_job_source.ToEventParametersCallback());
 }
 
+void ClientSocketPoolBaseHelper::SetPriority(const std::string& group_name,
+                                             ClientSocketHandle* handle,
+                                             RequestPriority priority) {
+  PendingCallbackMap::iterator callback_it = pending_callback_map_.find(handle);

[mmenke, 2016/04/22 17:04:14]

This seems a roundabout way of doing things, and while perf does matter, doing
two searches here seems unnecessary (And suppose it could increase binary size,
which matters more).

Can we just use group_map_.find(group_name), and if it's not found, DCHECK that
it's in pending_callback_map_?

[mmenke, 2016/04/22 17:05:19]

does matter -> does not matter

[Randy Smith (Not in Mondays), 2016/05/08 01:36:19]

Sure, that's fine by me.  Done.

I did it this way copying the pattern in CancelRequest; should I extend the
change there too?

[Randy Smith (Not in Mondays), 2016/05/08 01:36:20]

Acknowledged.

+  // The Request has already completed and been destroyed; nothing to
+  // reprioritize.
+  if (callback_it != pending_callback_map_.end())
+    return;
+
+  DCHECK(ContainsKey(group_map_, group_name));
+
+  Group* group = GetOrCreateGroup(group_name);
+  std::unique_ptr<Request> request = group->FindAndRemovePendingRequest(handle);
+  DCHECK(request);
+  request->set_priority(priority);

[mmenke, 2016/04/22 17:04:14]

Can we either DCHECK the priorities are not the same, or do nothing if they are
the same?  Just helps use keep FIFO ordering if we do nothing when they're the
same.

[mmenke, 2016/04/22 17:04:14]

DCHECK(!request->ignore_limits())?  Decreasing the priority of an ignore limits
request isn't really allowed.

[Randy Smith (Not in Mondays), 2016/05/08 01:36:19]

Hmmm.  I'm not certain how to do nothing if they are the same (which would be my
preference); I don't see the Group class as having lookup operators that don't
remove the request.  Am I missing something?   Should I add an interface like
that to Group?

It's probably just my rationalizing avoiding something that would be tricky to
do :-}, but I'm also not sure that I don't want the current semantics.  If we
ever document the behavior of requests within a priority group, we'll be saying
FIFO, with the call to SetPriority for movements between priorities being an
insert (i.e. putting things at the end of the queue).  It seems like not putting
the request in question at the end of the queue if you do a SetPriority to the
same priority would violate that pattern.

[Randy Smith (Not in Mondays), 2016/05/08 01:36:19]

Done.

[mmenke, 2016/05/09 18:41:03]

Looks like you're right.  You could just DCHECK they're not the same, and catch
them at the URLRequest::SetPriority (Looks like we already catch that case
there, actually).
But then again, we're already doing that, per above comment.  I'm fine with
changing behavior in the future, but then we should make sure it affects
everything that cares about priority.

+  group->InsertPendingRequest(std::move(request));
+}
+
 void ClientSocketPoolBaseHelper::CancelRequest(
     const std::string& group_name, ClientSocketHandle* handle) {
   PendingCallbackMap::iterator callback_it = pending_callback_map_.find(handle);
   if (callback_it != pending_callback_map_.end()) {
     int result = callback_it->second.result;
     pending_callback_map_.erase(callback_it);
     std::unique_ptr<StreamSocket> socket = handle->PassSocket();
     if (socket) {
       if (result != OK)
         socket->Disconnect();
       ReleaseSocket(handle->group_name(), std::move(socket), handle->id());
     }
     return;
   }
 
   CHECK(ContainsKey(group_map_, group_name));
 
   Group* group = GetOrCreateGroup(group_name);
 
   // Search pending_requests for matching handle.
-  std::unique_ptr<const Request> request =
-      group->FindAndRemovePendingRequest(handle);
+  std::unique_ptr<Request> request = group->FindAndRemovePendingRequest(handle);
   if (request) {
     request->net_log().AddEvent(NetLog::TYPE_CANCELLED);
     request->net_log().EndEvent(NetLog::TYPE_SOCKET_POOL);
 
     // 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_request_count() &&
         ReachedMaxSocketsLimit()) {
       RemoveConnectJob(*group->jobs().begin(), group);
       CheckForStalledSocketGroups();
@@ skipping 335 matching lines @@
   // accessed.
   BoundNetLog job_log = job->net_log();
   LoadTimingInfo::ConnectTiming connect_timing = job->connect_timing();
 
   // RemoveConnectJob(job, _) must be called by all branches below;
   // otherwise, |job| will be leaked.
 
   if (result == OK) {
     DCHECK(socket.get());
     RemoveConnectJob(job, group);
-    std::unique_ptr<const Request> request = group->PopNextPendingRequest();
+    std::unique_ptr<Request> request = group->PopNextPendingRequest();
     if (request) {
       LogBoundConnectJobToRequest(job_log.source(), *request);
       HandOutSocket(std::move(socket), ClientSocketHandle::UNUSED,
                     connect_timing, request->handle(), base::TimeDelta(), group,
                     request->net_log());
       request->net_log().EndEvent(NetLog::TYPE_SOCKET_POOL);
       InvokeUserCallbackLater(request->handle(), request->callback(), result);
     } else {
       AddIdleSocket(std::move(socket), group);
       OnAvailableSocketSlot(group_name, group);
       CheckForStalledSocketGroups();
     }
   } else {
     // If we got a socket, it must contain error information so pass that
     // up so that the caller can retrieve it.
     bool handed_out_socket = false;
-    std::unique_ptr<const Request> request = group->PopNextPendingRequest();
+    std::unique_ptr<Request> request = group->PopNextPendingRequest();
     if (request) {
       LogBoundConnectJobToRequest(job_log.source(), *request);
       job->GetAdditionalErrorState(request->handle());
       RemoveConnectJob(job, group);
       if (socket.get()) {
         handed_out_socket = true;
         HandOutSocket(std::move(socket), ClientSocketHandle::UNUSED,
                       connect_timing, request->handle(), base::TimeDelta(),
                       group, request->net_log());
       }
@@ skipping 48 matching lines @@
   // 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) {
-    std::unique_ptr<const Request> request = group->PopNextPendingRequest();
+    std::unique_ptr<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);
   }
 }
 
 void ClientSocketPoolBaseHelper::HandOutSocket(
@@ skipping 55 matching lines @@
     }
   }
   DCHECK_EQ(0, connecting_socket_count_);
 }
 
 void ClientSocketPoolBaseHelper::CancelAllRequestsWithError(int error) {
   for (GroupMap::iterator i = group_map_.begin(); i != group_map_.end();) {
     Group* group = i->second;
 
     while (true) {
-      std::unique_ptr<const Request> request = group->PopNextPendingRequest();
+      std::unique_ptr<Request> request = group->PopNextPendingRequest();
       if (!request)
         break;
       InvokeUserCallbackLater(request->handle(), request->callback(), error);
     }
 
     // Delete group if no longer needed.
     if (group->IsEmpty()) {
       // RemoveGroup() will call .erase() which will invalidate the iterator,
       // but i will already have been incremented to a valid iterator before
       // RemoveGroup() is called.
@@ skipping 217 matching lines @@
   for (RequestQueue::Pointer pointer = pending_requests_.FirstMax();
        !pointer.is_null() && i < jobs_.size();
        pointer = pending_requests_.GetNextTowardsLastMin(pointer), ++i) {
     if (pointer.value()->handle() == handle)
       return true;
   }
   return false;
 }
 
 void ClientSocketPoolBaseHelper::Group::InsertPendingRequest(
-    std::unique_ptr<const Request> request) {
+    std::unique_ptr<Request> request) {
   // This value must be cached before we release |request|.
   RequestPriority priority = request->priority();
   if (request->respect_limits() == ClientSocketPool::RespectLimits::DISABLED) {
     // Put requests with RespectLimits::DISABLED (which should have
     // priority == MAXIMUM_PRIORITY) ahead of other requests with
     // MAXIMUM_PRIORITY.
     DCHECK_EQ(priority, MAXIMUM_PRIORITY);
     pending_requests_.InsertAtFront(request.release(), priority);
   } else {
     pending_requests_.Insert(request.release(), priority);
   }
 }
 
-std::unique_ptr<const ClientSocketPoolBaseHelper::Request>
+std::unique_ptr<ClientSocketPoolBaseHelper::Request>
 ClientSocketPoolBaseHelper::Group::PopNextPendingRequest() {
   if (pending_requests_.empty())
-    return std::unique_ptr<const ClientSocketPoolBaseHelper::Request>();
+    return std::unique_ptr<ClientSocketPoolBaseHelper::Request>();
   return RemovePendingRequest(pending_requests_.FirstMax());
 }
 
-std::unique_ptr<const ClientSocketPoolBaseHelper::Request>
+std::unique_ptr<ClientSocketPoolBaseHelper::Request>
 ClientSocketPoolBaseHelper::Group::FindAndRemovePendingRequest(
     ClientSocketHandle* handle) {
   for (RequestQueue::Pointer pointer = pending_requests_.FirstMax();
        !pointer.is_null();
        pointer = pending_requests_.GetNextTowardsLastMin(pointer)) {
     if (pointer.value()->handle() == handle) {
-      std::unique_ptr<const Request> request = RemovePendingRequest(pointer);
+      std::unique_ptr<Request> request = RemovePendingRequest(pointer);
       return request;
     }
   }
-  return std::unique_ptr<const ClientSocketPoolBaseHelper::Request>();
+  return std::unique_ptr<ClientSocketPoolBaseHelper::Request>();
 }
 
-std::unique_ptr<const ClientSocketPoolBaseHelper::Request>
+std::unique_ptr<ClientSocketPoolBaseHelper::Request>
 ClientSocketPoolBaseHelper::Group::RemovePendingRequest(
     const RequestQueue::Pointer& pointer) {
   // TODO(eroman): Temporary for debugging http://crbug.com/467797.
   CHECK(!pointer.is_null());
-  std::unique_ptr<const Request> request(pointer.value());
+  std::unique_ptr<Request> request(pointer.value());
   pending_requests_.Erase(pointer);
   // If there are no more requests, kill the backup timer.
   if (pending_requests_.empty())
     backup_job_timer_.Stop();
   request->CrashIfInvalid();
   return request;
 }
 
 }  // namespace internal
 
 }  // namespace net