Change the interface of GetAlternativeServicesFor, always return the best Alt-Svc entry.

net/http/http_stream_factory_impl.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.h"
 
 #include <string>
 
 #include "base/logging.h"
 #include "base/stl_util.h"
@@ skipping 76 matching lines @@
     const BoundNetLog& net_log) {
   Request* request = new Request(request_info.url,
                                  this,
                                  delegate,
                                  websocket_handshake_stream_create_helper,
                                  net_log);
   Job* job = new Job(this, session_, request_info, priority, server_ssl_config,
                      proxy_ssl_config, net_log.net_log());
   request->AttachJob(job);
 
-  const AlternativeServiceVector alternative_service_vector =
-      GetAlternativeServicesFor(request_info.url, delegate);
+  const AlternativeService alternative_service =
+      GetAlternativeServiceFor(request_info, delegate);
 
-  if (!alternative_service_vector.empty()) {
-    // TODO(bnc): Pass on multiple alternative services to Job.
-    const AlternativeService& alternative_service =
-        alternative_service_vector[0];
+  if (alternative_service.protocol != UNINITIALIZED_ALTERNATE_PROTOCOL) {
     // Never share connection with other jobs for FTP requests.
     DCHECK(!request_info.url.SchemeIs("ftp"));
 
     Job* alternative_job =
         new Job(this, session_, request_info, priority, server_ssl_config,
                 proxy_ssl_config, alternative_service, net_log.net_log());
     request->AttachJob(alternative_job);
 
     job->WaitFor(alternative_job);
     // Make sure to wait until we call WaitFor(), before starting
     // |alternative_job|, otherwise |alternative_job| will not notify |job|
     // appropriately.
     alternative_job->Start(request);
   }
 
   // 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 |job| is always safe.
   job->Start(request);
   return request;
 }
 
 void HttpStreamFactoryImpl::PreconnectStreams(
     int num_streams,
     const HttpRequestInfo& request_info,
     const SSLConfig& server_ssl_config,
     const SSLConfig& proxy_ssl_config) {
   DCHECK(!for_websockets_);
-  AlternativeService alternative_service;
-  AlternativeServiceVector alternative_service_vector =
-      GetAlternativeServicesFor(request_info.url, nullptr);
-  if (!alternative_service_vector.empty()) {
-    // TODO(bnc): Pass on multiple alternative services to Job.
-    alternative_service = alternative_service_vector[0];
+  AlternativeService alternative_service =
+      GetAlternativeServiceFor(request_info, nullptr);
+  if (alternative_service.protocol != UNINITIALIZED_ALTERNATE_PROTOCOL) {
     if (session_->params().quic_disable_preconnect_if_0rtt &&
         alternative_service.protocol == QUIC &&
         session_->quic_stream_factory()->ZeroRTTEnabledFor(QuicServerId(
             alternative_service.host_port_pair(), request_info.privacy_mode))) {
       return;
     }
   }
 
   // Due to how the socket pools handle priorities and idle sockets, only IDLE
   // priority currently makes sense for preconnects. The priority for
   // preconnects is currently ignored (see RequestSocketsForPool()), but could
   // be used at some point for proxy resolution or something.
   Job* job =
       new Job(this, session_, request_info, IDLE, server_ssl_config,
               proxy_ssl_config, alternative_service, session_->net_log());
   preconnect_job_set_.insert(job);
   job->Preconnect(num_streams);
 }
 
 const HostMappingRules* HttpStreamFactoryImpl::GetHostMappingRules() const {
   return session_->params().host_mapping_rules;
 }
 
-AlternativeServiceVector HttpStreamFactoryImpl::GetAlternativeServicesFor(
-    const GURL& original_url,
+AlternativeService HttpStreamFactoryImpl::GetAlternativeServiceFor(
+    const HttpRequestInfo& request_info,
     HttpStreamRequest::Delegate* delegate) {
+  GURL original_url = request_info.url;
+
   if (original_url.SchemeIs("ftp"))
-    return AlternativeServiceVector();
+    return AlternativeService();
 
   HostPortPair origin = HostPortPair::FromURL(original_url);
   HttpServerProperties& http_server_properties =
       *session_->http_server_properties();
   const AlternativeServiceVector alternative_service_vector =
       http_server_properties.GetAlternativeServices(origin);
   if (alternative_service_vector.empty())
-    return AlternativeServiceVector();
+    return AlternativeService();
 
   bool quic_advertised = false;
   bool quic_all_broken = true;
 
   const bool enable_different_host =
       session_->params().use_alternative_services;
 
-  AlternativeServiceVector enabled_alternative_service_vector;
+  AlternativeService preferred_alternative_service;
+  AlternativeService candidate_alternative_service;
   for (const AlternativeService& alternative_service :
        alternative_service_vector) {
     DCHECK(IsAlternateProtocolValid(alternative_service.protocol));
     if (!quic_advertised && alternative_service.protocol == QUIC)
       quic_advertised = true;
     if (http_server_properties.IsAlternativeServiceBroken(
             alternative_service)) {
       HistogramAlternateProtocolUsage(ALTERNATE_PROTOCOL_USAGE_BROKEN);
       continue;
     }
@@ skipping 15 matching lines @@
 
     origin.set_port(alternative_service.port);
     if (alternative_service.protocol >= NPN_SPDY_MINIMUM_VERSION &&
         alternative_service.protocol <= NPN_SPDY_MAXIMUM_VERSION) {
       if (!HttpStreamFactory::spdy_enabled())
         continue;
 
       if (session_->HasSpdyExclusion(origin))
         continue;
 
-      enabled_alternative_service_vector.push_back(alternative_service);
+      // enabled_alternative_service_vector.push_back(alternative_service);
+      // Cache the first entry if no preferred Alt-Svc selected yet.
+      if (candidate_alternative_service.protocol ==
+              UNINITIALIZED_ALTERNATE_PROTOCOL &&
+          preferred_alternative_service.protocol ==
+              UNINITIALIZED_ALTERNATE_PROTOCOL) {
+        candidate_alternative_service = alternative_service;
+      }
       continue;
     }
 
     DCHECK_EQ(QUIC, alternative_service.protocol);
     quic_all_broken = false;
     if (!session_->params().enable_quic)
       continue;
 
     if (session_->quic_stream_factory()->IsQuicDisabled(origin.port()))
       continue;
 
     if (!original_url.SchemeIs("https"))
       continue;
 
-    enabled_alternative_service_vector.push_back(alternative_service);
+    if (preferred_alternative_service.protocol ==
+        UNINITIALIZED_ALTERNATE_PROTOCOL) {
+      // Check to see whether there's an active session to pool for this QUIC
+      // alternative service.
+      HostPortPair destination = alternative_service.host_port_pair();
+      StringPiece origin_host =
+          ApplyHostMappingRules(request_info.url, &destination).host();

[Ryan Hamilton, 2015/12/18 17:52:51]

Are you sure this is correct? It may well be, but it's not obvious to me.

[Zhongyi Shi, 2015/12/18 21:59:29]

I still leave the origin_host calculation here, but removed the one in
HttpStreamFactoryImpl::Job::DoStart() by passing server and origin_url to the
cxtor of Job so as to avoid calling HttpStreamFactoryImpl::ApplyHostMappingRules
from there.

+      if (session_->quic_stream_factory()->CanPool(
+              destination, request_info.privacy_mode, origin_host))
+        preferred_alternative_service = alternative_service;

[Ryan Hamilton, 2015/12/18 17:52:51]

I'm not sure that I understand the dance here with preferred vs candidate. Can
you simplify this by just returning at this point?

[Zhongyi Shi, 2015/12/18 17:57:23]

Candidate is the first entry of the list. While preferred is the QUIC Alt-Svc
that has a active session already. 

The reason that I don't do an early return: we need to go thru the whole list to
identify whether QUIC should be marked broken for this origin (domain
reliability). Does that make sense?

[Ryan Hamilton, 2015/12/18 19:52:59]

Are you specifically thinking about the call to delegate->OnQuicBroken()? If I'm
reading that correctly, it only happens if all QUIC Alt-Svcs are broken. But if
we're able to call CanPool() then we've already set quic_all_broken to false,
right? (And if we can actually use that QUIC Alt-Svc it seems like QUIC is by
definition not broken?)

[Zhongyi Shi, 2015/12/18 21:59:29]

Ah, thanks for pointing this out! If we are able to find a QUIC Alt-Svc that has
a active quic connection for it, we already marked QUIC as non-broken. Early
return should be fine here. Thanks for pointing it out. I will change it in this
cl. Also will upload the other cl that defer the selection to job. But does that
sounds like doing the selection here is more cheap here? We will avoid going
thru the whole alt-svc list for one round, also get early return from the
preconnect streams rather than bothering wait until the DoStart in the job?

+    }
+    // enabled_alternative_service_vector.push_back(alternative_service);
+    if (candidate_alternative_service.protocol ==
+            UNINITIALIZED_ALTERNATE_PROTOCOL &&
+        preferred_alternative_service.protocol ==
+            UNINITIALIZED_ALTERNATE_PROTOCOL) {
+      candidate_alternative_service = alternative_service;
+    }
   }
   if (quic_advertised && quic_all_broken && delegate != nullptr)
     delegate->OnQuicBroken();
-  return enabled_alternative_service_vector;
+  if (preferred_alternative_service.protocol ==
+      UNINITIALIZED_ALTERNATE_PROTOCOL)
+    return candidate_alternative_service;
+  return preferred_alternative_service;
 }
 
 void HttpStreamFactoryImpl::OrphanJob(Job* job, const Request* request) {
   DCHECK(ContainsKey(request_map_, job));
   DCHECK_EQ(request_map_[job], request);
   DCHECK(!ContainsKey(orphaned_job_set_, job));
 
   request_map_.erase(job);
 
   orphaned_job_set_.insert(job);
@@ skipping 45 matching lines @@
   delete job;
 }
 
 void HttpStreamFactoryImpl::OnPreconnectsComplete(const Job* job) {
   preconnect_job_set_.erase(job);
   delete job;
   OnPreconnectsCompleteInternal();
 }
 
 }  // namespace net