Race preconnects to HTTP2 proxies that support alternate proxies

net/http/http_stream_factory_impl_job_controller.cc

 // Copyright (c) 2016 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_controller.h"
 
 #include <memory>
 #include <string>
 #include <utility>
 
@@ skipping 80 matching lines @@
 
 void HttpStreamFactoryImpl::JobController::Preconnect(
     int num_streams,
     const HttpRequestInfo& request_info,
     const SSLConfig& server_ssl_config,
     const SSLConfig& proxy_ssl_config) {
   DCHECK(!main_job_);
   DCHECK(!alternative_job_);
 
   is_preconnect_ = true;
+
+  can_start_alternative_proxy_job_ =
+      session_->params().race_preconnects_to_http2_proxies;
+
   HostPortPair destination(HostPortPair::FromURL(request_info.url));
   GURL origin_url = ApplyHostMappingRules(request_info.url, &destination);
 
   const AlternativeService alternative_service = GetAlternativeServiceFor(
       request_info, nullptr, HttpStreamRequest::HTTP_STREAM);
 
   if (alternative_service.protocol != kProtoUnknown) {
     if (session_->params().quic_disable_preconnect_if_0rtt &&
         alternative_service.protocol == kProtoQUIC &&
         session_->quic_stream_factory()->ZeroRTTEnabledFor(QuicServerId(
@@ skipping 276 matching lines @@
     HttpStreamRequest::StreamType stream_type) {
   DCHECK(job);
 
   ProxyServer alternative_proxy_server;
   if (!ShouldCreateAlternativeProxyServerJob(job, job->proxy_info(),
                                              request_info.url,
                                              &alternative_proxy_server)) {
     return;
   }
 
-  DCHECK(main_job_);
-  DCHECK_EQ(MAIN, job->job_type());
+  DCHECK(is_preconnect_ || job->job_type() == MAIN);
+  DCHECK_EQ(main_job_.get(), job);

[Ryan Hamilton, 2017/01/24 19:36:59]

The previous line suggest that this job might not be the main job if
is_preconnect_ is true. If that's the case, isn't this DCHECK invalid? Or
alternatively, should is_preconnect_ be removed from the first DCHECK? (Or more
likely than either, am I confused?)

   DCHECK(!alternative_job_);
   DCHECK(!main_job_is_blocked_);
 
+  if (is_preconnect_) {
+    // Restrict the number of streams to 1 since the proxy server supports spdy.
+    job->RestrictNumStreamsToOne();
+  }

[Ryan Hamilton, 2017/01/24 19:36:59]

So this restricts the number of streams on the main job, right? But if we're
doing a preconnect for the main job, wouldn't we have started it by calling
Preconnect() which sets num_streams to 1?

+
   HostPortPair destination(HostPortPair::FromURL(request_info.url));
   GURL origin_url = ApplyHostMappingRules(request_info.url, &destination);
 
   alternative_job_.reset(job_factory_->CreateJob(
-      this, ALTERNATIVE, session_, request_info, priority, server_ssl_config,
-      proxy_ssl_config, destination, origin_url, alternative_proxy_server,
-      job->net_log().net_log()));
-  AttachJob(alternative_job_.get());
+      this, is_preconnect_ ? PRECONNECT : ALTERNATIVE, session_, request_info,
+      priority, server_ssl_config, proxy_ssl_config, destination, origin_url,
+      alternative_proxy_server, job->net_log().net_log()));
+
+  if (!is_preconnect_) {
+    // Preconnect jobs do not have an associated request. So, the preconnect
+    // job can't be attached.
+    AttachJob(alternative_job_.get());
+  }
 
   can_start_alternative_proxy_job_ = false;
   main_job_is_blocked_ = true;

[Ryan Hamilton, 2017/01/24 19:36:59]

I believe this all means that the main job will now pause and give the alt-job a
head start. (Which is desirable). Can you make sure we have a test of this case?
(Where we don't send any traffic to the HTTP/2 proxy if the QUIC proxy can do
0-RTT.)

 
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE,
       base::Bind(
           &HttpStreamFactoryImpl::JobController::StartAlternativeProxyServerJob,
           ptr_factory_.GetWeakPtr()));
 }
 
 void HttpStreamFactoryImpl::JobController::OnNewSpdySessionReady(
     Job* job,
@@ skipping 56 matching lines @@
     factory->OnNewSpdySessionReady(spdy_session, direct, used_ssl_config,
                                    used_proxy_info, was_alpn_negotiated,
                                    negotiated_protocol, using_spdy, net_log);
   }
   if (is_job_orphaned) {
     OnOrphanedJobComplete(job);
   }
 }
 
 void HttpStreamFactoryImpl::JobController::OnPreconnectsComplete(Job* job) {
-  DCHECK_EQ(main_job_.get(), job);
-  main_job_.reset();
-  factory_->OnPreconnectsCompleteInternal();
-  MaybeNotifyFactoryOfCompletion();
+  DCHECK(job);
+  DCHECK(main_job_.get() == job || alternative_job_.get() == job);
+  DCHECK_EQ(PRECONNECT, job->job_type());
+  if (is_preconnect_ && main_job_ && alternative_job_) {
+    UMA_HISTOGRAM_EXACT_LINEAR(
+        "Net.QuicAlternativeProxy.PreconnectRace.FirstJobFinished", 1, 2);

[Ryan Hamilton, 2017/01/24 19:36:59]

I'm not sure "FirstJobFinished" adds any value to the name of the histograms.
I'd probably remove it.

Also, you might consider making the histogram's value be true if the main job
finished first, and false otherwise (or use an enum). This will help us
understand what's going on better.

+  }
+
+  // Note that completion of a preconnect job does not indicate that
+  // preconnection was actually successful since the socket layer currently does
+  // not provide the success or failure status of the (possibly) multiple
+  // preconnect sockets.
+  if (job == main_job_.get())
+    main_job_.reset();
+
+  if (job == alternative_job_.get())
+    alternative_job_.reset();
+
+  if (!main_job_ && !alternative_job_) {
+    factory_->OnPreconnectsCompleteInternal();
+    MaybeNotifyFactoryOfCompletion();
+  }
 }
 
 void HttpStreamFactoryImpl::JobController::OnOrphanedJobComplete(
     const Job* job) {
   if (job->job_type() == MAIN) {
     DCHECK_EQ(main_job_.get(), job);
     main_job_.reset();
   } else {
     DCHECK_EQ(alternative_job_.get(), job);
     alternative_job_.reset();
@@ skipping 193 matching lines @@
     can_start_alternative_proxy_job_ = true;
   }
   // Even if |alternative_job| has already finished, it will not have notified
   // the request yet, since we defer that to the next iteration of the
   // MessageLoop, so starting |main_job_| is always safe.
   main_job_->Start(request_->stream_type());
 }
 
 void HttpStreamFactoryImpl::JobController::AttachJob(Job* job) {
   DCHECK(job);
+  DCHECK(!is_preconnect_);
   factory_->request_map_[job] = request_;
 }
 
 void HttpStreamFactoryImpl::JobController::BindJob(Job* job) {
   DCHECK(request_);
   DCHECK(job);
   DCHECK(job == alternative_job_.get() || job == main_job_.get());
   DCHECK(!job_bound_);
   DCHECK(!bound_job_);
 
@@ skipping 314 matching lines @@
     // already been started.
     return false;
   }
 
   if (job->job_type() == ALTERNATIVE) {
     // If |job| is using alternative service, then alternative proxy server
     // should not be used.
     return false;
   }
 
-  if (is_preconnect_ || job->job_type() == PRECONNECT) {
-    // Preconnects should be fetched using only the main job to keep the
-    // resource utilization down.
-    return false;

[Ryan Hamilton, 2017/01/24 19:36:59]

you could probably add a DCHECK here checking the param which enable preconnects
to alt proxies.

-  }
-
   if (proxy_info.is_empty() || proxy_info.is_direct() || proxy_info.is_quic()) {
     // Alternative proxy server job can be created only if |job| fetches the
     // |request_| through a non-QUIC proxy.
     return false;
   }
 
   if (!url.SchemeIs(url::kHttpScheme)) {
     // Only HTTP URLs can be fetched through alternative proxy server, since the
     // alternative proxy server may not support fetching of URLs with other
     // schemes.
@@ skipping 47 matching lines @@
     HistogramAlternateProtocolUsage(ALTERNATE_PROTOCOL_USAGE_NO_RACE,
                                     proxy_server_used);
     return;
   }
 
   HistogramAlternateProtocolUsage(ALTERNATE_PROTOCOL_USAGE_WON_RACE,
                                   proxy_server_used);
 }
 
 void HttpStreamFactoryImpl::JobController::StartAlternativeProxyServerJob() {
-  if (!alternative_job_ || !request_)
+  if (!alternative_job_)
     return;
   DCHECK(alternative_job_->alternative_proxy_server().is_valid());
-  alternative_job_->Start(request_->stream_type());
+
+  if (!is_preconnect_) {
+    if (!request_)
+      return;
+    DCHECK(alternative_job_->alternative_proxy_server().is_quic());
+    alternative_job_->Start(request_->stream_type());
+  } else {
+    // Restrict the number of streams to 1 since the alternative proxy server
+    // supports request priorities.
+    DCHECK(alternative_job_->alternative_proxy_server().is_quic());
+    alternative_job_->Preconnect(1 /* num_streams */);
+  }

[Ryan Hamilton, 2017/01/24 19:36:59]

nit: I'd structure this slightly differently:

if (is_preconnect) {
 ... Preconnect();
 return;
}

if (!request_)
  return;

...

This avoid a bit of nesting which is always nice.

 }
 
 bool HttpStreamFactoryImpl::JobController::IsJobOrphaned(Job* job) const {
   return !request_ || (job_bound_ && bound_job_ != job);
 }
 
 }  // namespace net