Add reprioritization to socket pools.

net/socket/client_socket_handle.cc

Index: net/socket/client_socket_handle.cc
diff --git a/net/socket/client_socket_handle.cc b/net/socket/client_socket_handle.cc
index 0dc7b39309cfedafb6f12b9a47d1e4c549403a08..f4e8ae5e13f0c307b2ec10d6f5861d7bbcc65bd3 100644
--- a/net/socket/client_socket_handle.cc
+++ b/net/socket/client_socket_handle.cc
@@ -31,6 +31,11 @@ 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();