Add reprioritization to socket pools.

net/socket/client_socket_handle.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_handle.h"
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
@@ skipping 13 matching lines @@
       callback_(base::Bind(&ClientSocketHandle::OnIOComplete,
                            base::Unretained(this))),
       is_ssl_error_(false),
       ssl_failure_state_(SSL_FAILURE_NONE) {
 }
 
 ClientSocketHandle::~ClientSocketHandle() {
   Reset();
 }
 
+void ClientSocketHandle::SetPriority(RequestPriority priority) {
+  if (pool_)

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

When is this called without a pool?

[Randy Smith (Not in Mondays), 2017/01/03 21:09:08]

Well, as I read the code, HttpStreamFactoryImpl::Job allocates a
ClientSocketHandle for itself upon creation.  That handle doesn't get passed
into the pools for initialization until DoInitConnection.  So for all the states
before that, the ClientSocketHandle doesn't have a pool.  I'd think that
URLRequest::SetPriority() could be called anytime during that state machine, and
that that would propagate down to this level (and if that's not currently
plumbed, it should be, but I think it is :-}).  

Am I missing something?

[mmenke, 2017/01/03 21:28:22]

You're right, but setting the priority of an "uninitialized" object seems weird
(It's safe, but....but...).  I wonder if we can just create the
ClientSocketHandle in ClientSocketHandle::DoInitConnectionImpl(), and not call
SetPriority if ClientSocketHandle is nullptr.

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

So I looked into this option in response to your comment, and it makes me
nervous in the "there's something here I don't understand" way.  The
ClientSocketHandle::Reset() is called at various points in
HttpStreamFactoryImpl::Job, I presume so that the state diagram can wrap back up
to before it was initialized (because why else would you bother).  Those Reset()
calls would turn into frees (client_socket_handle_->Reset() ==>
client_socket_handle.reset()), but that brings up the question of why Reset()
exists at all--the delete and recreate idiom strikes me as more natural.  It may
be for historical reasons or because ClientSocketHandle needs to be reset
sometimes when it's a data member for a class (though why isn't that true here
if so?), but it might also be because there's some hidden cost about
deallocating and reallocating the object.

I looked at just forbidding setting the priority of an uninitialized
ClientSocketHandle (ClientSocketHandle::Init() takes a priority and
HttpStreamFactoryImpl::Job stores the priority, so I can just no-op the
HSFI::J::SetPriority call if ClientSocketHandle hasn't been initialized yet),
but the problem is that an error return of a ClientSocketHandle from the pools
will put it into an uninitialized state, which makes me feel like I can't forbid
consumers from calling SetPriority in an uninitialized state.  I could say
something like "SetPriority() may not be called unless Init() has previously
been called, has not completed in error, and Reset() has not been called" but
that's a) a pretty complicated contract, and b) requires the consumer to track
whether or not it's received the callback and the initialization has completed
in error.  

So I think this is the least bad option?  But I'm open to perspectives I haven't
come up with, or just plain argument :-}.

+    pool_->SetPriority(group_name_, this, priority);

[mmenke, 2017/01/03 21:28:22]

Also, this is calling SetPriority even after this ClientSocketHandle has a
socket, which doesn't seem right.  A bit unfortunate that the ClientSocketHandle
 both serves as a ClientSocketHandle and a SocketRequestHandle, but fixing that
is well beyond the scope of this CL.

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

It's worse than doesn't seem right--it can produce an error if the pool has no
knowledge of the request (as happens after the socket's been allocated).  I
fixed that in the most recent patchset, but by making SetPriority() a no-op if
there's a socket.  

I'm inclined to think that that is the right choice, as I could conceivably
imagine a meaning for SetPriority() on a ClientSocketHandle with a socket (it
could affect TCP params).  I have no intention of implementing something like
that in the short term, but it makes me think the right solution is no-op'ing in
the implementation rather than forbidding the call in the API.  Probably not an
issue for this CL, as you say, but WDYT of that argument?

+}
+
 void ClientSocketHandle::Reset() {
   ResetInternal(true);
   ResetErrorState();
 }
 
 void ClientSocketHandle::ResetInternal(bool cancel) {
   // Was Init called?
   if (!group_name_.empty()) {
     // If so, we must have a pool.
     CHECK(pool_);
@@ skipping 125 matching lines @@
   // TODO(eroman): This logging is not complete, in particular set_socket() and
   // release() socket. It ends up working though, since those methods are being
   // used to layer sockets (and the destination sources are the same).
   DCHECK(socket_.get());
   socket_->NetLog().BeginEvent(
       NetLog::TYPE_SOCKET_IN_USE,
       requesting_source_.ToEventParametersCallback());
 }
 
 }  // namespace net