Refactor pooled socket request code to prepare for implementation of pretend-to-preconnect.

net/socket/client_socket_pool_manager.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/socket/client_socket_pool_manager.h"
 
 #include <string>
 
 #include "base/basictypes.h"
 #include "base/logging.h"
@@ skipping 46 matching lines @@
 // http://crbug.com/44501 for details about proxy server connection limits.
 int g_max_sockets_per_proxy_server[] = {
   kDefaultMaxSocketsPerProxyServer,  // NORMAL_SOCKET_POOL
   kDefaultMaxSocketsPerProxyServer   // WEBSOCKET_SOCKET_POOL
 };
 
 COMPILE_ASSERT(arraysize(g_max_sockets_per_proxy_server) ==
                    HttpNetworkSession::NUM_SOCKET_POOL_TYPES,
                max_sockets_per_proxy_server_length_mismatch);
 
-// The meat of the implementation for the InitSocketHandleForHttpRequest,
-// InitSocketHandleForRawConnect and PreconnectSocketsForHttpRequest methods.
-int InitSocketPoolHelper(const GURL& request_url,
-                         const HttpRequestHeaders& request_extra_headers,
-                         int request_load_flags,
-                         RequestPriority request_priority,
-                         HttpNetworkSession* session,
-                         const ProxyInfo& proxy_info,
-                         bool force_spdy_over_ssl,
-                         bool want_spdy_over_npn,
-                         const SSLConfig& ssl_config_for_origin,
-                         const SSLConfig& ssl_config_for_proxy,
-                         bool force_tunnel,
-                         const BoundNetLog& net_log,
-                         int num_preconnect_streams,
-                         ClientSocketHandle* socket_handle,
-                         const OnHostResolutionCallback& resolution_callback,
-                         const CompletionCallback& callback) {
-  scoped_refptr<TransportSocketParams> tcp_params;
-  scoped_refptr<HttpProxySocketParams> http_proxy_params;
-  scoped_refptr<SOCKSSocketParams> socks_params;
-  scoped_ptr<HostPortPair> proxy_host_port;
-
-  bool using_ssl = request_url.SchemeIs("https") || force_spdy_over_ssl;
-
-  HostPortPair origin_host_port =
-      HostPortPair(request_url.HostNoBrackets(),
-                   request_url.EffectiveIntPort());
-
-  if (!using_ssl && session->params().testing_fixed_http_port != 0) {
-    origin_host_port.set_port(session->params().testing_fixed_http_port);
-  } else if (using_ssl && session->params().testing_fixed_https_port != 0) {
-    origin_host_port.set_port(session->params().testing_fixed_https_port);
-  }
-
-  bool disable_resolver_cache =
-      request_load_flags & LOAD_BYPASS_CACHE ||
-      request_load_flags & LOAD_VALIDATE_CACHE ||
-      request_load_flags & LOAD_DISABLE_CACHE;
-
-  int load_flags = request_load_flags;
-  if (session->params().ignore_certificate_errors)
-    load_flags |= LOAD_IGNORE_ALL_CERT_ERRORS;
-
+std::string ConnectionGroupForUrl(const GURL& request_url,
+                                  bool force_ssl,
+                                  const HostPortPair& host_port,
+                                  uint16 ssl_version_max,
+                                  ProxyServer::Scheme proxy_scheme) {
   // Build the string used to uniquely identify connections of this type.
   // Determine the host and port to connect to.
-  std::string connection_group = origin_host_port.ToString();
+  std::string connection_group = host_port.ToString();
   DCHECK(!connection_group.empty());
   if (request_url.SchemeIs("ftp")) {
-    // Combining FTP with forced SPDY over SSL would be a "path to madness".
-    // Make sure we never do that.
-    DCHECK(!using_ssl);
     connection_group = "ftp/" + connection_group;
-  }
-  if (using_ssl) {
+  } else if (force_ssl || request_url.SchemeIs("https")) {
     // All connections in a group should use the same SSLConfig settings.
     // Encode version_max in the connection group's name, unless it's the
     // default version_max. (We want the common case to use the shortest
     // encoding). A version_max of TLS 1.1 is encoded as "ssl(max:3.2)/"
     // rather than "tlsv1.1/" because the actual protocol version, which
     // is selected by the server, may not be TLS 1.1. Do not encode
     // version_min in the connection group's name because version_min
     // should be the same for all connections, whereas version_max may
     // change for version fallbacks.
     std::string prefix = "ssl/";
-    if (ssl_config_for_origin.version_max !=
+    if (ssl_version_max !=
         SSLConfigService::default_version_max()) {
-      switch (ssl_config_for_origin.version_max) {
+      switch (ssl_version_max) {
         case SSL_PROTOCOL_VERSION_TLS1_2:
           prefix = "ssl(max:3.3)/";
           break;
         case SSL_PROTOCOL_VERSION_TLS1_1:
           prefix = "ssl(max:3.2)/";
           break;
         case SSL_PROTOCOL_VERSION_TLS1:
           prefix = "ssl(max:3.1)/";
           break;
         case SSL_PROTOCOL_VERSION_SSL3:
           prefix = "sslv3/";
           break;
         default:
           CHECK(false);
           break;
       }
     }
     connection_group = prefix + connection_group;
   }
 
-  bool ignore_limits = (request_load_flags & LOAD_IGNORE_LIMITS) != 0;
+  char socks_version = 0;
+  if (proxy_scheme == ProxyServer::SCHEME_SOCKS4)
+    socks_version = '4';
+  else if (proxy_scheme == ProxyServer::SCHEME_SOCKS5)
+    socks_version = '5';
+
+  if (socks_version)
+    connection_group = base::StringPrintf("socks%c/%s", socks_version,
+                                          connection_group.c_str());
+
+  return connection_group;
+}
+
+HostPortPair HostPortPairFromUrlForSession(const GURL& request_url,
+                                           bool using_ssl,
+                                           HttpNetworkSession* session) {
+  HostPortPair host_port(request_url.HostNoBrackets(),
+                         request_url.EffectiveIntPort());
+
+  if (!using_ssl && session->params().testing_fixed_http_port != 0) {
+    host_port.set_port(session->params().testing_fixed_http_port);
+  } else if (using_ssl && session->params().testing_fixed_https_port != 0) {
+    host_port.set_port(session->params().testing_fixed_https_port);
+  }
+
+  return host_port;
+}
+
+template<typename Callback>
+int
+InvokeWithSocketParamsAndPool(
+    const GURL& request_url,
+    const HttpRequestHeaders& request_extra_headers,
+    int request_load_flags,
+    RequestPriority request_priority,
+    HttpNetworkSession* session,
+    const ProxyInfo& proxy_info,
+    bool force_spdy_over_ssl,
+    bool want_spdy_over_npn,
+    const SSLConfig& ssl_config_for_origin,
+    const SSLConfig& ssl_config_for_proxy,
+    bool force_tunnel,
+    const OnHostResolutionCallback& resolution_callback,
+    const Callback& callback) {
+  bool using_ssl = request_url.SchemeIs("https") || force_spdy_over_ssl;
+  // Combining FTP with forced SPDY over SSL would be a "path to madness".
+  // Make sure we never do that.
+  DCHECK(!(using_ssl && request_url.SchemeIs("ftp")));
+
+  HostPortPair origin_host_port =
+      HostPortPairFromUrlForSession(request_url, using_ssl, session);
+
+  ProxyServer::Scheme proxy_scheme = proxy_info.proxy_server().scheme();
+  std::string connection_group =
+      ConnectionGroupForUrl(request_url, force_spdy_over_ssl, origin_host_port,
+                            using_ssl ? ssl_config_for_origin.version_max : 0,
+                            proxy_scheme);
+
+  scoped_ptr<HostPortPair> proxy_host_port;
+  HostPortPair* target_host_port = NULL;
   if (proxy_info.is_direct()) {
-    tcp_params = new TransportSocketParams(origin_host_port,
+    target_host_port = &origin_host_port;
+  } else {
+    proxy_host_port.reset(
+        new HostPortPair(proxy_info.proxy_server().host_port_pair()));
+    target_host_port = proxy_host_port.get();
+  }
+
+  SSLClientSocketPool* ssl_pool = NULL;
+  if (using_ssl) {
+    if (proxy_info.is_direct())
+      ssl_pool = session->GetSSLSocketPool(
+          HttpNetworkSession::NORMAL_SOCKET_POOL);
+    else
+      ssl_pool = session->GetSocketPoolForSSLWithProxy(
+          HttpNetworkSession::NORMAL_SOCKET_POOL,
+          *proxy_host_port);
+  }
+
+  int load_flags = request_load_flags;
+  if (session->params().ignore_certificate_errors)
+    load_flags |= LOAD_IGNORE_ALL_CERT_ERRORS;
+
+  scoped_refptr<TransportSocketParams> tcp_params;
+  {
+    bool disable_resolver_cache =
+        request_load_flags & LOAD_BYPASS_CACHE ||
+        request_load_flags & LOAD_VALIDATE_CACHE ||
+        request_load_flags & LOAD_DISABLE_CACHE;
+    bool ignore_limits = (request_load_flags & LOAD_IGNORE_LIMITS) != 0;
+    tcp_params = new TransportSocketParams(*target_host_port,
                                            request_priority,
                                            disable_resolver_cache,
                                            ignore_limits,
                                            resolution_callback);
+  }
+
+  if (proxy_info.is_direct()) {
+    if (using_ssl) {
+      scoped_refptr<SSLSocketParams> ssl_params =
+          SSLSocketParams::CreateForDirectConnection(
+              tcp_params,
+              origin_host_port,
+              ssl_config_for_origin,
+              load_flags,
+              force_spdy_over_ssl,
+              want_spdy_over_npn);
+
+      return callback(ssl_pool, connection_group, ssl_params, request_priority);
+    } else {
+      TransportClientSocketPool* pool = session->GetTransportSocketPool(
+          HttpNetworkSession::NORMAL_SOCKET_POOL);
+
+      return callback(pool, connection_group, tcp_params, request_priority);
+    }
+  } else if (proxy_info.is_http() || proxy_info.is_https()) {
+    std::string user_agent;
+    request_extra_headers.GetHeader(HttpRequestHeaders::kUserAgent,
+                                    &user_agent);
+    scoped_refptr<SSLSocketParams> ssl_params;
+    if (proxy_info.is_https()) {
+      ssl_params = SSLSocketParams::CreateForDirectConnection(
+          tcp_params,
+          *proxy_host_port.get(),
+          ssl_config_for_proxy,
+          load_flags,
+          force_spdy_over_ssl,
+          want_spdy_over_npn);
+    }
+
+    scoped_refptr<HttpProxySocketParams> http_proxy_params =
+        new HttpProxySocketParams(proxy_info.is_https() ? NULL : tcp_params,
+                                  ssl_params,
+                                  request_url,
+                                  user_agent,
+                                  origin_host_port,
+                                  session->http_auth_cache(),
+                                  session->http_auth_handler_factory(),
+                                  session->spdy_session_pool(),
+                                  force_tunnel || using_ssl);
+
+    if (using_ssl) {
+      scoped_refptr<SSLSocketParams> ssl_params =
+          SSLSocketParams::CreateForHttpProxy(
+              http_proxy_params,
+              proxy_scheme,
+              origin_host_port,
+              ssl_config_for_origin,
+              load_flags,
+              force_spdy_over_ssl,
+              want_spdy_over_npn);
+
+      return callback(ssl_pool, connection_group, ssl_params, request_priority);
+    } else {
+      HttpProxyClientSocketPool* pool =
+          session->GetSocketPoolForHTTPProxy(
+              HttpNetworkSession::NORMAL_SOCKET_POOL,
+              *proxy_host_port);
+
+      return callback(pool, connection_group,
+                      http_proxy_params, request_priority);
+    }
   } else {
-    ProxyServer proxy_server = proxy_info.proxy_server();
-    proxy_host_port.reset(new HostPortPair(proxy_server.host_port_pair()));
-    scoped_refptr<TransportSocketParams> proxy_tcp_params(
-        new TransportSocketParams(*proxy_host_port,
-                                  request_priority,
-                                  disable_resolver_cache,
-                                  ignore_limits,
-                                  resolution_callback));
-
-    if (proxy_info.is_http() || proxy_info.is_https()) {
-      std::string user_agent;
-      request_extra_headers.GetHeader(HttpRequestHeaders::kUserAgent,
-                                      &user_agent);
-      scoped_refptr<SSLSocketParams> ssl_params;
-      if (proxy_info.is_https()) {
-        // Set ssl_params, and unset proxy_tcp_params
-        ssl_params = new SSLSocketParams(proxy_tcp_params,
-                                         NULL,
-                                         NULL,
-                                         ProxyServer::SCHEME_DIRECT,
-                                         *proxy_host_port.get(),
-                                         ssl_config_for_proxy,
-                                         load_flags,
-                                         force_spdy_over_ssl,
-                                         want_spdy_over_npn);
-        proxy_tcp_params = NULL;
-      }
-
-      http_proxy_params =
-          new HttpProxySocketParams(proxy_tcp_params,
-                                    ssl_params,
-                                    request_url,
-                                    user_agent,
-                                    origin_host_port,
-                                    session->http_auth_cache(),
-                                    session->http_auth_handler_factory(),
-                                    session->spdy_session_pool(),
-                                    force_tunnel || using_ssl);
+    DCHECK(proxy_info.is_socks());
+
+    scoped_refptr<SOCKSSocketParams> socks_params =
+        new SOCKSSocketParams(tcp_params,
+                              proxy_scheme == ProxyServer::SCHEME_SOCKS5,
+                              origin_host_port,
+                              request_priority);
+
+    if (using_ssl) {
+      scoped_refptr<SSLSocketParams> ssl_params =
+          SSLSocketParams::CreateForSocksProxy(
+              socks_params,
+              proxy_scheme,
+              origin_host_port,
+              ssl_config_for_origin,
+              load_flags,
+              force_spdy_over_ssl,
+              want_spdy_over_npn);
+
+      return callback(ssl_pool, connection_group, ssl_params, request_priority);
     } else {
-      DCHECK(proxy_info.is_socks());
-      char socks_version;
-      if (proxy_server.scheme() == ProxyServer::SCHEME_SOCKS5)
-        socks_version = '5';
-      else
-        socks_version = '4';
-      connection_group = base::StringPrintf(
-          "socks%c/%s", socks_version, connection_group.c_str());
-
-      socks_params = new SOCKSSocketParams(proxy_tcp_params,
-                                           socks_version == '5',
-                                           origin_host_port,
-                                           request_priority);
-    }
-  }
-
-  // Deal with SSL - which layers on top of any given proxy.
-  if (using_ssl) {
-    scoped_refptr<SSLSocketParams> ssl_params =
-        new SSLSocketParams(tcp_params,
-                            socks_params,
-                            http_proxy_params,
-                            proxy_info.proxy_server().scheme(),
-                            origin_host_port,
-                            ssl_config_for_origin,
-                            load_flags,
-                            force_spdy_over_ssl,
-                            want_spdy_over_npn);
-    SSLClientSocketPool* ssl_pool = NULL;
-    if (proxy_info.is_direct()) {
-      ssl_pool = session->GetSSLSocketPool(
-          HttpNetworkSession::NORMAL_SOCKET_POOL);
-    } else {
-      ssl_pool = session->GetSocketPoolForSSLWithProxy(
+      SOCKSClientSocketPool* pool = session->GetSocketPoolForSOCKSProxy(
           HttpNetworkSession::NORMAL_SOCKET_POOL,
           *proxy_host_port);
-    }
-
-    if (num_preconnect_streams) {
-      RequestSocketsForPool(ssl_pool, connection_group, ssl_params,
-                            num_preconnect_streams, net_log);
-      return OK;
-    }
-
-    return socket_handle->Init(connection_group, ssl_params,
-                               request_priority, callback, ssl_pool,
-                               net_log);
-  }
-
-  // Finally, get the connection started.
-
-  if (proxy_info.is_http() || proxy_info.is_https()) {
-    HttpProxyClientSocketPool* pool =
-        session->GetSocketPoolForHTTPProxy(
-            HttpNetworkSession::NORMAL_SOCKET_POOL,
-            *proxy_host_port);
-    if (num_preconnect_streams) {
-      RequestSocketsForPool(pool, connection_group, http_proxy_params,
-                            num_preconnect_streams, net_log);
-      return OK;
-    }
-
-    return socket_handle->Init(connection_group, http_proxy_params,
-                               request_priority, callback,
-                               pool, net_log);
-  }
-
-  if (proxy_info.is_socks()) {
-    SOCKSClientSocketPool* pool =
-        session->GetSocketPoolForSOCKSProxy(
-            HttpNetworkSession::NORMAL_SOCKET_POOL,
-            *proxy_host_port);
-    if (num_preconnect_streams) {
-      RequestSocketsForPool(pool, connection_group, socks_params,
-                            num_preconnect_streams, net_log);
-      return OK;
-    }
-
-    return socket_handle->Init(connection_group, socks_params,
-                               request_priority, callback, pool,
-                               net_log);
-  }
-
-  DCHECK(proxy_info.is_direct());
-
-  TransportClientSocketPool* pool =
-      session->GetTransportSocketPool(HttpNetworkSession::NORMAL_SOCKET_POOL);
-  if (num_preconnect_streams) {
-    RequestSocketsForPool(pool, connection_group, tcp_params,
-                          num_preconnect_streams, net_log);
+
+      return callback(pool, connection_group, socks_params, request_priority);
+    }
+  }
+
+  NOTREACHED();
+  return ERR_UNEXPECTED;
+}
+
+class RequestSocketForPool {

[mmenke, 2013/05/01 19:31:12]

I think that these are sufficiently ugly that we should just get rid of them. 
Creating a single templated function that takes both sets of parameters, and
figures out which to use, works.  It isn't pretty, but the nested templates like
this just seems to ugly to me.

In the other CL, I suggested another way of doing this that avoids the need for
the callback, and is less prone to regress.

I'm also not convinced that we gain a whole lot by measuring at the network
level instead of the predictor level.  I think that histograming there would get
us much more important / useful data (If the potentially preconnected site was
visited).  I don't think the number of preconnects that we would have used for
each motivation is at all useful without that knowledge, and I don't think it
adds a whole lot of value on top of that knowledge, either.

I also think it wouldn't be unreasonable to improve our histograms a bit and
just enable preconnecting in the new cases on Canary, individually, and keep an
eye on the histograms.  The network histograms I suggest (May or may not already
be present):  How often a request can be served by an existing socket (Either
one already connected, or one currently being connected), how long requests have
to wait for idle sockets, and how many sockets we connect but never use.  Higher
level histograms, like time per request, may also be worth looking at, but tend
to be very noisy.

If you disagree, I'm fine with a temporary load flag, though I don't want to
keep it around too long.

+public:
+  RequestSocketForPool(ClientSocketHandle* socket_handle,
+                      const CompletionCallback& callback,
+                      const BoundNetLog& net_log)
+    : socket_handle_(socket_handle),
+      callback_(callback),
+      net_log_(net_log) {}
+
+  template <typename PoolType, typename SocketParams>
+  int operator()(PoolType* pool,
+                 const std::string& group_name,
+                 const scoped_refptr<SocketParams>& params,
+                 RequestPriority request_priority) const {
+    CheckIsValidSocketParamsForPool<PoolType, SocketParams>();
+
+    return socket_handle_->Init(group_name, params, request_priority,
+                                callback_, pool, net_log_);
+  }
+
+private:
+  ClientSocketHandle* socket_handle_;
+  const CompletionCallback& callback_;
+  const BoundNetLog& net_log_;
+};
+
+class RequestSocketsForPool {
+public:
+  RequestSocketsForPool(int num_preconnect_streams,
+                        const BoundNetLog& net_log)
+    : num_preconnect_streams_(num_preconnect_streams),
+      net_log_(net_log) {}
+
+  template <typename PoolType, typename SocketParams>
+  int operator()(PoolType* pool,
+                 const std::string& group_name,
+                 const scoped_refptr<SocketParams>& params,
+                 RequestPriority request_priority) const {
+    CheckIsValidSocketParamsForPool<PoolType, SocketParams>();
+
+    pool->RequestSockets(
+        group_name, &params, num_preconnect_streams_, net_log_);
     return OK;
   }
 
-  return socket_handle->Init(connection_group, tcp_params,
-                             request_priority, callback,
-                             pool, net_log);
-}
+private:
+  int num_preconnect_streams_;
+  const BoundNetLog& net_log_;
+};
 
 }  // namespace
 
 ClientSocketPoolManager::ClientSocketPoolManager() {}
 ClientSocketPoolManager::~ClientSocketPoolManager() {}
 
 // static
 int ClientSocketPoolManager::max_sockets_per_pool(
     HttpNetworkSession::SocketPoolType pool_type) {
   DCHECK_LT(pool_type, HttpNetworkSession::NUM_SOCKET_POOL_TYPES);
@@ skipping 64 matching lines @@
     const ProxyInfo& proxy_info,
     bool force_spdy_over_ssl,
     bool want_spdy_over_npn,
     const SSLConfig& ssl_config_for_origin,
     const SSLConfig& ssl_config_for_proxy,
     const BoundNetLog& net_log,
     ClientSocketHandle* socket_handle,
     const OnHostResolutionCallback& resolution_callback,
     const CompletionCallback& callback) {
   DCHECK(socket_handle);
-  return InitSocketPoolHelper(
+
+  RequestSocketForPool socketRequest(socket_handle, callback, net_log);
+
+  return InvokeWithSocketParamsAndPool(
       request_url, request_extra_headers, request_load_flags, request_priority,
       session, proxy_info, force_spdy_over_ssl, want_spdy_over_npn,
-      ssl_config_for_origin, ssl_config_for_proxy, false, net_log, 0,
-      socket_handle, resolution_callback, callback);
+      ssl_config_for_origin, ssl_config_for_proxy, false,
+      resolution_callback, socketRequest);
 }
 
 int InitSocketHandleForRawConnect(
     const HostPortPair& host_port_pair,
     HttpNetworkSession* session,
     const ProxyInfo& proxy_info,
     const SSLConfig& ssl_config_for_origin,
     const SSLConfig& ssl_config_for_proxy,
     const BoundNetLog& net_log,
     ClientSocketHandle* socket_handle,
     const CompletionCallback& callback) {
   DCHECK(socket_handle);
+
+  RequestSocketForPool socketRequest(socket_handle, callback, net_log);
+
   // Synthesize an HttpRequestInfo.
   GURL request_url = GURL("http://" + host_port_pair.ToString());
   HttpRequestHeaders request_extra_headers;
   int request_load_flags = 0;
   RequestPriority request_priority = MEDIUM;
 
-  return InitSocketPoolHelper(
+  return InvokeWithSocketParamsAndPool(
       request_url, request_extra_headers, request_load_flags, request_priority,
-      session, proxy_info, false, false, ssl_config_for_origin,
-      ssl_config_for_proxy, true, net_log, 0, socket_handle,
-      OnHostResolutionCallback(), callback);
+      session, proxy_info, false, false,
+      ssl_config_for_origin, ssl_config_for_proxy, true,
+      OnHostResolutionCallback(), socketRequest);
 }
 
 int PreconnectSocketsForHttpRequest(
     const GURL& request_url,
     const HttpRequestHeaders& request_extra_headers,
     int request_load_flags,
     RequestPriority request_priority,
     HttpNetworkSession* session,
     const ProxyInfo& proxy_info,
     bool force_spdy_over_ssl,
     bool want_spdy_over_npn,
     const SSLConfig& ssl_config_for_origin,
     const SSLConfig& ssl_config_for_proxy,
     const BoundNetLog& net_log,
     int num_preconnect_streams) {
-  return InitSocketPoolHelper(
+  RequestSocketsForPool socketsRequest(num_preconnect_streams, net_log);
+
+  return InvokeWithSocketParamsAndPool(
       request_url, request_extra_headers, request_load_flags, request_priority,
       session, proxy_info, force_spdy_over_ssl, want_spdy_over_npn,
-      ssl_config_for_origin, ssl_config_for_proxy, false, net_log,
-      num_preconnect_streams, NULL, OnHostResolutionCallback(),
-      CompletionCallback());
+      ssl_config_for_origin, ssl_config_for_proxy, false,
+      OnHostResolutionCallback(), socketsRequest);
 }
 
 }  // namespace net