SSL Connect Job Waiting

net/socket/ssl_client_socket_pool.h

 // 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.
 
 #ifndef NET_SOCKET_SSL_CLIENT_SOCKET_POOL_H_
 #define NET_SOCKET_SSL_CLIENT_SOCKET_POOL_H_
 
+#include <map>
 #include <string>
+#include <vector>
 
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/time/time.h"
 #include "net/base/privacy_mode.h"
 #include "net/dns/host_resolver.h"
 #include "net/http/http_response_info.h"
 #include "net/socket/client_socket_pool.h"
 #include "net/socket/client_socket_pool_base.h"
 #include "net/socket/client_socket_pool_histograms.h"
@@ skipping 72 matching lines @@
   const bool want_spdy_over_npn_;
   bool ignore_limits_;
 
   DISALLOW_COPY_AND_ASSIGN(SSLSocketParams);
 };
 
 // SSLConnectJob handles the SSL handshake after setting up the underlying
 // connection as specified in the params.
 class SSLConnectJob : public ConnectJob {
  public:
-  SSLConnectJob(
-      const std::string& group_name,
-      RequestPriority priority,
-      const scoped_refptr<SSLSocketParams>& params,
-      const base::TimeDelta& timeout_duration,
-      TransportClientSocketPool* transport_pool,
-      SOCKSClientSocketPool* socks_pool,
-      HttpProxyClientSocketPool* http_proxy_pool,
-      ClientSocketFactory* client_socket_factory,
-      HostResolver* host_resolver,
-      const SSLClientSocketContext& context,
-      Delegate* delegate,
-      NetLog* net_log);
+  // List of SSLConnectJobs that are either in the process of connecting
+  // to or waiting to connect to the same server.
+  typedef std::vector<SSLConnectJob*> PendingJobList;
+
+  // Note: the SSLConnectJob does not own pending_jobs_list
+  // so it must outlive the job.

[Ryan Sleevi, 2014/06/24 23:40:44]

I think the "Note:" here is probably best omitted.

This is true of all the other pointer-variables passed here (eg: no ownership
changes happen; the owner must ensure the object outlives the job).

[mshelley1, 2014/06/25 17:03:56]

Done.

+  SSLConnectJob(const std::string& group_name,
+                RequestPriority priority,
+                const scoped_refptr<SSLSocketParams>& params,
+                const base::TimeDelta& timeout_duration,
+                TransportClientSocketPool* transport_pool,
+                SOCKSClientSocketPool* socks_pool,
+                HttpProxyClientSocketPool* http_proxy_pool,
+                ClientSocketFactory* client_socket_factory,
+                HostResolver* host_resolver,
+                const SSLClientSocketContext& context,
+                PendingJobList* pending_jobs_list,
+                Delegate* delegate,
+                NetLog* net_log);
   virtual ~SSLConnectJob();
 
   // ConnectJob methods.
   virtual LoadState GetLoadState() const OVERRIDE;
 
   virtual void GetAdditionalErrorState(ClientSocketHandle * handle) OVERRIDE;
 
+  // Enable SSLConnectJob waiting if |enable| is true.

[Ryan Sleevi, 2014/06/24 23:40:44]

// Enables or disables SSLConnectJob waiting.
// If waiting is enabled, simultaneous attempts to connect
// to the same server will be serialized until an SSL session
// is cached, allowing the remaining connections to perform
// resumption handshakes.

That is, provide some description of what "job waiting" is; not too much, not
too little :)

[mshelley1, 2014/06/25 17:03:56]

Done.

+  static NET_EXPORT void EnableJobWaiting(bool enable);
+
+  static bool GetEnableJobWaiting();
+
  private:
   enum State {
     STATE_TRANSPORT_CONNECT,
     STATE_TRANSPORT_CONNECT_COMPLETE,
     STATE_SOCKS_CONNECT,
     STATE_SOCKS_CONNECT_COMPLETE,
     STATE_TUNNEL_CONNECT,
     STATE_TUNNEL_CONNECT_COMPLETE,
+    STATE_CREATE_SSL_SOCKET,
+    STATE_CHECK_FOR_RESUME,
     STATE_SSL_CONNECT,
     STATE_SSL_CONNECT_COMPLETE,
     STATE_NONE,
   };
 
   void OnIOComplete(int result);
 
   // Runs the state transition loop.
   int DoLoop(int result);
 
   int DoTransportConnect();
   int DoTransportConnectComplete(int result);
   int DoSOCKSConnect();
   int DoSOCKSConnectComplete(int result);
   int DoTunnelConnect();
   int DoTunnelConnectComplete(int result);
+  int DoCreateSSLSocket();
+  int DoCheckForResume();
   int DoSSLConnect();
   int DoSSLConnectComplete(int result);
 
+  // Decides how pending jobs should continue their connection
+  // based upon the success or failure of the leading job.
+  void ProcessPendingJobs(int result);
+
+  // Tells a waiting SSLConnectJob to resume its SSL connection.
+  void ResumeSSLConnection();
+
+  static bool enable_job_waiting_;
+
   // Returns the initial state for the state machine based on the
   // |connection_type|.
   static State GetInitialState(SSLSocketParams::ConnectionType connection_type);
 
   // Starts the SSL connection process.  Returns OK on success and
   // ERR_IO_PENDING if it cannot immediately service the request.
   // Otherwise, it returns a net error code.
   virtual int ConnectInternal() OVERRIDE;
 
   scoped_refptr<SSLSocketParams> params_;
   TransportClientSocketPool* const transport_pool_;
   SOCKSClientSocketPool* const socks_pool_;
   HttpProxyClientSocketPool* const http_proxy_pool_;
   ClientSocketFactory* const client_socket_factory_;
   HostResolver* const host_resolver_;
 
   const SSLClientSocketContext context_;
 
   State next_state_;
-  CompletionCallback callback_;
+  CompletionCallback io_callback_;
   scoped_ptr<ClientSocketHandle> transport_socket_handle_;
   scoped_ptr<SSLClientSocket> ssl_socket_;
 
+  // TODO(mshelley) ensure that these pointers are deleted.

[Ryan Sleevi, 2014/06/24 23:40:44]

Is this TODO still applicable?

[mshelley1, 2014/06/25 17:03:56]

Yes -- I need to make sure that the pending jobs inside of the pending_jobs_map
are delted. Wan-teh suggested making that another CL.

+  PendingJobList* pending_jobs_;
   HttpResponseInfo error_response_info_;
 
   DISALLOW_COPY_AND_ASSIGN(SSLConnectJob);
 };
 
 class NET_EXPORT_PRIVATE SSLClientSocketPool
     : public ClientSocketPool,
       public HigherLayeredPool,
       public SSLConfigService::Observer {
  public:
@@ skipping 96 matching lines @@
 
     // ClientSocketPoolBase::ConnectJobFactory methods.
     virtual scoped_ptr<ConnectJob> NewConnectJob(
         const std::string& group_name,
         const PoolBase::Request& request,
         ConnectJob::Delegate* delegate) const OVERRIDE;
 
     virtual base::TimeDelta ConnectionTimeout() const OVERRIDE;
 
    private:
+    // Maps SSLConnectJob group names to lists of pending connect jobs to
+    // that group name.
+    typedef std::map<std::string, SSLConnectJob::PendingJobList*> PendingJobMap;
+
     TransportClientSocketPool* const transport_pool_;
     SOCKSClientSocketPool* const socks_pool_;
     HttpProxyClientSocketPool* const http_proxy_pool_;
     ClientSocketFactory* const client_socket_factory_;
     HostResolver* const host_resolver_;
     const SSLClientSocketContext context_;
     base::TimeDelta timeout_;
     NetLog* net_log_;
+    scoped_ptr<PendingJobMap> pending_jobs_map_;

[Ryan Sleevi, 2014/06/24 23:40:44]

Why can't this just be a PendingJobsMap?

That is, why a pointer at all?

[mshelley1, 2014/06/25 17:03:56]

If this is just a PendingJobsMap then I can't insert into the map inside of the
const NewConnectJob method (that was my understanding at least). Removing const
from the method impacts several classes that are derived from the class, so I
figured this was a better solution. 

[Ryan Sleevi, 2014/06/25 19:13:21]

Ah, I missed that.

Yeah, it's definitely weird, and strictly speaking, a violation of
const-correctness, but one that's subtle and easy to make (e.g.: This class
already does a lot with violating const-correctness by virtue of the pools)

In an "ideal" world (one few people will ever see in reality), when a method is
declared const, it won't mutate any state on the object - OR any state on any of
the sub-objects it holds.

This is enforced somewhat when you have member variables (as you discovered),
but not when the member variable is a pointer (*shrug*).

I think this is probably fine for the short-term, but let's add a TODO here to
come up with a better solution (or perhaps it may change entirely)

 
     DISALLOW_COPY_AND_ASSIGN(SSLConnectJobFactory);
   };
 
   TransportClientSocketPool* const transport_pool_;
   SOCKSClientSocketPool* const socks_pool_;
   HttpProxyClientSocketPool* const http_proxy_pool_;
   PoolBase base_;
   const scoped_refptr<SSLConfigService> ssl_config_service_;
 
   DISALLOW_COPY_AND_ASSIGN(SSLClientSocketPool);
 };
 
 }  // namespace net
 
 #endif  // NET_SOCKET_SSL_CLIENT_SOCKET_POOL_H_