okhttp: Pass TransportFactory directly to transport constructor

This greatly reduces the number of arguments passed to the constructor
and allows using the builder in tests to change specific arguments
without having to pass all the other arguments. It also makes it easier
to see where tests are doing something special.

While it is weird to expose fields as package-private for digging-into
in the constructor, it's actually very similar to the pattern of passing
the builder instance into the constuctor. In this case, the weirdness is
because the builder isn't a nested class of the transport and there is
an additional level of building going on (Builder and TransportFactory).
We do this pattern already in ManagedChannelImpl which only has the one
level of building.

okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java

@@ -505,7 +505,7 @@ public OkHttpChannelBuilder maxInboundMessageSize(int max) {
     return this;
   }
 
-  ClientTransportFactory buildTransportFactory() {
+  OkHttpTransportFactory buildTransportFactory() {
     boolean enableKeepAlive = keepAliveTimeNanos != KEEPALIVE_TIME_NANOS_DISABLED;
     return new OkHttpTransportFactory(
         transportExecutor,
@@ -712,25 +712,25 @@ public SslSocketFactoryResult withCallCredentials(CallCredentials callCreds) {
    */
   @Internal
   static final class OkHttpTransportFactory implements ClientTransportFactory {
-    private final Executor executor;
+    final Executor executor;
     private final boolean usingSharedExecutor;
     private final boolean usingSharedScheduler;
-    private final TransportTracer.Factory transportTracerFactory;
-    private final SocketFactory socketFactory;
-    @Nullable private final SSLSocketFactory sslSocketFactory;
+    final TransportTracer.Factory transportTracerFactory;
+    final SocketFactory socketFactory;
+    @Nullable final SSLSocketFactory sslSocketFactory;
     @Nullable
-    private final HostnameVerifier hostnameVerifier;
-    private final ConnectionSpec connectionSpec;
-    private final int maxMessageSize;
+    final HostnameVerifier hostnameVerifier;
+    final ConnectionSpec connectionSpec;
+    final int maxMessageSize;
     private final boolean enableKeepAlive;
     private final long keepAliveTimeNanos;
     private final AtomicBackoff keepAliveBackoff;
     private final long keepAliveTimeoutNanos;
-    private final int flowControlWindow;
+    final int flowControlWindow;
     private final boolean keepAliveWithoutCalls;
-    private final int maxInboundMetadataSize;
+    final int maxInboundMetadataSize;
     private final ScheduledExecutorService timeoutService;
-    private final boolean useGetForSafeMethods;
+    final boolean useGetForSafeMethods;
     private boolean closed;
 
     private OkHttpTransportFactory(
@@ -793,22 +793,13 @@ public void run() {
       InetSocketAddress inetSocketAddr = (InetSocketAddress) addr;
       // TODO(carl-mastrangelo): Pass channelLogger in.
       OkHttpClientTransport transport = new OkHttpClientTransport(
+          this,
           inetSocketAddr,
           options.getAuthority(),
           options.getUserAgent(),
           options.getEagAttributes(),
-          executor,
-          socketFactory,
-          sslSocketFactory,
-          hostnameVerifier,
-          connectionSpec,
-          maxMessageSize,
-          flowControlWindow,
           options.getHttpConnectProxiedSocketAddress(),
-          tooManyPingsRunnable,
-          maxInboundMetadataSize,
-          transportTracerFactory.create(),
-          useGetForSafeMethods);
+          tooManyPingsRunnable);
       if (enableKeepAlive) {
         transport.enableKeepAlive(
             true, keepAliveTimeNanosState.get(), keepAliveTimeoutNanos, keepAliveWithoutCalls);

okhttp/src/main/java/io/grpc/okhttp/OkHttpClientTransport.java

@@ -227,87 +227,62 @@ protected void handleNotInUse() {
   SettableFuture<Void> connectedFuture;
 
   public OkHttpClientTransport(
+      OkHttpChannelBuilder.OkHttpTransportFactory transportFactory,
       InetSocketAddress address,
       String authority,
       @Nullable String userAgent,
       Attributes eagAttrs,
-      Executor executor,
-      @Nullable SocketFactory socketFactory,
-      @Nullable SSLSocketFactory sslSocketFactory,
-      @Nullable HostnameVerifier hostnameVerifier,
-      ConnectionSpec connectionSpec,
-      int maxMessageSize,
-      int initialWindowSize,
       @Nullable HttpConnectProxiedSocketAddress proxiedAddr,
-      Runnable tooManyPingsRunnable,
-      int maxInboundMetadataSize,
-      TransportTracer transportTracer,
-      boolean useGetForSafeMethods) {
+      Runnable tooManyPingsRunnable) {
     this(
+        transportFactory,
         address,
         authority,
         userAgent,
         eagAttrs,
-        executor,
-        socketFactory,
-        sslSocketFactory,
-        hostnameVerifier,
-        connectionSpec,
         GrpcUtil.STOPWATCH_SUPPLIER,
         new Http2(),
-        maxMessageSize,
-        initialWindowSize,
         proxiedAddr,
-        tooManyPingsRunnable,
-        maxInboundMetadataSize,
-        transportTracer,
-        useGetForSafeMethods);
+        tooManyPingsRunnable);
   }
 
   private OkHttpClientTransport(
+      OkHttpChannelBuilder.OkHttpTransportFactory transportFactory,
       InetSocketAddress address,
       String authority,
       @Nullable String userAgent,
       Attributes eagAttrs,
-      Executor executor,
-      @Nullable SocketFactory socketFactory,
-      @Nullable SSLSocketFactory sslSocketFactory,
-      @Nullable HostnameVerifier hostnameVerifier,
-      ConnectionSpec connectionSpec,
       Supplier<Stopwatch> stopwatchFactory,
       Variant variant,
-      int maxMessageSize,
-      int initialWindowSize,
       @Nullable HttpConnectProxiedSocketAddress proxiedAddr,
-      Runnable tooManyPingsRunnable,
-      int maxInboundMetadataSize,
-      TransportTracer transportTracer,
-      boolean useGetForSafeMethods) {
+      Runnable tooManyPingsRunnable) {
     this.address = Preconditions.checkNotNull(address, "address");
     this.defaultAuthority = authority;
-    this.maxMessageSize = maxMessageSize;
-    this.initialWindowSize = initialWindowSize;
-    this.executor = Preconditions.checkNotNull(executor, "executor");
-    serializingExecutor = new SerializingExecutor(executor);
+    this.maxMessageSize = transportFactory.maxMessageSize;
+    this.initialWindowSize = transportFactory.flowControlWindow;
+    this.executor = Preconditions.checkNotNull(transportFactory.executor, "executor");
+    serializingExecutor = new SerializingExecutor(transportFactory.executor);
     // Client initiated streams are odd, server initiated ones are even. Server should not need to
     // use it. We start clients at 3 to avoid conflicting with HTTP negotiation.
     nextStreamId = 3;
-    this.socketFactory = socketFactory == null ? SocketFactory.getDefault() : socketFactory;
-    this.sslSocketFactory = sslSocketFactory;
-    this.hostnameVerifier = hostnameVerifier;
-    this.connectionSpec = Preconditions.checkNotNull(connectionSpec, "connectionSpec");
+    this.socketFactory = transportFactory.socketFactory == null
+        ? SocketFactory.getDefault() : transportFactory.socketFactory;
+    this.sslSocketFactory = transportFactory.sslSocketFactory;
+    this.hostnameVerifier = transportFactory.hostnameVerifier;
+    this.connectionSpec = Preconditions.checkNotNull(
+        transportFactory.connectionSpec, "connectionSpec");
     this.stopwatchFactory = Preconditions.checkNotNull(stopwatchFactory, "stopwatchFactory");
     this.variant = Preconditions.checkNotNull(variant, "variant");
     this.userAgent = GrpcUtil.getGrpcUserAgent("okhttp", userAgent);
     this.proxiedAddr = proxiedAddr;
     this.tooManyPingsRunnable =
         Preconditions.checkNotNull(tooManyPingsRunnable, "tooManyPingsRunnable");
-    this.maxInboundMetadataSize = maxInboundMetadataSize;
-    this.transportTracer = Preconditions.checkNotNull(transportTracer);
+    this.maxInboundMetadataSize = transportFactory.maxInboundMetadataSize;
+    this.transportTracer = transportFactory.transportTracerFactory.create();
     this.logId = InternalLogId.allocate(getClass(), address.toString());
     this.attributes = Attributes.newBuilder()
         .set(GrpcAttributes.ATTR_CLIENT_EAG_ATTRS, eagAttrs).build();
-    this.useGetForSafeMethods = useGetForSafeMethods;
+    this.useGetForSafeMethods = transportFactory.useGetForSafeMethods;
     initTransportTracer();
   }
 
@@ -316,36 +291,23 @@ private OkHttpClientTransport(
    */
   @VisibleForTesting
   OkHttpClientTransport(
+      OkHttpChannelBuilder.OkHttpTransportFactory transportFactory,
       String userAgent,
-      Executor executor,
-      @Nullable SocketFactory socketFactory,
       Supplier<Stopwatch> stopwatchFactory,
       Variant variant,
       @Nullable Runnable connectingCallback,
       SettableFuture<Void> connectedFuture,
-      int maxMessageSize,
-      int initialWindowSize,
-      Runnable tooManyPingsRunnable,
-      TransportTracer transportTracer) {
+      Runnable tooManyPingsRunnable) {
     this(
+        transportFactory,
         new InetSocketAddress("127.0.0.1", 80),
         "notarealauthority:80",
         userAgent,
         Attributes.EMPTY,
-        executor,
-        socketFactory,
-        null,
-        null,
-        OkHttpChannelBuilder.INTERNAL_DEFAULT_CONNECTION_SPEC,
         stopwatchFactory,
         variant,
-        maxMessageSize,
-        initialWindowSize,
         null,
-        tooManyPingsRunnable,
-        Integer.MAX_VALUE,
-        transportTracer,
-        false);
+        tooManyPingsRunnable);
     this.connectingCallback = connectingCallback;
     this.connectedFuture = Preconditions.checkNotNull(connectedFuture, "connectedFuture");
   }