Add reprioritization to socket pools.

net/socket/client_socket_pool_base.cc

Index: net/socket/client_socket_pool_base.cc
diff --git a/net/socket/client_socket_pool_base.cc b/net/socket/client_socket_pool_base.cc
index 252ca294d4c76536297089003726cca5dac043d1..530fb14b4ac885c232869674897947c9c8ca0fa2 100644
--- a/net/socket/client_socket_pool_base.cc
+++ b/net/socket/client_socket_pool_base.cc
@@ -305,7 +305,7 @@ void ClientSocketPoolBaseHelper::RemoveHigherLayeredPool(
 
 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());
 
@@ -562,6 +562,20 @@ void ClientSocketPoolBaseHelper::LogBoundConnectJobToRequest(
                              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);
@@ -582,8 +596,7 @@ void ClientSocketPoolBaseHelper::CancelRequest(
   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);
@@ -947,7 +960,7 @@ void ClientSocketPoolBaseHelper::OnConnectJobComplete(
   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,
@@ -964,7 +977,7 @@ void ClientSocketPoolBaseHelper::OnConnectJobComplete(
     // 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());
@@ -1033,7 +1046,7 @@ void ClientSocketPoolBaseHelper::ProcessPendingRequest(
 
   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);
@@ -1120,7 +1133,7 @@ void ClientSocketPoolBaseHelper::CancelAllRequestsWithError(int error) {
     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);
@@ -1363,7 +1376,7 @@ bool ClientSocketPoolBaseHelper::Group::HasConnectJobForHandle(
 }
 
 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) {
@@ -1377,33 +1390,59 @@ void ClientSocketPoolBaseHelper::Group::InsertPendingRequest(
   }
 }
 
-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>();
+}
+
+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<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())