SSL Connect Job Waiting

net/socket/ssl_client_socket_pool.h

Index: net/socket/ssl_client_socket_pool.h
diff --git a/net/socket/ssl_client_socket_pool.h b/net/socket/ssl_client_socket_pool.h
index e03b76ade6ab5946e2d981e7d7802b1cb70d6dba..addf4106332d4a3b9df827be4b25b481531fe204 100644
--- a/net/socket/ssl_client_socket_pool.h
+++ b/net/socket/ssl_client_socket_pool.h
@@ -5,7 +5,9 @@
 #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"
@@ -98,19 +100,25 @@ class NET_EXPORT_PRIVATE SSLSocketParams
 // 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.
@@ -118,6 +126,11 @@ class SSLConnectJob : public ConnectJob {
 
   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,
@@ -126,6 +139,8 @@ class SSLConnectJob : public ConnectJob {
     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,
@@ -142,9 +157,20 @@ class SSLConnectJob : public ConnectJob {
   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);
@@ -164,10 +190,12 @@ class SSLConnectJob : public ConnectJob {
   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);
@@ -284,6 +312,10 @@ class NET_EXPORT_PRIVATE SSLClientSocketPool
     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_;
@@ -292,6 +324,7 @@ class NET_EXPORT_PRIVATE SSLClientSocketPool
     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);
   };