Add reprioritization to socket pools.

net/http/http_stream_factory_impl_job.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/http/http_stream_factory_impl_job.h"
 
 #include <algorithm>
 #include <string>
 
 #include "base/bind.h"
@@ skipping 300 matching lines @@
     if (connection_ && connection_->socket()) {
       connection_->socket()->Disconnect();
     }
     delegate_->OnOrphanedJobComplete(this);
   }
   // |this| may be deleted after this call.
 }
 
 void HttpStreamFactoryImpl::Job::SetPriority(RequestPriority priority) {
   priority_ = priority;
-  // TODO(akalin): Propagate this to |connection_| and maybe the
-  // preconnect state.
+  // Ownership of |connection_| is passed to the newly created stream
+  // or H2 session in DoCreateStream(), and the consumer is not
+  // notified immediately, so this call may occur when |connection_|
+  // is null.
+  // TODO(rdsmith): Also set the priority on the stream, if it's present, to
+  // make sure the above race doesn't drop a reprioritization on the floor.

[mmenke, 2017/01/05 20:18:35]

I see some ways to test it, but nothing that seems sane.

Mock SSL socket pool, mock SSL socket, set it so we get the handshake message
that tells us we have an H2 connection on demand (That message may actually be
bypassed, so need to connect on demand - if so, need to implement that.  IF it's
just a mock read that does that, that's already supported).  Then once you let
the handshake complete, the session and stream will by synchronously created, so
you can just set the priority at that point, and then make sure the right
priority is sent over the wire.

Could alternative mock out the SpdySessionPool/SpdySession and but no tests do
that.

Another option would be to make HttpStream's priority observable (And
HttpBasicStream track the priority), but not sure we want to add too much stuff
to the HttpStreamInterface.  That would work as an HttpStreamFactoryImpl test. 
Getting in before the callback would require the same strategy as above, with
the extra H2 stuff.

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

You know, unless you object after reading the following points, I'm going to say
heck with this case and completely remove the TODO.  Consider: The HttpStream
API has the consumer of the stream specifying the priority via InitializeStream,
*not* the constructor (also true for the HttpBasicStream constructor, and the
other constructors in the subclass tree I spot-checked).  InitializeStream is
called from HttpNetworkSession, long after HttpStreamFactoryImpl::Job completes,
and before (I believe) we actually do anything with the stream.  

So I think the conceptual presentation of HttpStream is "It doesn't have a
priority until it's initialized" and that anything that needs the HttpStream
priority should be done after it's initialized.  Thus I think that SetPriority
shouldn't be called before initialization, and the right thing to do with
HttpStreamFactoryImpl::Job::SetPriority after the stream is created is to drop
it on the floor.

This means that I still want a crash test (to make sure I don't indirect through
the non-existent stream request) but that's easier.

+  if (connection_ && connection_->is_initialized())
+    connection_->SetPriority(priority);
+  // TODO(akalin): Maybe Propagate this to the preconnect state.
 }
 
 bool HttpStreamFactoryImpl::Job::was_alpn_negotiated() const {
   return was_alpn_negotiated_;
 }
 
 NextProto HttpStreamFactoryImpl::Job::negotiated_protocol() const {
   return negotiated_protocol_;
 }
 
@@ skipping 1172 matching lines @@
 
   ConnectionAttempts socket_attempts = connection_->connection_attempts();
   if (connection_->socket()) {
     connection_->socket()->GetConnectionAttempts(&socket_attempts);
   }
 
   delegate_->AddConnectionAttemptsToRequest(this, socket_attempts);
 }
 
 }  // namespace net