Create WebSocketTransportClientSocketPool

net/socket/transport_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_TRANSPORT_CLIENT_SOCKET_POOL_H_
 #define NET_SOCKET_TRANSPORT_CLIENT_SOCKET_POOL_H_
 
 #include <string>
 
 #include "base/basictypes.h"
@@ skipping 37 matching lines @@
   friend class base::RefCounted<TransportSocketParams>;
   ~TransportSocketParams();
 
   HostResolver::RequestInfo destination_;
   bool ignore_limits_;
   const OnHostResolutionCallback host_resolution_callback_;
 
   DISALLOW_COPY_AND_ASSIGN(TransportSocketParams);
 };
 
+// Common data and logic shared between TransportConnectJob and
+// WebSocketTransportConnectJob.
+class NET_EXPORT_PRIVATE TransportConnectJobHelper {
+ public:
+  enum State {
+    STATE_RESOLVE_HOST,
+    STATE_RESOLVE_HOST_COMPLETE,
+    STATE_TRANSPORT_CONNECT,
+    STATE_TRANSPORT_CONNECT_COMPLETE,
+    STATE_NONE,
+  };
+
+  // For recording the connection time in the appropriate bucket.
+  enum ConnectionLatencyHistogram {
+    CONNECTION_LATENCY_UNKNOWN,
+    CONNECTION_LATENCY_IPV4_WINS_RACE,
+    CONNECTION_LATENCY_IPV4_NO_RACE,
+    CONNECTION_LATENCY_IPV6_RACEABLE,
+    CONNECTION_LATENCY_IPV6_SOLO,
+  };
+
+  TransportConnectJobHelper(const scoped_refptr<TransportSocketParams>& params,
+                            ClientSocketFactory* client_socket_factory,
+                            HostResolver* host_resolver,
+                            LoadTimingInfo::ConnectTiming* connect_timing);
+  ~TransportConnectJobHelper();
+
+  ClientSocketFactory* client_socket_factory() {
+    return client_socket_factory_;
+  }
+

[Ryan Sleevi, 2014/07/08 18:50:02]

This is a huge set of methods to leave public, for any #include'r of
TransportClientSocketPool.

This seems to break multiple layers of encapsulation/isolation - e.g. - these
details are not exposed to holders of TransportConnectJobs normally.

[tyoshino (SeeGerritForStatus), 2014/07/09 00:17:40]

Sorry about this. I saw use of ClientSocketPoolBaseHelper and was taking things
easy too much.

Basically they're for reducing code duplication. We should be able to put them
into each pool implementation as a quick fix while we should also try to come up
with better design with less code duplication.

Also, shall we put this into an internal namespace as well as
ClientSocketPoolBaseHelper?

[Ryan Sleevi, 2014/07/09 00:20:46]

Right, CSPBH is to be a base class for a set of templated classes, but avoid the
overhead of instantiating all of the common methods on the templates.

That is, it's a performance optimization, more than a code/dependency
optimization.
Right, I think we're on the same page that we want to minimize code duplication
(through template instantiation) and code duplication (through
implementation/copy paste). We just want to make sure none of that
de-duplication leaks out into the interfaces, nor does it violate the layering.
Yes, this is something we'd minimally mark internal

+  const AddressList& addresses() const { return addresses_; }
+  State next_state() const { return next_state_; }
+  void set_next_state(State next_state) { next_state_ = next_state; }
+  CompletionCallback on_io_complete() const { return on_io_complete_; }
+
+  int DoResolveHost(RequestPriority priority, const BoundNetLog& net_log);
+  int DoResolveHostComplete(int result, const BoundNetLog& net_log);
+
+  template <class T>
+  int DoConnectInternal(T* job);

[Ryan Sleevi, 2014/07/08 18:50:02]

These methods are public.

Anyone with a helper can use it to fully mutate the state of an Job. That's
definitely a layering violation.

[Adam Rice, 2014/07/09 08:33:02]

Sorry, yes, when I wrote this I was just focussed on factoring out the common
code in the simplest way possible. I was expecting to get some push-back that
would ultimately lead to a better solution, so I wasn't paying attention to
encapsulation.

But it's easy to miss things in a big CL, especially one with parts that are
ugly-as-designed, so this ended-up getting checked in as-is.

+
+  template <class T>
+  void SetOnIOComplete(T* job);
+
+  template <class T>
+  void OnIOComplete(T* job, int result);

[Ryan Sleevi, 2014/07/08 18:50:02]

Template methods like this are almost always a bad thing - they accept any type
and can be used to mutate the internal state.

+
+  // Record the histograms Net.DNS_Resolution_And_TCP_Connection_Latency2 and
+  // Net.TCP_Connection_Latency and return the connect duration.
+  base::TimeDelta HistogramDuration(ConnectionLatencyHistogram race_result);
+
+  static const int kIPv6FallbackTimerInMs;
+
+ private:
+  template <class T>
+  int DoLoop(T* job, int result);
+
+  scoped_refptr<TransportSocketParams> params_;
+  ClientSocketFactory* const client_socket_factory_;
+  SingleRequestHostResolver resolver_;
+  AddressList addresses_;
+  State next_state_;
+  CompletionCallback on_io_complete_;
+  LoadTimingInfo::ConnectTiming* connect_timing_;
+
+  DISALLOW_COPY_AND_ASSIGN(TransportConnectJobHelper);
+};
+
 // TransportConnectJob handles the host resolution necessary for socket creation
 // and the transport (likely TCP) connect. TransportConnectJob also has fallback
 // logic for IPv6 connect() timeouts (which may happen due to networks / routers
 // with broken IPv6 support). Those timeouts take 20s, so rather than make the
 // user wait 20s for the timeout to fire, we use a fallback timer
 // (kIPv6FallbackTimerInMs) and start a connect() to a IPv4 address if the timer
 // fires. Then we race the IPv4 connect() against the IPv6 connect() (which has
 // a headstart) and return the one that completes first to the socket pool.
 class NET_EXPORT_PRIVATE TransportConnectJob : public ConnectJob {
  public:
   TransportConnectJob(const std::string& group_name,
                       RequestPriority priority,
                       const scoped_refptr<TransportSocketParams>& params,
                       base::TimeDelta timeout_duration,
                       ClientSocketFactory* client_socket_factory,
                       HostResolver* host_resolver,
                       Delegate* delegate,
                       NetLog* net_log);
   virtual ~TransportConnectJob();
 
   // ConnectJob methods.
   virtual LoadState GetLoadState() const OVERRIDE;
 
   // Rolls |addrlist| forward until the first IPv4 address, if any.
   // WARNING: this method should only be used to implement the prefer-IPv4 hack.
   static void MakeAddressListStartWithIPv4(AddressList* addrlist);
 
-  static const int kIPv6FallbackTimerInMs;
-
  private:
-  enum State {
-    STATE_RESOLVE_HOST,
-    STATE_RESOLVE_HOST_COMPLETE,
-    STATE_TRANSPORT_CONNECT,
-    STATE_TRANSPORT_CONNECT_COMPLETE,
-    STATE_NONE,
-  };
-
   enum ConnectInterval {
     CONNECT_INTERVAL_LE_10MS,
     CONNECT_INTERVAL_LE_20MS,
     CONNECT_INTERVAL_GT_20MS,
   };
 
-  void OnIOComplete(int result);
-
-  // Runs the state transition loop.
-  int DoLoop(int result);
+  friend class TransportConnectJobHelper;

[Ryan Sleevi, 2014/07/08 18:50:02]

Every time we have a friend class, we have failed encapsulation, and it should
only be done with great pain/suffering/introspection.

I'd like to work to explore alternate designs that actually maintain a
well-encapsulated boundary.

 
   int DoResolveHost();
   int DoResolveHostComplete(int result);
   int DoTransportConnect();
   int DoTransportConnectComplete(int result);
 
   // Not part of the state machine.
   void DoIPv6FallbackTransportConnect();
   void DoIPv6FallbackTransportConnectComplete(int result);
 
   // Begins the host resolution and the TCP connect.  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<TransportSocketParams> params_;
-  ClientSocketFactory* const client_socket_factory_;
-  SingleRequestHostResolver resolver_;
-  AddressList addresses_;
-  State next_state_;
+  TransportConnectJobHelper helper_;
 
   scoped_ptr<StreamSocket> transport_socket_;
 
   scoped_ptr<StreamSocket> fallback_transport_socket_;
   scoped_ptr<AddressList> fallback_addresses_;
   base::TimeTicks fallback_connect_start_time_;
   base::OneShotTimer<TransportConnectJob> fallback_timer_;
 
   // Track the interval between this connect and previous connect.
   ConnectInterval interval_between_connects_;
@@ skipping 44 matching lines @@
       const std::string& type,
       bool include_nested_pools) const OVERRIDE;
   virtual base::TimeDelta ConnectionTimeout() const OVERRIDE;
   virtual ClientSocketPoolHistograms* histograms() const OVERRIDE;
 
   // HigherLayeredPool implementation.
   virtual bool IsStalled() const OVERRIDE;
   virtual void AddHigherLayeredPool(HigherLayeredPool* higher_pool) OVERRIDE;
   virtual void RemoveHigherLayeredPool(HigherLayeredPool* higher_pool) OVERRIDE;
 
+ protected:

[Ryan Sleevi, 2014/07/08 18:50:02]

The Chromium style guide strongly discourages protected methods 

+  // Methods shared with WebSocketTransportClientSocketPool
+  void NetLogTcpClientSocketPoolRequestedSocket(
+      const BoundNetLog& net_log,
+      const scoped_refptr<TransportSocketParams>* casted_params);

[Ryan Sleevi, 2014/07/08 18:50:02]

This doesn't look right at all. A pointer to a const scoped-refptr?

[Adam Rice, 2014/07/09 08:33:02]

Whereas the void* argument to RequestSocket() is totally cool?

+
  private:
   typedef ClientSocketPoolBase<TransportSocketParams> PoolBase;
 
   class TransportConnectJobFactory
       : public PoolBase::ConnectJobFactory {
    public:
     TransportConnectJobFactory(ClientSocketFactory* client_socket_factory,
                          HostResolver* host_resolver,
                          NetLog* net_log)
         : client_socket_factory_(client_socket_factory),
@@ skipping 17 matching lines @@
     NetLog* net_log_;
 
     DISALLOW_COPY_AND_ASSIGN(TransportConnectJobFactory);
   };
 
   PoolBase base_;
 
   DISALLOW_COPY_AND_ASSIGN(TransportClientSocketPool);
 };
 
+template <class T>
+int TransportConnectJobHelper::DoConnectInternal(T* job) {
+  next_state_ = STATE_RESOLVE_HOST;
+  return this->DoLoop(job, OK);
+}
+
+template <class T>
+void TransportConnectJobHelper::SetOnIOComplete(T* job) {
+  // These usages of base::Unretained() are safe because IO callbacks are
+  // guaranteed not to be called after the object is destroyed.
+  on_io_complete_ = base::Bind(&TransportConnectJobHelper::OnIOComplete<T>,
+                               base::Unretained(this),
+                               base::Unretained(job));
+}
+
+template <class T>
+void TransportConnectJobHelper::OnIOComplete(T* job, int result) {
+  result = this->DoLoop(job, result);
+  if (result != ERR_IO_PENDING)
+    job->NotifyDelegateOfCompletion(result);  // Deletes |job| and |this|
+}
+
+template <class T>
+int TransportConnectJobHelper::DoLoop(T* job, int result) {
+  DCHECK_NE(next_state_, STATE_NONE);
+
+  int rv = result;
+  do {
+    State state = next_state_;
+    next_state_ = STATE_NONE;
+    switch (state) {
+      case STATE_RESOLVE_HOST:
+        DCHECK_EQ(OK, rv);
+        rv = job->DoResolveHost();
+        break;
+      case STATE_RESOLVE_HOST_COMPLETE:
+        rv = job->DoResolveHostComplete(rv);
+        break;
+      case STATE_TRANSPORT_CONNECT:
+        DCHECK_EQ(OK, rv);
+        rv = job->DoTransportConnect();
+        break;
+      case STATE_TRANSPORT_CONNECT_COMPLETE:
+        rv = job->DoTransportConnectComplete(rv);
+        break;
+      default:
+        NOTREACHED();
+        rv = ERR_FAILED;
+        break;
+    }
+  } while (rv != ERR_IO_PENDING && next_state_ != STATE_NONE);
+
+  return rv;
+}
+
 }  // namespace net
 
 #endif  // NET_SOCKET_TRANSPORT_CLIENT_SOCKET_POOL_H_