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 287 matching lines @@
 
 void ClientSocketPoolBaseHelper::RemoveHigherLayeredPool(
     HigherLayeredPool* higher_pool) {
   CHECK(higher_pool);
   CHECK(base::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.
   CleanupIdleSockets(false);
 
   request->net_log().BeginEvent(NetLogEventType::SOCKET_POOL);
   Group* group = GetOrCreateGroup(group_name);
 
   int rv = RequestSocketInternal(group_name, *request);
@@ skipping 236 matching lines @@
 }
 
 // static
 void ClientSocketPoolBaseHelper::LogBoundConnectJobToRequest(
     const NetLogSource& connect_job_source,
     const Request& request) {
   request.net_log().AddEvent(NetLogEventType::SOCKET_POOL_BOUND_TO_CONNECT_JOB,
                              connect_job_source.ToEventParametersCallback());
 }
 
+void ClientSocketPoolBaseHelper::SetPriority(const std::string& group_name,
+                                             ClientSocketHandle* handle,
+                                             RequestPriority priority) {
+  GroupMap::iterator group_it = group_map_.find(group_name);
+  if (group_it == group_map_.end()) {
+    DCHECK(base::ContainsKey(pending_callback_map_, handle));
+    // The Request has already completed and been destroyed; nothing to
+    // reprioritize.
+    return;

[mmenke, 2017/01/03 22:51:22]

Per earlier comment, the ClientSocketHandle knows when this happens.  Should
that class just early abort the call in that case, and DCHECK here?  (The
ClientSocketHandle consumer may not know if that's the case, since we may post a
task to inform it of completion).

[Randy Smith (Not in Mondays), 2017/01/05 03:47:18]

I'm embarrassed to say that when I first read your comment, I thought I knew
what you meant but there was a trickiness to it, but coming back I've lost that
sense completely :-}.  So I'll speak from my current perspective and D'oh! if
needed.

If the handle is still in pending_callback_map_, that means that the callback
hasn't yet occurred, which means that the state in the ClientSocketHandle is
still "socket allocation pending" and it doesn't know not to call into the pool
to set the priority.  The request may have been completed and been destroyed
before the consumer is informed of the request result.  So I think we need this
check.  Am I missing something?

+  }
+
+  group_it->second->SetPriority(handle, priority);
+}
+
 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(base::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(NetLogEventType::CANCELLED);
     request->net_log().EndEvent(NetLogEventType::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()->get(), group);
       CheckForStalledSocketGroups();
@@ skipping 343 matching lines @@
   // accessed.
   NetLogWithSource 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(NetLogEventType::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(NetLogEventType::SOCKET_POOL,
                                                 rv);
     InvokeUserCallbackLater(request->handle(), request->callback(), rv);
   }
 }
 
@@ skipping 66 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 222 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>
+void ClientSocketPoolBaseHelper::Group::SetPriority(ClientSocketHandle* handle,
+                                                    RequestPriority priority) {
+  for (RequestQueue::Pointer pointer = pending_requests_.FirstMax();
+       !pointer.is_null();
+       pointer = pending_requests_.GetNextTowardsLastMin(pointer)) {
+    if (pointer.value()->handle() == handle) {
+      if (pointer.priority() == priority)
+        return;
+
+      std::unique_ptr<Request> request = RemovePendingRequest(pointer);
+
+      // Requests that ignore limits much be created and remain at the highest
+      // priority, and should not be reprioritized.
+      DCHECK_EQ(request->respect_limits(),
+                ClientSocketPool::RespectLimits::ENABLED);
+
+      request->set_priority(priority);
+      InsertPendingRequest(std::move(request));
+      return;
+    }
+  }

[mmenke, 2017/01/03 22:51:22]

This entire block is just:

std::unique_ptr<Request> request = FindAndRemovePendingRequest();
request->set_priority(priority);
InsertPendingRequest(std::move(request));

(With some extra DCHECKs)

As an added bonus, if we ever decide FindAndRemovePendingRequest is too weird
and overhead-y as-is, this will benefit from the change as well (Or vice-versa).

We lose the "pointer.priority() == priority" early exit, but that case should
never happen, per the NOTREACHED below.

[Randy Smith (Not in Mondays), 2017/01/05 03:47:18]

The entire reason for this code is indeed the case where the priority is being
set to the already existing priority, in response to yours and Charles' comments
about not wanting that case to have any effect (and my reluctance to forbid that
at this API level and implicitly rely on the short-circuit in
URLRequest::SetPriority).  If you object to the extra code for a case that won't
happen in the name of API cleanliness, I'll reluctantly fix it up (an API
contract saying "This one specific value that you can't query isn't allowed"
bothers me) , but I don't see what it has to do with the NOTREACHED() below? 
That's for whether or not the socket's in the group's list.

[Randy Smith (Not in Mondays), 2017/01/13 23:05:44]

Ping?

[mmenke, 2017/01/17 18:56:31]

Sorry, I was thinking the early return was a break.

Anyhow, in the name of cleanliness, I'd rather we do one of the following:

1) Just disallow setting the priority to be the same.  If the consumer is
setting the API to be what it was before, they're wasting CPU cycles, and should
be smarter.  Worth noting that URLRequest already short circuits this case, so
we're just talking about stuff outside net/ that directly uses socket pools. 
Which is uncommon (And which are currently second class citizens, and weird, as
they use socket pooling without really wanting pooled sockets, anyways.  So
they're icky).

2) Catch it in ClientSocketHandle.  This does require yet another bloaty field. 
It's not much, alone, but we have so much cruft all over the place, I'm still
reluctant to add more unless we really needed it.

3)  Just move the request to the back of the queue.

[Randy Smith (Not in Mondays), 2017/01/17 19:50:36]

So I'm quite happy to do #3; I changed it to this because of your and Charles'
opposition (see
https://codereview.chromium.org/1898133002/diff/40001/net/socket/client_socke...;
Charles weighed in offline, I believe).  I'd rather avoid #2 for the same reason
as you, and I'd rather avoid #1 because looked at in isolation is strikes me as
an ugly API (the disallowed value is not probeable, so there's implicitly a
tracking requirement on e consumer).   Just confirming (with both you and
Charles): Is #3 ok?  It won't change the behavior for most requests because of
the short-circuit at the URLRequest level, as you note (though there may come a
time when I want that new behavior, but that'll be a different CL).

[Charlie Harrison, 2017/01/17 20:04:39]

I am OK with #3 as long as it is documented clearly.

[mmenke, 2017/01/17 20:11:28]

Could also do a DLOG(WARNING), though if DLOG's are sufficiently discourages in
favor of DVLOG, I don't think it's worth doing.

+
+  // This function must be called with a valid ClientSocketHandle.
+  NOTREACHED();
+}
+
+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