Race preconnects to HTTP2 proxies that support alternate proxies

net/http/http_stream_factory_impl_job_controller.cc

Index: net/http/http_stream_factory_impl_job_controller.cc
diff --git a/net/http/http_stream_factory_impl_job_controller.cc b/net/http/http_stream_factory_impl_job_controller.cc
index 7e76ce5ce3436767ba5f47262d75ec4dbcaf16b8..5665ae85b34bbb3415ba451d4da47791880b3e1a 100644
--- a/net/http/http_stream_factory_impl_job_controller.cc
+++ b/net/http/http_stream_factory_impl_job_controller.cc
@@ -98,6 +98,10 @@ void HttpStreamFactoryImpl::JobController::Preconnect(
   DCHECK(!alternative_job_);
 
   is_preconnect_ = true;
+
+  can_start_alternative_proxy_job_ =
+      session_->params().race_preconnects_to_proxies;
+
   HostPortPair destination(HostPortPair::FromURL(request_info.url));
   GURL origin_url = ApplyHostMappingRules(request_info.url, &destination);
 
@@ -390,19 +394,30 @@ void HttpStreamFactoryImpl::JobController::OnResolveProxyComplete(
     return;
   }
 
-  DCHECK(main_job_);
-  DCHECK_EQ(MAIN, job->job_type());
+  DCHECK(is_preconnect_ || job->job_type() == MAIN);
+  DCHECK_EQ(main_job_.get(), job);
   DCHECK(!alternative_job_);
   DCHECK(!main_job_is_blocked_);
+  DCHECK(alternative_proxy_server.is_quic());

[Ryan Hamilton, 2017/01/23 18:05:53]

Why is this guaranteed to be true?

[tbansal1, 2017/01/23 21:48:22]

Removed. (Currently it is true because QUIC is the only alternative service).

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

We also support HTTP/2 based Alt-Svc.

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

[Ryan Hamilton, 2017/01/23 18:05:53]

It seems like this is a bit orthogonal to racing alt-proxies, isn't it?

[tbansal1, 2017/01/23 21:48:22]

The preconnections for non-proxy http2 jobs restrict the number of preconnect
streams to 1.
https://cs.chromium.org/chromium/src/net/http/http_stream_factory_impl_job.cc...

This CL enables preconnects for proxy http2 jobs. So, we should also restrict
the preconnect streams to 1 for proxy http2/quic jobs.

I can do it in a separate CL too, but then I would need to plumb |num_streams|
from the Job to
HttpStreamFactoryImpl::JobController::StartAlternativeProxyServerJob() method
below.

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

Ah, I see. You're saying that "restrict to 1 stream" is inherently part of
preconnect, and since this CL implements a new preconnect mechanism, we need to
do the same thing. Did I get that right?

+  }
 
   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;
@@ -482,10 +497,28 @@ void HttpStreamFactoryImpl::JobController::OnNewSpdySessionReady(
 }
 
 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.PreconnectJobsRaced",
+                               1, 2);

[Ryan Hamilton, 2017/01/23 18:05:53]

I don't understand what this histogram is recording?

[tbansal1, 2017/01/23 21:48:23]

It is just recording how many times preconnect jobs were raced. I have changed
the variable name.

+  }
+
+  // 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(
@@ -682,6 +715,7 @@ void HttpStreamFactoryImpl::JobController::CreateJobs(
 
 void HttpStreamFactoryImpl::JobController::AttachJob(Job* job) {
   DCHECK(job);
+  DCHECK(!is_preconnect_);
   factory_->request_map_[job] = request_;
 }
 
@@ -1006,12 +1040,6 @@ bool HttpStreamFactoryImpl::JobController::
     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;
-  }
-
   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.
@@ -1079,10 +1107,21 @@ void HttpStreamFactoryImpl::JobController::ReportAlternateProtocolUsage(
 }
 
 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 */);
+  }
 }
 
 }  // namespace net