From cecbd5eb3c372e154d924d88248477a09367aa35 Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Thu, 13 Aug 2020 17:18:17 -0400 Subject: [PATCH 01/54] WIP --- .../internal/ManagedChannelImplBuilder.java | 25 +++++++++++++++++++ .../io/grpc/okhttp/OkHttpChannelBuilder.java | 20 ++++++++++----- 2 files changed, 39 insertions(+), 6 deletions(-) create mode 100644 core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java new file mode 100644 index 00000000000..a3d34906e9f --- /dev/null +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java @@ -0,0 +1,25 @@ +package io.grpc.internal; + +public class ManagedChannelImplBuilder extends AbstractManagedChannelImplBuilder { + private final TransportFactoryBuilder transportFactoryBuilder; + + public interface TransportFactoryBuilder { + ClientTransportFactory buildTransportFactory(); + } + + protected ManagedChannelImplBuilder(String target, + TransportFactoryBuilder transportFactoryBuilder) { + super(target); + this.transportFactoryBuilder = transportFactoryBuilder; + } + + public static ManagedChannelImplBuilder forTarget(String target, + TransportFactoryBuilder transportFactoryBuilder) { + return new ManagedChannelImplBuilder(target, transportFactoryBuilder); + } + + @Override + protected ClientTransportFactory buildTransportFactory() { + return transportFactoryBuilder.buildTransportFactory(); + } +} \ No newline at end of file diff --git a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java index 26a76f2c8c4..56f247b503d 100644 --- a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java +++ b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java @@ -24,13 +24,14 @@ import com.google.common.base.Preconditions; import io.grpc.ChannelLogger; import io.grpc.ExperimentalApi; +import io.grpc.ForwardingChannelBuilder; import io.grpc.Internal; -import io.grpc.internal.AbstractManagedChannelImplBuilder; import io.grpc.internal.AtomicBackoff; import io.grpc.internal.ClientTransportFactory; import io.grpc.internal.ConnectionClientTransport; import io.grpc.internal.GrpcUtil; import io.grpc.internal.KeepAliveManager; +import io.grpc.internal.ManagedChannelImplBuilder; import io.grpc.internal.SharedResourceHolder; import io.grpc.internal.SharedResourceHolder.Resource; import io.grpc.internal.TransportTracer; @@ -55,9 +56,11 @@ /** Convenience class for building channels with the OkHttp transport. */ @ExperimentalApi("https://github.com/grpc/grpc-java/issues/1785") public class OkHttpChannelBuilder extends - AbstractManagedChannelImplBuilder { + ForwardingChannelBuilder { public static final int DEFAULT_FLOW_CONTROL_WINDOW = 65535; + private final ManagedChannelImplBuilder managedChannelImplBuilder; + private TransportTracer.Factory transportTracerFactory = TransportTracer.getDefaultFactory(); /** Identifies the negotiation used for starting up HTTP/2. */ private enum NegotiationType { @@ -115,6 +118,11 @@ public static OkHttpChannelBuilder forTarget(String target) { return new OkHttpChannelBuilder(target); } + @Override + protected ManagedChannelImplBuilder delegate() { + return managedChannelImplBuilder; + } + private Executor transportExecutor; private ScheduledExecutorService scheduledExecutorService; @@ -140,7 +148,9 @@ protected OkHttpChannelBuilder(String host, int port) { } private OkHttpChannelBuilder(String target) { - super(target); + super(); + managedChannelImplBuilder = ManagedChannelImplBuilder.forTarget(target, + this::buildTransportFactory); } @VisibleForTesting @@ -363,9 +373,7 @@ public OkHttpChannelBuilder maxInboundMetadataSize(int bytes) { return this; } - @Override - @Internal - protected final ClientTransportFactory buildTransportFactory() { + private final ClientTransportFactory buildTransportFactory() { boolean enableKeepAlive = keepAliveTimeNanos != KEEPALIVE_TIME_NANOS_DISABLED; return new OkHttpTransportFactory( transportExecutor, From efef6509a4fd2d33fa6d4dd95c838dce860a92ed Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Thu, 13 Aug 2020 20:16:37 -0400 Subject: [PATCH 02/54] Figure out default port --- .../java/io/grpc/SimpleForwardingChannelBuilder.java | 7 +++++++ .../io/grpc/internal/ManagedChannelImplBuilder.java | 11 +++++++++++ .../java/io/grpc/okhttp/OkHttpChannelBuilder.java | 12 +++++++++--- 3 files changed, 27 insertions(+), 3 deletions(-) create mode 100644 api/src/main/java/io/grpc/SimpleForwardingChannelBuilder.java diff --git a/api/src/main/java/io/grpc/SimpleForwardingChannelBuilder.java b/api/src/main/java/io/grpc/SimpleForwardingChannelBuilder.java new file mode 100644 index 00000000000..9a071faeeae --- /dev/null +++ b/api/src/main/java/io/grpc/SimpleForwardingChannelBuilder.java @@ -0,0 +1,7 @@ +package io.grpc; + +import io.grpc.internal.AbstractManagedChannelImplBuilder; + +public abstract class SimpleForwardingChannelBuilder + > extends ForwardingChannelBuilder { +} diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java index a3d34906e9f..90114682aa3 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java @@ -2,6 +2,7 @@ public class ManagedChannelImplBuilder extends AbstractManagedChannelImplBuilder { private final TransportFactoryBuilder transportFactoryBuilder; + private int defaultPort; public interface TransportFactoryBuilder { ClientTransportFactory buildTransportFactory(); @@ -11,6 +12,7 @@ protected ManagedChannelImplBuilder(String target, TransportFactoryBuilder transportFactoryBuilder) { super(target); this.transportFactoryBuilder = transportFactoryBuilder; + this.defaultPort = super.getDefaultPort(); } public static ManagedChannelImplBuilder forTarget(String target, @@ -22,4 +24,13 @@ public static ManagedChannelImplBuilder forTarget(String target, protected ClientTransportFactory buildTransportFactory() { return transportFactoryBuilder.buildTransportFactory(); } + + @Override + public int getDefaultPort() { + return defaultPort; + } + + public void setDefaultPort(int defaultPort) { + this.defaultPort = defaultPort; + } } \ No newline at end of file diff --git a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java index 56f247b503d..6fcd3468aa9 100644 --- a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java +++ b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java @@ -24,8 +24,9 @@ import com.google.common.base.Preconditions; import io.grpc.ChannelLogger; import io.grpc.ExperimentalApi; -import io.grpc.ForwardingChannelBuilder; import io.grpc.Internal; +import io.grpc.ManagedChannel; +import io.grpc.SimpleForwardingChannelBuilder; import io.grpc.internal.AtomicBackoff; import io.grpc.internal.ClientTransportFactory; import io.grpc.internal.ConnectionClientTransport; @@ -56,7 +57,7 @@ /** Convenience class for building channels with the OkHttp transport. */ @ExperimentalApi("https://github.com/grpc/grpc-java/issues/1785") public class OkHttpChannelBuilder extends - ForwardingChannelBuilder { + SimpleForwardingChannelBuilder { public static final int DEFAULT_FLOW_CONTROL_WINDOW = 65535; private final ManagedChannelImplBuilder managedChannelImplBuilder; @@ -394,7 +395,12 @@ private final ClientTransportFactory buildTransportFactory() { } @Override - protected int getDefaultPort() { + public ManagedChannel build() { + managedChannelImplBuilder.setDefaultPort(getDefaultPort()); + return super.build(); + } + + int getDefaultPort() { switch (negotiationType) { case PLAINTEXT: return GrpcUtil.DEFAULT_PORT_PLAINTEXT; From 897e5f5559439d5ea2ee54973dee075b01294248 Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Thu, 13 Aug 2020 20:18:52 -0400 Subject: [PATCH 03/54] fix maxInboundMessageSize() --- .../io/grpc/internal/AbstractManagedChannelImplBuilder.java | 2 +- .../main/java/io/grpc/internal/ManagedChannelImplBuilder.java | 4 ++++ okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java | 2 +- 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java index b92fdf5410a..4a34fcd7369 100644 --- a/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java @@ -171,7 +171,7 @@ public T maxInboundMessageSize(int max) { return thisT(); } - protected final int maxInboundMessageSize() { + protected int maxInboundMessageSize() { return maxInboundMessageSize; } diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java index 90114682aa3..f8ff243ad0b 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java @@ -33,4 +33,8 @@ public int getDefaultPort() { public void setDefaultPort(int defaultPort) { this.defaultPort = defaultPort; } + + public int maxInboundMessageSize() { + return super.maxInboundMessageSize(); + } } \ No newline at end of file diff --git a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java index 6fcd3468aa9..02b4d21e9b9 100644 --- a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java +++ b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java @@ -383,7 +383,7 @@ private final ClientTransportFactory buildTransportFactory() { createSslSocketFactory(), hostnameVerifier, connectionSpec, - maxInboundMessageSize(), + managedChannelImplBuilder.maxInboundMessageSize(), enableKeepAlive, keepAliveTimeNanos, keepAliveTimeoutNanos, From 795a821183eb873d712028abea3e297be6469a06 Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Thu, 13 Aug 2020 20:33:26 -0400 Subject: [PATCH 04/54] Java 7 support --- .../grpc/SimpleForwardingChannelBuilder.java | 2 -- .../internal/ManagedChannelImplBuilder.java | 19 +++++++++++-------- .../io/grpc/okhttp/OkHttpChannelBuilder.java | 9 +++++++-- 3 files changed, 18 insertions(+), 12 deletions(-) diff --git a/api/src/main/java/io/grpc/SimpleForwardingChannelBuilder.java b/api/src/main/java/io/grpc/SimpleForwardingChannelBuilder.java index 9a071faeeae..0def3f670c0 100644 --- a/api/src/main/java/io/grpc/SimpleForwardingChannelBuilder.java +++ b/api/src/main/java/io/grpc/SimpleForwardingChannelBuilder.java @@ -1,7 +1,5 @@ package io.grpc; -import io.grpc.internal.AbstractManagedChannelImplBuilder; - public abstract class SimpleForwardingChannelBuilder > extends ForwardingChannelBuilder { } diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java index f8ff243ad0b..7acf9b11ec2 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java @@ -1,28 +1,31 @@ package io.grpc.internal; +import java.util.concurrent.Callable; + public class ManagedChannelImplBuilder extends AbstractManagedChannelImplBuilder { - private final TransportFactoryBuilder transportFactoryBuilder; + private final Callable transportFactoryBuilder; private int defaultPort; - public interface TransportFactoryBuilder { - ClientTransportFactory buildTransportFactory(); - } - protected ManagedChannelImplBuilder(String target, - TransportFactoryBuilder transportFactoryBuilder) { + Callable transportFactoryBuilder) { super(target); this.transportFactoryBuilder = transportFactoryBuilder; this.defaultPort = super.getDefaultPort(); } public static ManagedChannelImplBuilder forTarget(String target, - TransportFactoryBuilder transportFactoryBuilder) { + Callable transportFactoryBuilder) { return new ManagedChannelImplBuilder(target, transportFactoryBuilder); } @Override protected ClientTransportFactory buildTransportFactory() { - return transportFactoryBuilder.buildTransportFactory(); + try { + return transportFactoryBuilder.call(); + } catch (Exception e) { + e.printStackTrace(); + } + return null; } @Override diff --git a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java index 02b4d21e9b9..82706eec0ee 100644 --- a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java +++ b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java @@ -43,6 +43,7 @@ import java.net.InetSocketAddress; import java.net.SocketAddress; import java.security.GeneralSecurityException; +import java.util.concurrent.Callable; import java.util.concurrent.Executor; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; @@ -150,8 +151,12 @@ protected OkHttpChannelBuilder(String host, int port) { private OkHttpChannelBuilder(String target) { super(); - managedChannelImplBuilder = ManagedChannelImplBuilder.forTarget(target, - this::buildTransportFactory); + managedChannelImplBuilder = ManagedChannelImplBuilder.forTarget(target, new Callable() { + @Override + public ClientTransportFactory call() { + return buildTransportFactory(); + } + }); } @VisibleForTesting From 9c4deecb9424238eab1bc6ceeca723320c4b486e Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Thu, 13 Aug 2020 20:39:22 -0400 Subject: [PATCH 05/54] Minor format --- .../main/java/io/grpc/internal/ManagedChannelImplBuilder.java | 3 ++- okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java index 7acf9b11ec2..2f4e9e5c559 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java @@ -25,6 +25,7 @@ protected ClientTransportFactory buildTransportFactory() { } catch (Exception e) { e.printStackTrace(); } + // @todo: handle differently? return null; } @@ -40,4 +41,4 @@ public void setDefaultPort(int defaultPort) { public int maxInboundMessageSize() { return super.maxInboundMessageSize(); } -} \ No newline at end of file +} diff --git a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java index 82706eec0ee..5fe942025d2 100644 --- a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java +++ b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java @@ -405,7 +405,7 @@ public ManagedChannel build() { return super.build(); } - int getDefaultPort() { + public int getDefaultPort() { switch (negotiationType) { case PLAINTEXT: return GrpcUtil.DEFAULT_PORT_PLAINTEXT; From bf4af5a83e7951d3faf138fe4691224671eac355 Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Fri, 14 Aug 2020 14:42:47 -0400 Subject: [PATCH 06/54] Use interface to pass buildTransportFactory() --- .../internal/ManagedChannelImplBuilder.java | 22 +++++++++---------- .../io/grpc/okhttp/OkHttpChannelBuilder.java | 15 +++++-------- 2 files changed, 16 insertions(+), 21 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java index 2f4e9e5c559..0865b1241da 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java @@ -1,32 +1,29 @@ package io.grpc.internal; -import java.util.concurrent.Callable; public class ManagedChannelImplBuilder extends AbstractManagedChannelImplBuilder { - private final Callable transportFactoryBuilder; + public interface ClientTransportFactoryFactory { + ClientTransportFactory buildTransportFactory(); + } + + private final ClientTransportFactoryFactory clientTransportFactoryFactory; private int defaultPort; protected ManagedChannelImplBuilder(String target, - Callable transportFactoryBuilder) { + ClientTransportFactoryFactory clientTransportFactoryFactory) { super(target); - this.transportFactoryBuilder = transportFactoryBuilder; + this.clientTransportFactoryFactory = clientTransportFactoryFactory; this.defaultPort = super.getDefaultPort(); } public static ManagedChannelImplBuilder forTarget(String target, - Callable transportFactoryBuilder) { + ClientTransportFactoryFactory transportFactoryBuilder) { return new ManagedChannelImplBuilder(target, transportFactoryBuilder); } @Override protected ClientTransportFactory buildTransportFactory() { - try { - return transportFactoryBuilder.call(); - } catch (Exception e) { - e.printStackTrace(); - } - // @todo: handle differently? - return null; + return clientTransportFactoryFactory.buildTransportFactory(); } @Override @@ -38,6 +35,7 @@ public void setDefaultPort(int defaultPort) { this.defaultPort = defaultPort; } + @Override public int maxInboundMessageSize() { return super.maxInboundMessageSize(); } diff --git a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java index 5fe942025d2..ad8f706d8db 100644 --- a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java +++ b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java @@ -43,7 +43,6 @@ import java.net.InetSocketAddress; import java.net.SocketAddress; import java.security.GeneralSecurityException; -import java.util.concurrent.Callable; import java.util.concurrent.Executor; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; @@ -58,7 +57,8 @@ /** Convenience class for building channels with the OkHttp transport. */ @ExperimentalApi("https://github.com/grpc/grpc-java/issues/1785") public class OkHttpChannelBuilder extends - SimpleForwardingChannelBuilder { + SimpleForwardingChannelBuilder implements + ManagedChannelImplBuilder.ClientTransportFactoryFactory { public static final int DEFAULT_FLOW_CONTROL_WINDOW = 65535; private final ManagedChannelImplBuilder managedChannelImplBuilder; @@ -151,12 +151,7 @@ protected OkHttpChannelBuilder(String host, int port) { private OkHttpChannelBuilder(String target) { super(); - managedChannelImplBuilder = ManagedChannelImplBuilder.forTarget(target, new Callable() { - @Override - public ClientTransportFactory call() { - return buildTransportFactory(); - } - }); + managedChannelImplBuilder = ManagedChannelImplBuilder.forTarget(target, this); } @VisibleForTesting @@ -379,7 +374,9 @@ public OkHttpChannelBuilder maxInboundMetadataSize(int bytes) { return this; } - private final ClientTransportFactory buildTransportFactory() { + @Internal + @Override + public final ClientTransportFactory buildTransportFactory() { boolean enableKeepAlive = keepAliveTimeNanos != KEEPALIVE_TIME_NANOS_DISABLED; return new OkHttpTransportFactory( transportExecutor, From bf9a2a855c1f314c1808073081984b7a0739e849 Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Mon, 17 Aug 2020 11:46:46 -0400 Subject: [PATCH 07/54] Save notes --- .../io/grpc/internal/ManagedChannelImplBuilder.java | 10 +++------- .../main/java/io/grpc/okhttp/OkHttpChannelBuilder.java | 1 + 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java index 0865b1241da..df8c8cabc98 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java @@ -1,6 +1,6 @@ package io.grpc.internal; - +// we won't be able to override checkAuthority anymore - bury it here public class ManagedChannelImplBuilder extends AbstractManagedChannelImplBuilder { public interface ClientTransportFactoryFactory { ClientTransportFactory buildTransportFactory(); @@ -9,6 +9,7 @@ public interface ClientTransportFactoryFactory { private final ClientTransportFactoryFactory clientTransportFactoryFactory; private int defaultPort; + // one more for SocketAddress protected ManagedChannelImplBuilder(String target, ClientTransportFactoryFactory clientTransportFactoryFactory) { super(target); @@ -16,13 +17,9 @@ protected ManagedChannelImplBuilder(String target, this.defaultPort = super.getDefaultPort(); } - public static ManagedChannelImplBuilder forTarget(String target, - ClientTransportFactoryFactory transportFactoryBuilder) { - return new ManagedChannelImplBuilder(target, transportFactoryBuilder); - } - @Override protected ClientTransportFactory buildTransportFactory() { + // set the port return clientTransportFactoryFactory.buildTransportFactory(); } @@ -38,5 +35,4 @@ public void setDefaultPort(int defaultPort) { @Override public int maxInboundMessageSize() { return super.maxInboundMessageSize(); - } } diff --git a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java index ad8f706d8db..18acede1da5 100644 --- a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java +++ b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java @@ -377,6 +377,7 @@ public OkHttpChannelBuilder maxInboundMetadataSize(int bytes) { @Internal @Override public final ClientTransportFactory buildTransportFactory() { + // @todo: internal class, anonymous class (worse for debugging) boolean enableKeepAlive = keepAliveTimeNanos != KEEPALIVE_TIME_NANOS_DISABLED; return new OkHttpTransportFactory( transportExecutor, From e60db3eaa4baf6c32fc05534c15d05ebf2105aec Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Mon, 17 Aug 2020 15:27:12 -0400 Subject: [PATCH 08/54] Fix lint, make build pass --- .../internal/ManagedChannelImplBuilder.java | 91 ++++++++++++------- .../io/grpc/okhttp/OkHttpChannelBuilder.java | 4 +- 2 files changed, 59 insertions(+), 36 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java index df8c8cabc98..aa031f18629 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java @@ -1,38 +1,61 @@ +/* + * Copyright 2020 The gRPC Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + package io.grpc.internal; // we won't be able to override checkAuthority anymore - bury it here -public class ManagedChannelImplBuilder extends AbstractManagedChannelImplBuilder { - public interface ClientTransportFactoryFactory { - ClientTransportFactory buildTransportFactory(); - } - - private final ClientTransportFactoryFactory clientTransportFactoryFactory; - private int defaultPort; - - // one more for SocketAddress - protected ManagedChannelImplBuilder(String target, - ClientTransportFactoryFactory clientTransportFactoryFactory) { - super(target); - this.clientTransportFactoryFactory = clientTransportFactoryFactory; - this.defaultPort = super.getDefaultPort(); - } - - @Override - protected ClientTransportFactory buildTransportFactory() { - // set the port - return clientTransportFactoryFactory.buildTransportFactory(); - } - - @Override - public int getDefaultPort() { - return defaultPort; - } - - public void setDefaultPort(int defaultPort) { - this.defaultPort = defaultPort; - } - - @Override - public int maxInboundMessageSize() { - return super.maxInboundMessageSize(); +public class ManagedChannelImplBuilder + extends AbstractManagedChannelImplBuilder { + public interface ClientTransportFactoryFactory { + ClientTransportFactory buildTransportFactory(); + } + + private final ClientTransportFactoryFactory clientTransportFactoryFactory; + private int defaultPort; + + /** + * Creates a new builder for the given target that will be resolved by + * {@link io.grpc.NameResolver}. + */ + public ManagedChannelImplBuilder(String target, + ClientTransportFactoryFactory clientTransportFactoryFactory) { + // TODO(sergiitk): finish docblock + // TODO(sergiitk): one more for SocketAddress + super(target); + this.clientTransportFactoryFactory = clientTransportFactoryFactory; + this.defaultPort = super.getDefaultPort(); + } + + @Override + protected ClientTransportFactory buildTransportFactory() { + // set the port + return clientTransportFactoryFactory.buildTransportFactory(); + } + + @Override + public int getDefaultPort() { + return defaultPort; + } + + public void setDefaultPort(int defaultPort) { + this.defaultPort = defaultPort; + } + + @Override + public int maxInboundMessageSize() { + return super.maxInboundMessageSize(); + } } diff --git a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java index 18acede1da5..69e8d960cef 100644 --- a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java +++ b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java @@ -151,7 +151,7 @@ protected OkHttpChannelBuilder(String host, int port) { private OkHttpChannelBuilder(String target) { super(); - managedChannelImplBuilder = ManagedChannelImplBuilder.forTarget(target, this); + managedChannelImplBuilder = new ManagedChannelImplBuilder(target, this); } @VisibleForTesting @@ -377,7 +377,7 @@ public OkHttpChannelBuilder maxInboundMetadataSize(int bytes) { @Internal @Override public final ClientTransportFactory buildTransportFactory() { - // @todo: internal class, anonymous class (worse for debugging) + // TODO(sergiitk): internal class, anonymous class (worse for debugging) boolean enableKeepAlive = keepAliveTimeNanos != KEEPALIVE_TIME_NANOS_DISABLED; return new OkHttpTransportFactory( transportExecutor, From 644628833b73b15fdb7c5b7ad72f585cb9e34415 Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Mon, 17 Aug 2020 16:41:23 -0400 Subject: [PATCH 09/54] Inject buildTransportFactory via ClientTransportFactoryFactory interface --- .../io/grpc/okhttp/OkHttpChannelBuilder.java | 56 ++++++++++--------- 1 file changed, 30 insertions(+), 26 deletions(-) diff --git a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java index 69e8d960cef..c285a68ccd2 100644 --- a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java +++ b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java @@ -57,8 +57,7 @@ /** Convenience class for building channels with the OkHttp transport. */ @ExperimentalApi("https://github.com/grpc/grpc-java/issues/1785") public class OkHttpChannelBuilder extends - SimpleForwardingChannelBuilder implements - ManagedChannelImplBuilder.ClientTransportFactoryFactory { + SimpleForwardingChannelBuilder { public static final int DEFAULT_FLOW_CONTROL_WINDOW = 65535; private final ManagedChannelImplBuilder managedChannelImplBuilder; @@ -151,7 +150,35 @@ protected OkHttpChannelBuilder(String host, int port) { private OkHttpChannelBuilder(String target) { super(); - managedChannelImplBuilder = new ManagedChannelImplBuilder(target, this); + + final class OkHttpChannelTransportFactoryBuilder implements + ManagedChannelImplBuilder.ClientTransportFactoryFactory { + + @Override + @Internal + public ClientTransportFactory buildTransportFactory() { + boolean enableKeepAlive = keepAliveTimeNanos != KEEPALIVE_TIME_NANOS_DISABLED; + return new OkHttpTransportFactory( + transportExecutor, + scheduledExecutorService, + socketFactory, + createSslSocketFactory(), + hostnameVerifier, + connectionSpec, + managedChannelImplBuilder.maxInboundMessageSize(), + enableKeepAlive, + keepAliveTimeNanos, + keepAliveTimeoutNanos, + flowControlWindow, + keepAliveWithoutCalls, + maxInboundMetadataSize, + transportTracerFactory, + useGetForSafeMethods); + } + } + + managedChannelImplBuilder = new ManagedChannelImplBuilder(target, + new OkHttpChannelTransportFactoryBuilder()); } @VisibleForTesting @@ -374,29 +401,6 @@ public OkHttpChannelBuilder maxInboundMetadataSize(int bytes) { return this; } - @Internal - @Override - public final ClientTransportFactory buildTransportFactory() { - // TODO(sergiitk): internal class, anonymous class (worse for debugging) - boolean enableKeepAlive = keepAliveTimeNanos != KEEPALIVE_TIME_NANOS_DISABLED; - return new OkHttpTransportFactory( - transportExecutor, - scheduledExecutorService, - socketFactory, - createSslSocketFactory(), - hostnameVerifier, - connectionSpec, - managedChannelImplBuilder.maxInboundMessageSize(), - enableKeepAlive, - keepAliveTimeNanos, - keepAliveTimeoutNanos, - flowControlWindow, - keepAliveWithoutCalls, - maxInboundMetadataSize, - transportTracerFactory, - useGetForSafeMethods); - } - @Override public ManagedChannel build() { managedChannelImplBuilder.setDefaultPort(getDefaultPort()); From d15baa5146b26edb81a7855fa06b030cbef75aea Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Mon, 17 Aug 2020 16:45:57 -0400 Subject: [PATCH 10/54] Rename ClientTransportFactoryFactory to ClientTransportFactoryBuilder --- .../io/grpc/internal/ManagedChannelImplBuilder.java | 10 +++++----- .../main/java/io/grpc/okhttp/OkHttpChannelBuilder.java | 6 +++--- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java index aa031f18629..a9491915eca 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java @@ -19,11 +19,11 @@ // we won't be able to override checkAuthority anymore - bury it here public class ManagedChannelImplBuilder extends AbstractManagedChannelImplBuilder { - public interface ClientTransportFactoryFactory { + public interface ClientTransportFactoryBuilder { ClientTransportFactory buildTransportFactory(); } - private final ClientTransportFactoryFactory clientTransportFactoryFactory; + private final ClientTransportFactoryBuilder clientTransportFactoryBuilder; private int defaultPort; /** @@ -31,18 +31,18 @@ public interface ClientTransportFactoryFactory { * {@link io.grpc.NameResolver}. */ public ManagedChannelImplBuilder(String target, - ClientTransportFactoryFactory clientTransportFactoryFactory) { + ClientTransportFactoryBuilder clientTransportFactoryBuilder) { // TODO(sergiitk): finish docblock // TODO(sergiitk): one more for SocketAddress super(target); - this.clientTransportFactoryFactory = clientTransportFactoryFactory; + this.clientTransportFactoryBuilder = clientTransportFactoryBuilder; this.defaultPort = super.getDefaultPort(); } @Override protected ClientTransportFactory buildTransportFactory() { // set the port - return clientTransportFactoryFactory.buildTransportFactory(); + return clientTransportFactoryBuilder.buildTransportFactory(); } @Override diff --git a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java index c285a68ccd2..2c58cfa7eeb 100644 --- a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java +++ b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java @@ -33,6 +33,7 @@ import io.grpc.internal.GrpcUtil; import io.grpc.internal.KeepAliveManager; import io.grpc.internal.ManagedChannelImplBuilder; +import io.grpc.internal.ManagedChannelImplBuilder.ClientTransportFactoryBuilder; import io.grpc.internal.SharedResourceHolder; import io.grpc.internal.SharedResourceHolder.Resource; import io.grpc.internal.TransportTracer; @@ -151,9 +152,8 @@ protected OkHttpChannelBuilder(String host, int port) { private OkHttpChannelBuilder(String target) { super(); - final class OkHttpChannelTransportFactoryBuilder implements - ManagedChannelImplBuilder.ClientTransportFactoryFactory { - + // An anonymous class to inject buildTransportFactory(). + final class OkHttpChannelTransportFactoryBuilder implements ClientTransportFactoryBuilder { @Override @Internal public ClientTransportFactory buildTransportFactory() { From 2fb795d7d6b9229731e44612fa6901c8238074d4 Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Mon, 17 Aug 2020 17:32:49 -0400 Subject: [PATCH 11/54] Move SimpleForwardingChannelBuilder into ForwardingChannelBuilder --- api/src/main/java/io/grpc/ForwardingChannelBuilder.java | 8 ++++++++ .../main/java/io/grpc/SimpleForwardingChannelBuilder.java | 5 ----- .../java/io/grpc/internal/ManagedChannelImplBuilder.java | 5 +++-- .../main/java/io/grpc/okhttp/OkHttpChannelBuilder.java | 4 ++-- 4 files changed, 13 insertions(+), 9 deletions(-) delete mode 100644 api/src/main/java/io/grpc/SimpleForwardingChannelBuilder.java diff --git a/api/src/main/java/io/grpc/ForwardingChannelBuilder.java b/api/src/main/java/io/grpc/ForwardingChannelBuilder.java index 612779a7848..d7bd5cf0d85 100644 --- a/api/src/main/java/io/grpc/ForwardingChannelBuilder.java +++ b/api/src/main/java/io/grpc/ForwardingChannelBuilder.java @@ -268,4 +268,12 @@ protected final T thisT() { T thisT = (T) this; return thisT; } + + /** + * A simplified version of {@link ForwardingChannelBuilder}. + */ + public abstract static class SimpleForwardingChannelBuilder> + extends ForwardingChannelBuilder { + // TODO(sergiitk): implement + } } diff --git a/api/src/main/java/io/grpc/SimpleForwardingChannelBuilder.java b/api/src/main/java/io/grpc/SimpleForwardingChannelBuilder.java deleted file mode 100644 index 0def3f670c0..00000000000 --- a/api/src/main/java/io/grpc/SimpleForwardingChannelBuilder.java +++ /dev/null @@ -1,5 +0,0 @@ -package io.grpc; - -public abstract class SimpleForwardingChannelBuilder - > extends ForwardingChannelBuilder { -} diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java index a9491915eca..0e06bd91d62 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java @@ -19,6 +19,7 @@ // we won't be able to override checkAuthority anymore - bury it here public class ManagedChannelImplBuilder extends AbstractManagedChannelImplBuilder { + public interface ClientTransportFactoryBuilder { ClientTransportFactory buildTransportFactory(); } @@ -27,8 +28,8 @@ public interface ClientTransportFactoryBuilder { private int defaultPort; /** - * Creates a new builder for the given target that will be resolved by - * {@link io.grpc.NameResolver}. + * Creates a new builder for the given target that will be resolved by {@link + * io.grpc.NameResolver}. */ public ManagedChannelImplBuilder(String target, ClientTransportFactoryBuilder clientTransportFactoryBuilder) { diff --git a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java index 2c58cfa7eeb..ca3a1871398 100644 --- a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java +++ b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java @@ -24,9 +24,9 @@ import com.google.common.base.Preconditions; import io.grpc.ChannelLogger; import io.grpc.ExperimentalApi; +import io.grpc.ForwardingChannelBuilder.SimpleForwardingChannelBuilder; import io.grpc.Internal; import io.grpc.ManagedChannel; -import io.grpc.SimpleForwardingChannelBuilder; import io.grpc.internal.AtomicBackoff; import io.grpc.internal.ClientTransportFactory; import io.grpc.internal.ConnectionClientTransport; @@ -58,7 +58,7 @@ /** Convenience class for building channels with the OkHttp transport. */ @ExperimentalApi("https://github.com/grpc/grpc-java/issues/1785") public class OkHttpChannelBuilder extends - SimpleForwardingChannelBuilder { + SimpleForwardingChannelBuilder { public static final int DEFAULT_FLOW_CONTROL_WINDOW = 65535; private final ManagedChannelImplBuilder managedChannelImplBuilder; From 93806b33aaa15fc6caee2ca1b4f1803a8b7a1447 Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Mon, 17 Aug 2020 18:23:27 -0400 Subject: [PATCH 12/54] Refactor maxInboundMessageSize() --- .../grpc/internal/AbstractManagedChannelImplBuilder.java | 2 +- .../java/io/grpc/internal/ManagedChannelImplBuilder.java | 9 ++------- .../main/java/io/grpc/okhttp/OkHttpChannelBuilder.java | 6 +++--- 3 files changed, 6 insertions(+), 11 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java index 4a34fcd7369..b92fdf5410a 100644 --- a/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java @@ -171,7 +171,7 @@ public T maxInboundMessageSize(int max) { return thisT(); } - protected int maxInboundMessageSize() { + protected final int maxInboundMessageSize() { return maxInboundMessageSize; } diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java index 0e06bd91d62..282a191aa71 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java @@ -21,7 +21,7 @@ public class ManagedChannelImplBuilder extends AbstractManagedChannelImplBuilder { public interface ClientTransportFactoryBuilder { - ClientTransportFactory buildTransportFactory(); + ClientTransportFactory buildClientTransportFactory(int maxInboundMessageSize); } private final ClientTransportFactoryBuilder clientTransportFactoryBuilder; @@ -43,7 +43,7 @@ public ManagedChannelImplBuilder(String target, @Override protected ClientTransportFactory buildTransportFactory() { // set the port - return clientTransportFactoryBuilder.buildTransportFactory(); + return clientTransportFactoryBuilder.buildClientTransportFactory(maxInboundMessageSize()); } @Override @@ -54,9 +54,4 @@ public int getDefaultPort() { public void setDefaultPort(int defaultPort) { this.defaultPort = defaultPort; } - - @Override - public int maxInboundMessageSize() { - return super.maxInboundMessageSize(); - } } diff --git a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java index ca3a1871398..fddac25697c 100644 --- a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java +++ b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java @@ -152,11 +152,11 @@ protected OkHttpChannelBuilder(String host, int port) { private OkHttpChannelBuilder(String target) { super(); - // An anonymous class to inject buildTransportFactory(). + // An anonymous class to inject client transport factory builder. final class OkHttpChannelTransportFactoryBuilder implements ClientTransportFactoryBuilder { @Override @Internal - public ClientTransportFactory buildTransportFactory() { + public ClientTransportFactory buildClientTransportFactory(int maxInboundMessageSize) { boolean enableKeepAlive = keepAliveTimeNanos != KEEPALIVE_TIME_NANOS_DISABLED; return new OkHttpTransportFactory( transportExecutor, @@ -165,7 +165,7 @@ public ClientTransportFactory buildTransportFactory() { createSslSocketFactory(), hostnameVerifier, connectionSpec, - managedChannelImplBuilder.maxInboundMessageSize(), + maxInboundMessageSize, enableKeepAlive, keepAliveTimeNanos, keepAliveTimeoutNanos, From bf45ded080b0fd236f9dbe76772b8e21b4ed8453 Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Mon, 17 Aug 2020 18:42:56 -0400 Subject: [PATCH 13/54] Make transportTracerFactory protected to keep compatibility with AbstractManagedChannelImplBuilder --- .../java/io/grpc/okhttp/OkHttpChannelBuilder.java | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java index fddac25697c..5de801ff6ac 100644 --- a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java +++ b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java @@ -57,12 +57,11 @@ /** Convenience class for building channels with the OkHttp transport. */ @ExperimentalApi("https://github.com/grpc/grpc-java/issues/1785") -public class OkHttpChannelBuilder extends - SimpleForwardingChannelBuilder { +public class OkHttpChannelBuilder extends SimpleForwardingChannelBuilder { public static final int DEFAULT_FLOW_CONTROL_WINDOW = 65535; + protected TransportTracer.Factory transportTracerFactory = TransportTracer.getDefaultFactory(); private final ManagedChannelImplBuilder managedChannelImplBuilder; - private TransportTracer.Factory transportTracerFactory = TransportTracer.getDefaultFactory(); /** Identifies the negotiation used for starting up HTTP/2. */ private enum NegotiationType { @@ -120,11 +119,6 @@ public static OkHttpChannelBuilder forTarget(String target) { return new OkHttpChannelBuilder(target); } - @Override - protected ManagedChannelImplBuilder delegate() { - return managedChannelImplBuilder; - } - private Executor transportExecutor; private ScheduledExecutorService scheduledExecutorService; @@ -181,6 +175,11 @@ public ClientTransportFactory buildClientTransportFactory(int maxInboundMessageS new OkHttpChannelTransportFactoryBuilder()); } + @Override + protected ManagedChannelImplBuilder delegate() { + return managedChannelImplBuilder; + } + @VisibleForTesting final OkHttpChannelBuilder setTransportTracerFactory( TransportTracer.Factory transportTracerFactory) { From 4c9193db61bf636bd27adc69c243e6cd19ab840e Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Mon, 17 Aug 2020 20:09:44 -0400 Subject: [PATCH 14/54] Implement optional ChannelBuilderDefaultPortProvider --- .../internal/ManagedChannelImplBuilder.java | 35 ++++++++++++++----- .../io/grpc/okhttp/OkHttpChannelBuilder.java | 9 +---- 2 files changed, 27 insertions(+), 17 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java index 282a191aa71..d4faabf5c24 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java @@ -16,6 +16,8 @@ package io.grpc.internal; +import javax.annotation.Nullable; + // we won't be able to override checkAuthority anymore - bury it here public class ManagedChannelImplBuilder extends AbstractManagedChannelImplBuilder { @@ -24,34 +26,49 @@ public interface ClientTransportFactoryBuilder { ClientTransportFactory buildClientTransportFactory(int maxInboundMessageSize); } + public interface ChannelBuilderDefaultPortProvider { + int getDefaultPort(); + } + private final ClientTransportFactoryBuilder clientTransportFactoryBuilder; + @Nullable private final ChannelBuilderDefaultPortProvider channelBuilderDefaultPortProvider; private int defaultPort; /** - * Creates a new builder for the given target that will be resolved by {@link - * io.grpc.NameResolver}. + * Creates a new builder for the given target that will be resolved by + * {@link io.grpc.NameResolver}. */ public ManagedChannelImplBuilder(String target, ClientTransportFactoryBuilder clientTransportFactoryBuilder) { - // TODO(sergiitk): finish docblock + // TODO(sergiitk): finish javadoc + this(target, clientTransportFactoryBuilder, null); + } + + /** + * Creates a new builder for the given target that will be resolved by + * {@link io.grpc.NameResolver}. + */ + public ManagedChannelImplBuilder(String target, + ClientTransportFactoryBuilder clientTransportFactoryBuilder, + @Nullable ChannelBuilderDefaultPortProvider channelBuilderDefaultPortProvider) { + // TODO(sergiitk): finish javadoc // TODO(sergiitk): one more for SocketAddress super(target); this.clientTransportFactoryBuilder = clientTransportFactoryBuilder; + this.channelBuilderDefaultPortProvider = channelBuilderDefaultPortProvider; this.defaultPort = super.getDefaultPort(); } @Override protected ClientTransportFactory buildTransportFactory() { - // set the port + if (channelBuilderDefaultPortProvider != null) { + defaultPort = channelBuilderDefaultPortProvider.getDefaultPort(); + } return clientTransportFactoryBuilder.buildClientTransportFactory(maxInboundMessageSize()); } @Override - public int getDefaultPort() { + protected int getDefaultPort() { return defaultPort; } - - public void setDefaultPort(int defaultPort) { - this.defaultPort = defaultPort; - } } diff --git a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java index 5de801ff6ac..a9f3e8bff92 100644 --- a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java +++ b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java @@ -149,7 +149,6 @@ private OkHttpChannelBuilder(String target) { // An anonymous class to inject client transport factory builder. final class OkHttpChannelTransportFactoryBuilder implements ClientTransportFactoryBuilder { @Override - @Internal public ClientTransportFactory buildClientTransportFactory(int maxInboundMessageSize) { boolean enableKeepAlive = keepAliveTimeNanos != KEEPALIVE_TIME_NANOS_DISABLED; return new OkHttpTransportFactory( @@ -400,13 +399,7 @@ public OkHttpChannelBuilder maxInboundMetadataSize(int bytes) { return this; } - @Override - public ManagedChannel build() { - managedChannelImplBuilder.setDefaultPort(getDefaultPort()); - return super.build(); - } - - public int getDefaultPort() { + protected int getDefaultPort() { switch (negotiationType) { case PLAINTEXT: return GrpcUtil.DEFAULT_PORT_PLAINTEXT; From 4362ccefbec938a5204a274fcd1f333f5937683a Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Mon, 17 Aug 2020 20:15:47 -0400 Subject: [PATCH 15/54] OkHttpChannelBuilder passes getDefaultPort() --- .../java/io/grpc/okhttp/OkHttpChannelBuilder.java | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java index a9f3e8bff92..a50527f82e4 100644 --- a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java +++ b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java @@ -33,6 +33,7 @@ import io.grpc.internal.GrpcUtil; import io.grpc.internal.KeepAliveManager; import io.grpc.internal.ManagedChannelImplBuilder; +import io.grpc.internal.ManagedChannelImplBuilder.ChannelBuilderDefaultPortProvider; import io.grpc.internal.ManagedChannelImplBuilder.ClientTransportFactoryBuilder; import io.grpc.internal.SharedResourceHolder; import io.grpc.internal.SharedResourceHolder.Resource; @@ -170,8 +171,17 @@ public ClientTransportFactory buildClientTransportFactory(int maxInboundMessageS } } + // An anonymous class to inject client transport factory builder. + final class OkHttpChannelDefaultPortProvider implements ChannelBuilderDefaultPortProvider { + @Override + public int getDefaultPort() { + return OkHttpChannelBuilder.this.getDefaultPort(); + } + } + managedChannelImplBuilder = new ManagedChannelImplBuilder(target, - new OkHttpChannelTransportFactoryBuilder()); + new OkHttpChannelTransportFactoryBuilder(), + new OkHttpChannelDefaultPortProvider()); } @Override From f293bafe60cbb5c81fe6abe8cfd72409ec95793a Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Mon, 17 Aug 2020 20:17:09 -0400 Subject: [PATCH 16/54] Comment to todo --- .../main/java/io/grpc/internal/ManagedChannelImplBuilder.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java index d4faabf5c24..234179a77ea 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java @@ -18,7 +18,7 @@ import javax.annotation.Nullable; -// we won't be able to override checkAuthority anymore - bury it here +// TODO(sergiitk): We won't be able to override checkAuthority anymore - bury it here public class ManagedChannelImplBuilder extends AbstractManagedChannelImplBuilder { From fd0da1abf856160578850d7b90134bf67dce824f Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Tue, 18 Aug 2020 10:59:23 -0400 Subject: [PATCH 17/54] Start working on NettyChannelBuilder --- .../internal/ManagedChannelImplBuilder.java | 18 +++- .../io/grpc/netty/NettyChannelBuilder.java | 87 ++++++++++++------- .../io/grpc/okhttp/OkHttpChannelBuilder.java | 2 +- 3 files changed, 73 insertions(+), 34 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java index 234179a77ea..4fe63ff02d8 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java @@ -16,6 +16,7 @@ package io.grpc.internal; +import java.net.SocketAddress; import javax.annotation.Nullable; // TODO(sergiitk): We won't be able to override checkAuthority anymore - bury it here @@ -52,13 +53,28 @@ public ManagedChannelImplBuilder(String target, ClientTransportFactoryBuilder clientTransportFactoryBuilder, @Nullable ChannelBuilderDefaultPortProvider channelBuilderDefaultPortProvider) { // TODO(sergiitk): finish javadoc - // TODO(sergiitk): one more for SocketAddress super(target); this.clientTransportFactoryBuilder = clientTransportFactoryBuilder; this.channelBuilderDefaultPortProvider = channelBuilderDefaultPortProvider; this.defaultPort = super.getDefaultPort(); } + public ManagedChannelImplBuilder(SocketAddress directServerAddress, String authority, + ClientTransportFactoryBuilder clientTransportFactoryBuilder) { + // TODO(sergiitk): finish javadoc + this(directServerAddress, authority, clientTransportFactoryBuilder, null); + } + + public ManagedChannelImplBuilder(SocketAddress directServerAddress, String authority, + ClientTransportFactoryBuilder clientTransportFactoryBuilder, + @Nullable ChannelBuilderDefaultPortProvider channelBuilderDefaultPortProvider) { + // TODO(sergiitk): finish javadoc + super(directServerAddress, authority); + this.clientTransportFactoryBuilder = clientTransportFactoryBuilder; + this.channelBuilderDefaultPortProvider = channelBuilderDefaultPortProvider; + this.defaultPort = super.getDefaultPort(); + } + @Override protected ClientTransportFactory buildTransportFactory() { if (channelBuilderDefaultPortProvider != null) { diff --git a/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java b/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java index 59e86a117da..a8508d67bb8 100644 --- a/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java +++ b/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java @@ -28,15 +28,18 @@ import io.grpc.ChannelLogger; import io.grpc.EquivalentAddressGroup; import io.grpc.ExperimentalApi; +import io.grpc.ForwardingChannelBuilder.SimpleForwardingChannelBuilder; import io.grpc.HttpConnectProxiedSocketAddress; import io.grpc.Internal; -import io.grpc.internal.AbstractManagedChannelImplBuilder; import io.grpc.internal.AtomicBackoff; import io.grpc.internal.ClientTransportFactory; import io.grpc.internal.ConnectionClientTransport; import io.grpc.internal.FixedObjectPool; import io.grpc.internal.GrpcUtil; import io.grpc.internal.KeepAliveManager; +import io.grpc.internal.ManagedChannelImplBuilder; +import io.grpc.internal.ManagedChannelImplBuilder.ChannelBuilderDefaultPortProvider; +import io.grpc.internal.ManagedChannelImplBuilder.ClientTransportFactoryBuilder; import io.grpc.internal.ObjectPool; import io.grpc.internal.SharedResourcePool; import io.grpc.internal.TransportTracer; @@ -64,7 +67,7 @@ @ExperimentalApi("https://github.com/grpc/grpc-java/issues/1784") @CanIgnoreReturnValue public final class NettyChannelBuilder - extends AbstractManagedChannelImplBuilder { + extends SimpleForwardingChannelBuilder { // 1MiB. public static final int DEFAULT_FLOW_CONTROL_WINDOW = 1024 * 1024; @@ -76,6 +79,7 @@ public final class NettyChannelBuilder new ReflectiveChannelFactory<>(Utils.DEFAULT_CLIENT_CHANNEL_TYPE); private static final ObjectPool DEFAULT_EVENT_LOOP_GROUP_POOL = SharedResourcePool.forResource(Utils.DEFAULT_WORKER_EVENT_LOOP_GROUP); + protected TransportTracer.Factory transportTracerFactory = TransportTracer.getDefaultFactory(); static { String autoFlowControl = System.getenv("GRPC_EXPERIMENTAL_AUTOFLOWCONTROL"); @@ -85,6 +89,7 @@ public final class NettyChannelBuilder DEFAULT_AUTO_FLOW_CONTROL = Boolean.parseBoolean(autoFlowControl); } + private final ManagedChannelImplBuilder managedChannelImplBuilder; private final Map, Object> channelOptions = new HashMap<>(); @@ -145,6 +150,49 @@ public static NettyChannelBuilder forTarget(String target) { @CheckReturnValue NettyChannelBuilder(String target) { super(target); + + // An anonymous class to inject client transport factory builder. + final class NettyChannelTransportFactoryBuilder implements ClientTransportFactoryBuilder { + @CheckReturnValue + @Override + public ClientTransportFactory buildClientTransportFactory(int maxInboundMessageSize) { + assertEventLoopAndChannelType(); + + ProtocolNegotiator negotiator; + if (protocolNegotiatorFactory != null) { + negotiator = protocolNegotiatorFactory.buildProtocolNegotiator(); + } else { + SslContext localSslContext = sslContext; + if (negotiationType == NegotiationType.TLS && localSslContext == null) { + try { + localSslContext = GrpcSslContexts.forClient().build(); + } catch (SSLException ex) { + throw new RuntimeException(ex); + } + } + negotiator = createProtocolNegotiatorByType(negotiationType, localSslContext, + this.getOffloadExecutorPool()); + } + + return new NettyTransportFactory( + negotiator, channelFactory, channelOptions, + eventLoopGroupPool, autoFlowControl, flowControlWindow, maxInboundMessageSize, + maxHeaderListSize, keepAliveTimeNanos, keepAliveTimeoutNanos, keepAliveWithoutCalls, + transportTracerFactory, localSocketPicker, useGetForSafeMethods); + } + } + + // An anonymous class to provide ManagedChannelImplBuilder with the port getter. + final class NettyChannelDefaultPortProvider implements ChannelBuilderDefaultPortProvider { + @Override + public int getDefaultPort() { + return NettyChannelBuilder.this.getDefaultPort(); + } + } + + managedChannelImplBuilder = new ManagedChannelImplBuilder(target, + new NettyChannelTransportFactoryBuilder(), + new NettyChannelDefaultPortProvider()); } @CheckReturnValue @@ -152,6 +200,11 @@ public static NettyChannelBuilder forTarget(String target) { super(address, getAuthorityFromAddress(address)); } + @Override + protected ManagedChannelImplBuilder delegate() { + return managedChannelImplBuilder; + } + @CheckReturnValue private static String getAuthorityFromAddress(SocketAddress address) { if (address instanceof InetSocketAddress) { @@ -408,35 +461,6 @@ public SocketAddress createSocketAddress( } } - @Override - @CheckReturnValue - @Internal - protected ClientTransportFactory buildTransportFactory() { - assertEventLoopAndChannelType(); - - ProtocolNegotiator negotiator; - if (protocolNegotiatorFactory != null) { - negotiator = protocolNegotiatorFactory.buildProtocolNegotiator(); - } else { - SslContext localSslContext = sslContext; - if (negotiationType == NegotiationType.TLS && localSslContext == null) { - try { - localSslContext = GrpcSslContexts.forClient().build(); - } catch (SSLException ex) { - throw new RuntimeException(ex); - } - } - negotiator = createProtocolNegotiatorByType(negotiationType, localSslContext, - this.getOffloadExecutorPool()); - } - - return new NettyTransportFactory( - negotiator, channelFactory, channelOptions, - eventLoopGroupPool, autoFlowControl, flowControlWindow, maxInboundMessageSize(), - maxHeaderListSize, keepAliveTimeNanos, keepAliveTimeoutNanos, keepAliveWithoutCalls, - transportTracerFactory, localSocketPicker, useGetForSafeMethods); - } - @VisibleForTesting void assertEventLoopAndChannelType() { boolean bothProvided = channelFactory != DEFAULT_CHANNEL_FACTORY @@ -448,7 +472,6 @@ void assertEventLoopAndChannelType() { "Both EventLoopGroup and ChannelType should be provided or neither should be"); } - @Override @CheckReturnValue protected int getDefaultPort() { switch (negotiationType) { diff --git a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java index a50527f82e4..05a384fb666 100644 --- a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java +++ b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java @@ -171,7 +171,7 @@ public ClientTransportFactory buildClientTransportFactory(int maxInboundMessageS } } - // An anonymous class to inject client transport factory builder. + // An anonymous class to provide ManagedChannelImplBuilder with the port getter. final class OkHttpChannelDefaultPortProvider implements ChannelBuilderDefaultPortProvider { @Override public int getDefaultPort() { From 825379d7e8bb725c866325817950ff439d5d70ac Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Tue, 18 Aug 2020 11:15:47 -0400 Subject: [PATCH 18/54] Netty: update SocketAddress constructor --- .../io/grpc/netty/NettyChannelBuilder.java | 77 ++++++++++--------- 1 file changed, 40 insertions(+), 37 deletions(-) diff --git a/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java b/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java index a8508d67bb8..cb4a01df06c 100644 --- a/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java +++ b/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java @@ -147,49 +147,48 @@ public static NettyChannelBuilder forTarget(String target) { this(GrpcUtil.authorityFromHostAndPort(host, port)); } - @CheckReturnValue - NettyChannelBuilder(String target) { - super(target); - - // An anonymous class to inject client transport factory builder. - final class NettyChannelTransportFactoryBuilder implements ClientTransportFactoryBuilder { - @CheckReturnValue - @Override - public ClientTransportFactory buildClientTransportFactory(int maxInboundMessageSize) { - assertEventLoopAndChannelType(); - - ProtocolNegotiator negotiator; - if (protocolNegotiatorFactory != null) { - negotiator = protocolNegotiatorFactory.buildProtocolNegotiator(); - } else { - SslContext localSslContext = sslContext; - if (negotiationType == NegotiationType.TLS && localSslContext == null) { - try { - localSslContext = GrpcSslContexts.forClient().build(); - } catch (SSLException ex) { - throw new RuntimeException(ex); - } + /** An anonymous class to inject client transport factory builder. */ + final private class NettyChannelTransportFactoryBuilder implements ClientTransportFactoryBuilder { + @CheckReturnValue + @Override + public ClientTransportFactory buildClientTransportFactory(int maxInboundMessageSize) { + assertEventLoopAndChannelType(); + + ProtocolNegotiator negotiator; + if (protocolNegotiatorFactory != null) { + negotiator = protocolNegotiatorFactory.buildProtocolNegotiator(); + } else { + SslContext localSslContext = sslContext; + if (negotiationType == NegotiationType.TLS && localSslContext == null) { + try { + localSslContext = GrpcSslContexts.forClient().build(); + } catch (SSLException ex) { + throw new RuntimeException(ex); } - negotiator = createProtocolNegotiatorByType(negotiationType, localSslContext, - this.getOffloadExecutorPool()); } - - return new NettyTransportFactory( - negotiator, channelFactory, channelOptions, - eventLoopGroupPool, autoFlowControl, flowControlWindow, maxInboundMessageSize, - maxHeaderListSize, keepAliveTimeNanos, keepAliveTimeoutNanos, keepAliveWithoutCalls, - transportTracerFactory, localSocketPicker, useGetForSafeMethods); + negotiator = createProtocolNegotiatorByType(negotiationType, localSslContext, + this.getOffloadExecutorPool()); } + + return new NettyTransportFactory( + negotiator, channelFactory, channelOptions, + eventLoopGroupPool, autoFlowControl, flowControlWindow, maxInboundMessageSize, + maxHeaderListSize, keepAliveTimeNanos, keepAliveTimeoutNanos, keepAliveWithoutCalls, + transportTracerFactory, localSocketPicker, useGetForSafeMethods); } + } - // An anonymous class to provide ManagedChannelImplBuilder with the port getter. - final class NettyChannelDefaultPortProvider implements ChannelBuilderDefaultPortProvider { - @Override - public int getDefaultPort() { - return NettyChannelBuilder.this.getDefaultPort(); - } + /** An anonymous class to provide ManagedChannelImplBuilder with the port getter. */ + final class NettyChannelDefaultPortProvider implements ChannelBuilderDefaultPortProvider { + @Override + public int getDefaultPort() { + return NettyChannelBuilder.this.getDefaultPort(); } + } + @CheckReturnValue + NettyChannelBuilder(String target) { + super(); managedChannelImplBuilder = new ManagedChannelImplBuilder(target, new NettyChannelTransportFactoryBuilder(), new NettyChannelDefaultPortProvider()); @@ -197,7 +196,11 @@ public int getDefaultPort() { @CheckReturnValue NettyChannelBuilder(SocketAddress address) { - super(address, getAuthorityFromAddress(address)); + super(); + managedChannelImplBuilder = new ManagedChannelImplBuilder(address, + getAuthorityFromAddress(address), + new NettyChannelTransportFactoryBuilder(), + new NettyChannelDefaultPortProvider()); } @Override From 60a7d0a682088596b54bf57252a61c7aa4eb5804 Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Tue, 18 Aug 2020 11:22:37 -0400 Subject: [PATCH 19/54] Remove unsused import --- okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java | 1 - 1 file changed, 1 deletion(-) diff --git a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java index 05a384fb666..497a72c967d 100644 --- a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java +++ b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java @@ -26,7 +26,6 @@ import io.grpc.ExperimentalApi; import io.grpc.ForwardingChannelBuilder.SimpleForwardingChannelBuilder; import io.grpc.Internal; -import io.grpc.ManagedChannel; import io.grpc.internal.AtomicBackoff; import io.grpc.internal.ClientTransportFactory; import io.grpc.internal.ConnectionClientTransport; From f98e67351c111354f274d7a45f4307a218f05e7f Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Tue, 18 Aug 2020 12:20:20 -0400 Subject: [PATCH 20/54] Comments from 1:1 w/Eric --- .../internal/ManagedChannelImplBuilder.java | 34 ++++++------------- .../netty/InternalNettyServerBuilder.java | 2 ++ .../io/grpc/netty/NettyChannelBuilder.java | 4 +-- .../io/grpc/okhttp/OkHttpChannelBuilder.java | 9 +++-- .../io/grpc/okhttp/OkHttpTransportTest.java | 1 + 5 files changed, 22 insertions(+), 28 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java index 4fe63ff02d8..bb4ead5ec4d 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java @@ -20,7 +20,7 @@ import javax.annotation.Nullable; // TODO(sergiitk): We won't be able to override checkAuthority anymore - bury it here -public class ManagedChannelImplBuilder +public final class ManagedChannelImplBuilder extends AbstractManagedChannelImplBuilder { public interface ClientTransportFactoryBuilder { @@ -32,47 +32,35 @@ public interface ChannelBuilderDefaultPortProvider { } private final ClientTransportFactoryBuilder clientTransportFactoryBuilder; - @Nullable private final ChannelBuilderDefaultPortProvider channelBuilderDefaultPortProvider; + // see if this is used, otherwise just put + private final ChannelBuilderDefaultPortProvider channelBuilderDefaultPortProvider; private int defaultPort; - /** - * Creates a new builder for the given target that will be resolved by - * {@link io.grpc.NameResolver}. - */ - public ManagedChannelImplBuilder(String target, - ClientTransportFactoryBuilder clientTransportFactoryBuilder) { - // TODO(sergiitk): finish javadoc - this(target, clientTransportFactoryBuilder, null); - } - /** * Creates a new builder for the given target that will be resolved by * {@link io.grpc.NameResolver}. */ public ManagedChannelImplBuilder(String target, ClientTransportFactoryBuilder clientTransportFactoryBuilder, - @Nullable ChannelBuilderDefaultPortProvider channelBuilderDefaultPortProvider) { + ChannelBuilderDefaultPortProvider channelBuilderDefaultPortProvider) { // TODO(sergiitk): finish javadoc super(target); this.clientTransportFactoryBuilder = clientTransportFactoryBuilder; this.channelBuilderDefaultPortProvider = channelBuilderDefaultPortProvider; - this.defaultPort = super.getDefaultPort(); - } - - public ManagedChannelImplBuilder(SocketAddress directServerAddress, String authority, - ClientTransportFactoryBuilder clientTransportFactoryBuilder) { - // TODO(sergiitk): finish javadoc - this(directServerAddress, authority, clientTransportFactoryBuilder, null); +// this.defaultPort = GrpcUtil.DEFAULT_PORT_SSL; } public ManagedChannelImplBuilder(SocketAddress directServerAddress, String authority, ClientTransportFactoryBuilder clientTransportFactoryBuilder, - @Nullable ChannelBuilderDefaultPortProvider channelBuilderDefaultPortProvider) { + @Nullable ChannelBuilderDefaultPortProvider channelBuilderDefaultPortProvider) { // TODO(sergiitk): finish javadoc + // checknotnull. see okhttp:391 + // also Preconditions.checkArgument + // Preconditions.checknotnull super(directServerAddress, authority); this.clientTransportFactoryBuilder = clientTransportFactoryBuilder; this.channelBuilderDefaultPortProvider = channelBuilderDefaultPortProvider; - this.defaultPort = super.getDefaultPort(); +// this.defaultPort = super.getDefaultPort(); } @Override @@ -85,6 +73,6 @@ protected ClientTransportFactory buildTransportFactory() { @Override protected int getDefaultPort() { - return defaultPort; + return channelBuilderDefaultPortProvider.getDefaultPort(); } } diff --git a/netty/src/main/java/io/grpc/netty/InternalNettyServerBuilder.java b/netty/src/main/java/io/grpc/netty/InternalNettyServerBuilder.java index e89593364b3..5da4b62bb56 100644 --- a/netty/src/main/java/io/grpc/netty/InternalNettyServerBuilder.java +++ b/netty/src/main/java/io/grpc/netty/InternalNettyServerBuilder.java @@ -47,6 +47,8 @@ public static void setForceHeapBuffer(NettyServerBuilder builder, boolean value) builder.setForceHeapBuffer(value); } + // set tracer here + /** * Sets {@link io.grpc.Channel} and {@link io.netty.channel.EventLoopGroup}s to Nio. A major * benefit over using existing setters is gRPC will manage the life cycle of {@link diff --git a/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java b/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java index cb4a01df06c..ba2e062a957 100644 --- a/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java +++ b/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java @@ -147,7 +147,6 @@ public static NettyChannelBuilder forTarget(String target) { this(GrpcUtil.authorityFromHostAndPort(host, port)); } - /** An anonymous class to inject client transport factory builder. */ final private class NettyChannelTransportFactoryBuilder implements ClientTransportFactoryBuilder { @CheckReturnValue @Override @@ -178,8 +177,7 @@ public ClientTransportFactory buildClientTransportFactory(int maxInboundMessageS } } - /** An anonymous class to provide ManagedChannelImplBuilder with the port getter. */ - final class NettyChannelDefaultPortProvider implements ChannelBuilderDefaultPortProvider { + final private class NettyChannelDefaultPortProvider implements ChannelBuilderDefaultPortProvider { @Override public int getDefaultPort() { return NettyChannelBuilder.this.getDefaultPort(); diff --git a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java index 497a72c967d..971490b3496 100644 --- a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java +++ b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java @@ -26,6 +26,7 @@ import io.grpc.ExperimentalApi; import io.grpc.ForwardingChannelBuilder.SimpleForwardingChannelBuilder; import io.grpc.Internal; +import io.grpc.ManagedChannelBuilder; import io.grpc.internal.AtomicBackoff; import io.grpc.internal.ClientTransportFactory; import io.grpc.internal.ConnectionClientTransport; @@ -60,7 +61,7 @@ public class OkHttpChannelBuilder extends SimpleForwardingChannelBuilder { public static final int DEFAULT_FLOW_CONTROL_WINDOW = 65535; - protected TransportTracer.Factory transportTracerFactory = TransportTracer.getDefaultFactory(); + private TransportTracer.Factory transportTracerFactory = TransportTracer.getDefaultFactory(); private final ManagedChannelImplBuilder managedChannelImplBuilder; /** Identifies the negotiation used for starting up HTTP/2. */ @@ -150,6 +151,8 @@ private OkHttpChannelBuilder(String target) { final class OkHttpChannelTransportFactoryBuilder implements ClientTransportFactoryBuilder { @Override public ClientTransportFactory buildClientTransportFactory(int maxInboundMessageSize) { + // move back to the method, make it package-private + // reimplement maxInboundMessageSize in each builder boolean enableKeepAlive = keepAliveTimeNanos != KEEPALIVE_TIME_NANOS_DISABLED; return new OkHttpTransportFactory( transportExecutor, @@ -183,8 +186,9 @@ public int getDefaultPort() { new OkHttpChannelDefaultPortProvider()); } + @Internal @Override - protected ManagedChannelImplBuilder delegate() { + protected final ManagedChannelBuilder delegate() { return managedChannelImplBuilder; } @@ -409,6 +413,7 @@ public OkHttpChannelBuilder maxInboundMetadataSize(int bytes) { } protected int getDefaultPort() { + // move into the inject thing switch (negotiationType) { case PLAINTEXT: return GrpcUtil.DEFAULT_PORT_PLAINTEXT; diff --git a/okhttp/src/test/java/io/grpc/okhttp/OkHttpTransportTest.java b/okhttp/src/test/java/io/grpc/okhttp/OkHttpTransportTest.java index ac6b1122b90..ede392dfb06 100644 --- a/okhttp/src/test/java/io/grpc/okhttp/OkHttpTransportTest.java +++ b/okhttp/src/test/java/io/grpc/okhttp/OkHttpTransportTest.java @@ -53,6 +53,7 @@ public void releaseClientFactory() { @Override protected List newServer( List streamTracerFactories) { + // replace with internal netty server builder return AccessProtectedHack.serverBuilderBuildTransportServer( NettyServerBuilder .forPort(0) From 7a5ea9364ec8feb3a4bfe06a9af57068a10f1097 Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Tue, 18 Aug 2020 14:34:24 -0400 Subject: [PATCH 21/54] Organize todos --- .../io/grpc/internal/ManagedChannelImplBuilder.java | 10 ++++------ .../java/io/grpc/netty/InternalNettyServerBuilder.java | 2 +- .../main/java/io/grpc/okhttp/OkHttpChannelBuilder.java | 4 ++-- .../java/io/grpc/internal/AccessProtectedHack.java | 1 + .../test/java/io/grpc/okhttp/OkHttpTransportTest.java | 2 +- 5 files changed, 9 insertions(+), 10 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java index bb4ead5ec4d..56c6765136b 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java @@ -32,31 +32,29 @@ public interface ChannelBuilderDefaultPortProvider { } private final ClientTransportFactoryBuilder clientTransportFactoryBuilder; - // see if this is used, otherwise just put + // TODO(sergiitk): see where getDefaultPort() not overridden, Might be in InProcess private final ChannelBuilderDefaultPortProvider channelBuilderDefaultPortProvider; private int defaultPort; /** * Creates a new builder for the given target that will be resolved by * {@link io.grpc.NameResolver}. + * TODO(sergiitk): finish javadoc */ public ManagedChannelImplBuilder(String target, ClientTransportFactoryBuilder clientTransportFactoryBuilder, ChannelBuilderDefaultPortProvider channelBuilderDefaultPortProvider) { - // TODO(sergiitk): finish javadoc super(target); this.clientTransportFactoryBuilder = clientTransportFactoryBuilder; this.channelBuilderDefaultPortProvider = channelBuilderDefaultPortProvider; // this.defaultPort = GrpcUtil.DEFAULT_PORT_SSL; } + //* TODO(sergiitk): finish javadoc */ public ManagedChannelImplBuilder(SocketAddress directServerAddress, String authority, ClientTransportFactoryBuilder clientTransportFactoryBuilder, @Nullable ChannelBuilderDefaultPortProvider channelBuilderDefaultPortProvider) { - // TODO(sergiitk): finish javadoc - // checknotnull. see okhttp:391 - // also Preconditions.checkArgument - // Preconditions.checknotnull + // TODO(sergiitk): setup checknotnull. see okhttp:391 Preconditions.checkArgument Preconditions.checknotnull super(directServerAddress, authority); this.clientTransportFactoryBuilder = clientTransportFactoryBuilder; this.channelBuilderDefaultPortProvider = channelBuilderDefaultPortProvider; diff --git a/netty/src/main/java/io/grpc/netty/InternalNettyServerBuilder.java b/netty/src/main/java/io/grpc/netty/InternalNettyServerBuilder.java index 5da4b62bb56..42da95b70f2 100644 --- a/netty/src/main/java/io/grpc/netty/InternalNettyServerBuilder.java +++ b/netty/src/main/java/io/grpc/netty/InternalNettyServerBuilder.java @@ -47,7 +47,7 @@ public static void setForceHeapBuffer(NettyServerBuilder builder, boolean value) builder.setForceHeapBuffer(value); } - // set tracer here + // TODO(sergiitk): set tracer here /** * Sets {@link io.grpc.Channel} and {@link io.netty.channel.EventLoopGroup}s to Nio. A major diff --git a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java index 971490b3496..0a0df439241 100644 --- a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java +++ b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java @@ -151,8 +151,8 @@ private OkHttpChannelBuilder(String target) { final class OkHttpChannelTransportFactoryBuilder implements ClientTransportFactoryBuilder { @Override public ClientTransportFactory buildClientTransportFactory(int maxInboundMessageSize) { - // move back to the method, make it package-private - // reimplement maxInboundMessageSize in each builder + // TODO(sergiitk): move back to the method, make it package-private + // TODO(sergiitk): reimplement maxInboundMessageSize in each builder boolean enableKeepAlive = keepAliveTimeNanos != KEEPALIVE_TIME_NANOS_DISABLED; return new OkHttpTransportFactory( transportExecutor, diff --git a/okhttp/src/test/java/io/grpc/internal/AccessProtectedHack.java b/okhttp/src/test/java/io/grpc/internal/AccessProtectedHack.java index 6b53d821731..698494d6d1f 100644 --- a/okhttp/src/test/java/io/grpc/internal/AccessProtectedHack.java +++ b/okhttp/src/test/java/io/grpc/internal/AccessProtectedHack.java @@ -19,6 +19,7 @@ import io.grpc.ServerStreamTracer; import java.util.List; +// TODO(sergiitk): remove /** A hack to access protected methods from io.grpc.internal. */ public final class AccessProtectedHack { public static List serverBuilderBuildTransportServer( diff --git a/okhttp/src/test/java/io/grpc/okhttp/OkHttpTransportTest.java b/okhttp/src/test/java/io/grpc/okhttp/OkHttpTransportTest.java index ede392dfb06..943a6c688ea 100644 --- a/okhttp/src/test/java/io/grpc/okhttp/OkHttpTransportTest.java +++ b/okhttp/src/test/java/io/grpc/okhttp/OkHttpTransportTest.java @@ -53,7 +53,7 @@ public void releaseClientFactory() { @Override protected List newServer( List streamTracerFactories) { - // replace with internal netty server builder + // TODO(sergiitk): replace AccessProtectedHack with io.grpc.netty.InternalNettyServerBuilder return AccessProtectedHack.serverBuilderBuildTransportServer( NettyServerBuilder .forPort(0) From 54cc1b386681824a8d092c5a34af7702d57edd89 Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Tue, 18 Aug 2020 15:09:02 -0400 Subject: [PATCH 22/54] Make channelBuilderDefaultPortProvider required --- .../java/io/grpc/internal/ManagedChannelImplBuilder.java | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java index 56c6765136b..026f98d9c44 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java @@ -17,7 +17,6 @@ package io.grpc.internal; import java.net.SocketAddress; -import javax.annotation.Nullable; // TODO(sergiitk): We won't be able to override checkAuthority anymore - bury it here public final class ManagedChannelImplBuilder @@ -34,7 +33,6 @@ public interface ChannelBuilderDefaultPortProvider { private final ClientTransportFactoryBuilder clientTransportFactoryBuilder; // TODO(sergiitk): see where getDefaultPort() not overridden, Might be in InProcess private final ChannelBuilderDefaultPortProvider channelBuilderDefaultPortProvider; - private int defaultPort; /** * Creates a new builder for the given target that will be resolved by @@ -53,19 +51,15 @@ public ManagedChannelImplBuilder(String target, //* TODO(sergiitk): finish javadoc */ public ManagedChannelImplBuilder(SocketAddress directServerAddress, String authority, ClientTransportFactoryBuilder clientTransportFactoryBuilder, - @Nullable ChannelBuilderDefaultPortProvider channelBuilderDefaultPortProvider) { + ChannelBuilderDefaultPortProvider channelBuilderDefaultPortProvider) { // TODO(sergiitk): setup checknotnull. see okhttp:391 Preconditions.checkArgument Preconditions.checknotnull super(directServerAddress, authority); this.clientTransportFactoryBuilder = clientTransportFactoryBuilder; this.channelBuilderDefaultPortProvider = channelBuilderDefaultPortProvider; -// this.defaultPort = super.getDefaultPort(); } @Override protected ClientTransportFactory buildTransportFactory() { - if (channelBuilderDefaultPortProvider != null) { - defaultPort = channelBuilderDefaultPortProvider.getDefaultPort(); - } return clientTransportFactoryBuilder.buildClientTransportFactory(maxInboundMessageSize()); } From 18cc303340b8f8ad83df7045569118a9bca449e3 Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Tue, 18 Aug 2020 15:29:22 -0400 Subject: [PATCH 23/54] Add Preconditions.checkNotNull --- .../internal/ManagedChannelImplBuilder.java | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java index 026f98d9c44..e9cb2887a35 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java @@ -16,6 +16,9 @@ package io.grpc.internal; +import static com.google.common.base.Preconditions.checkNotNull; + +import com.google.common.base.Preconditions; import java.net.SocketAddress; // TODO(sergiitk): We won't be able to override checkAuthority anymore - bury it here @@ -43,19 +46,21 @@ public ManagedChannelImplBuilder(String target, ClientTransportFactoryBuilder clientTransportFactoryBuilder, ChannelBuilderDefaultPortProvider channelBuilderDefaultPortProvider) { super(target); - this.clientTransportFactoryBuilder = clientTransportFactoryBuilder; - this.channelBuilderDefaultPortProvider = channelBuilderDefaultPortProvider; -// this.defaultPort = GrpcUtil.DEFAULT_PORT_SSL; + this.clientTransportFactoryBuilder = checkNotNull(clientTransportFactoryBuilder, + "clientTransportFactoryBuilder cannot be null"); + this.channelBuilderDefaultPortProvider = checkNotNull(channelBuilderDefaultPortProvider, + "channelBuilderDefaultPortProvider cannot be null"); } //* TODO(sergiitk): finish javadoc */ public ManagedChannelImplBuilder(SocketAddress directServerAddress, String authority, ClientTransportFactoryBuilder clientTransportFactoryBuilder, ChannelBuilderDefaultPortProvider channelBuilderDefaultPortProvider) { - // TODO(sergiitk): setup checknotnull. see okhttp:391 Preconditions.checkArgument Preconditions.checknotnull super(directServerAddress, authority); - this.clientTransportFactoryBuilder = clientTransportFactoryBuilder; - this.channelBuilderDefaultPortProvider = channelBuilderDefaultPortProvider; + this.clientTransportFactoryBuilder = checkNotNull(clientTransportFactoryBuilder, + "clientTransportFactoryBuilder cannot be null"); + this.channelBuilderDefaultPortProvider = checkNotNull(channelBuilderDefaultPortProvider, + "channelBuilderDefaultPortProvider cannot be null"); } @Override From 7b99c970649c9d66cb695613077f6d9d995a9969 Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Tue, 18 Aug 2020 15:49:03 -0400 Subject: [PATCH 24/54] Temp Revert NettyChannelBuilder --- .../io/grpc/netty/NettyChannelBuilder.java | 92 +++++++------------ 1 file changed, 34 insertions(+), 58 deletions(-) diff --git a/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java b/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java index ba2e062a957..59e86a117da 100644 --- a/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java +++ b/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java @@ -28,18 +28,15 @@ import io.grpc.ChannelLogger; import io.grpc.EquivalentAddressGroup; import io.grpc.ExperimentalApi; -import io.grpc.ForwardingChannelBuilder.SimpleForwardingChannelBuilder; import io.grpc.HttpConnectProxiedSocketAddress; import io.grpc.Internal; +import io.grpc.internal.AbstractManagedChannelImplBuilder; import io.grpc.internal.AtomicBackoff; import io.grpc.internal.ClientTransportFactory; import io.grpc.internal.ConnectionClientTransport; import io.grpc.internal.FixedObjectPool; import io.grpc.internal.GrpcUtil; import io.grpc.internal.KeepAliveManager; -import io.grpc.internal.ManagedChannelImplBuilder; -import io.grpc.internal.ManagedChannelImplBuilder.ChannelBuilderDefaultPortProvider; -import io.grpc.internal.ManagedChannelImplBuilder.ClientTransportFactoryBuilder; import io.grpc.internal.ObjectPool; import io.grpc.internal.SharedResourcePool; import io.grpc.internal.TransportTracer; @@ -67,7 +64,7 @@ @ExperimentalApi("https://github.com/grpc/grpc-java/issues/1784") @CanIgnoreReturnValue public final class NettyChannelBuilder - extends SimpleForwardingChannelBuilder { + extends AbstractManagedChannelImplBuilder { // 1MiB. public static final int DEFAULT_FLOW_CONTROL_WINDOW = 1024 * 1024; @@ -79,7 +76,6 @@ public final class NettyChannelBuilder new ReflectiveChannelFactory<>(Utils.DEFAULT_CLIENT_CHANNEL_TYPE); private static final ObjectPool DEFAULT_EVENT_LOOP_GROUP_POOL = SharedResourcePool.forResource(Utils.DEFAULT_WORKER_EVENT_LOOP_GROUP); - protected TransportTracer.Factory transportTracerFactory = TransportTracer.getDefaultFactory(); static { String autoFlowControl = System.getenv("GRPC_EXPERIMENTAL_AUTOFLOWCONTROL"); @@ -89,7 +85,6 @@ public final class NettyChannelBuilder DEFAULT_AUTO_FLOW_CONTROL = Boolean.parseBoolean(autoFlowControl); } - private final ManagedChannelImplBuilder managedChannelImplBuilder; private final Map, Object> channelOptions = new HashMap<>(); @@ -147,63 +142,14 @@ public static NettyChannelBuilder forTarget(String target) { this(GrpcUtil.authorityFromHostAndPort(host, port)); } - final private class NettyChannelTransportFactoryBuilder implements ClientTransportFactoryBuilder { - @CheckReturnValue - @Override - public ClientTransportFactory buildClientTransportFactory(int maxInboundMessageSize) { - assertEventLoopAndChannelType(); - - ProtocolNegotiator negotiator; - if (protocolNegotiatorFactory != null) { - negotiator = protocolNegotiatorFactory.buildProtocolNegotiator(); - } else { - SslContext localSslContext = sslContext; - if (negotiationType == NegotiationType.TLS && localSslContext == null) { - try { - localSslContext = GrpcSslContexts.forClient().build(); - } catch (SSLException ex) { - throw new RuntimeException(ex); - } - } - negotiator = createProtocolNegotiatorByType(negotiationType, localSslContext, - this.getOffloadExecutorPool()); - } - - return new NettyTransportFactory( - negotiator, channelFactory, channelOptions, - eventLoopGroupPool, autoFlowControl, flowControlWindow, maxInboundMessageSize, - maxHeaderListSize, keepAliveTimeNanos, keepAliveTimeoutNanos, keepAliveWithoutCalls, - transportTracerFactory, localSocketPicker, useGetForSafeMethods); - } - } - - final private class NettyChannelDefaultPortProvider implements ChannelBuilderDefaultPortProvider { - @Override - public int getDefaultPort() { - return NettyChannelBuilder.this.getDefaultPort(); - } - } - @CheckReturnValue NettyChannelBuilder(String target) { - super(); - managedChannelImplBuilder = new ManagedChannelImplBuilder(target, - new NettyChannelTransportFactoryBuilder(), - new NettyChannelDefaultPortProvider()); + super(target); } @CheckReturnValue NettyChannelBuilder(SocketAddress address) { - super(); - managedChannelImplBuilder = new ManagedChannelImplBuilder(address, - getAuthorityFromAddress(address), - new NettyChannelTransportFactoryBuilder(), - new NettyChannelDefaultPortProvider()); - } - - @Override - protected ManagedChannelImplBuilder delegate() { - return managedChannelImplBuilder; + super(address, getAuthorityFromAddress(address)); } @CheckReturnValue @@ -462,6 +408,35 @@ public SocketAddress createSocketAddress( } } + @Override + @CheckReturnValue + @Internal + protected ClientTransportFactory buildTransportFactory() { + assertEventLoopAndChannelType(); + + ProtocolNegotiator negotiator; + if (protocolNegotiatorFactory != null) { + negotiator = protocolNegotiatorFactory.buildProtocolNegotiator(); + } else { + SslContext localSslContext = sslContext; + if (negotiationType == NegotiationType.TLS && localSslContext == null) { + try { + localSslContext = GrpcSslContexts.forClient().build(); + } catch (SSLException ex) { + throw new RuntimeException(ex); + } + } + negotiator = createProtocolNegotiatorByType(negotiationType, localSslContext, + this.getOffloadExecutorPool()); + } + + return new NettyTransportFactory( + negotiator, channelFactory, channelOptions, + eventLoopGroupPool, autoFlowControl, flowControlWindow, maxInboundMessageSize(), + maxHeaderListSize, keepAliveTimeNanos, keepAliveTimeoutNanos, keepAliveWithoutCalls, + transportTracerFactory, localSocketPicker, useGetForSafeMethods); + } + @VisibleForTesting void assertEventLoopAndChannelType() { boolean bothProvided = channelFactory != DEFAULT_CHANNEL_FACTORY @@ -473,6 +448,7 @@ void assertEventLoopAndChannelType() { "Both EventLoopGroup and ChannelType should be provided or neither should be"); } + @Override @CheckReturnValue protected int getDefaultPort() { switch (negotiationType) { From 0b87ee98695cdc3ac044b4e38c1671f0256a4a45 Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Tue, 18 Aug 2020 16:56:34 -0400 Subject: [PATCH 25/54] Make buildTransportFactory() package-private, reimplement maxInboundMessageSize --- .../internal/ManagedChannelImplBuilder.java | 6 +- .../io/grpc/okhttp/OkHttpChannelBuilder.java | 59 ++++++++++++------- 2 files changed, 41 insertions(+), 24 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java index e9cb2887a35..40785bdf6c7 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java @@ -26,7 +26,7 @@ public final class ManagedChannelImplBuilder extends AbstractManagedChannelImplBuilder { public interface ClientTransportFactoryBuilder { - ClientTransportFactory buildClientTransportFactory(int maxInboundMessageSize); + ClientTransportFactory buildClientTransportFactory(); } public interface ChannelBuilderDefaultPortProvider { @@ -52,7 +52,7 @@ public ManagedChannelImplBuilder(String target, "channelBuilderDefaultPortProvider cannot be null"); } - //* TODO(sergiitk): finish javadoc */ + /** TODO(sergiitk): finish javadoc */ public ManagedChannelImplBuilder(SocketAddress directServerAddress, String authority, ClientTransportFactoryBuilder clientTransportFactoryBuilder, ChannelBuilderDefaultPortProvider channelBuilderDefaultPortProvider) { @@ -65,7 +65,7 @@ public ManagedChannelImplBuilder(SocketAddress directServerAddress, String autho @Override protected ClientTransportFactory buildTransportFactory() { - return clientTransportFactoryBuilder.buildClientTransportFactory(maxInboundMessageSize()); + return clientTransportFactoryBuilder.buildClientTransportFactory(); } @Override diff --git a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java index 0a0df439241..deeeee51166 100644 --- a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java +++ b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java @@ -61,8 +61,9 @@ public class OkHttpChannelBuilder extends SimpleForwardingChannelBuilder { public static final int DEFAULT_FLOW_CONTROL_WINDOW = 65535; - private TransportTracer.Factory transportTracerFactory = TransportTracer.getDefaultFactory(); private final ManagedChannelImplBuilder managedChannelImplBuilder; + private TransportTracer.Factory transportTracerFactory = TransportTracer.getDefaultFactory(); + private int maxInboundMessageSize = GrpcUtil.DEFAULT_MAX_MESSAGE_SIZE; /** Identifies the negotiation used for starting up HTTP/2. */ private enum NegotiationType { @@ -150,26 +151,8 @@ private OkHttpChannelBuilder(String target) { // An anonymous class to inject client transport factory builder. final class OkHttpChannelTransportFactoryBuilder implements ClientTransportFactoryBuilder { @Override - public ClientTransportFactory buildClientTransportFactory(int maxInboundMessageSize) { - // TODO(sergiitk): move back to the method, make it package-private - // TODO(sergiitk): reimplement maxInboundMessageSize in each builder - boolean enableKeepAlive = keepAliveTimeNanos != KEEPALIVE_TIME_NANOS_DISABLED; - return new OkHttpTransportFactory( - transportExecutor, - scheduledExecutorService, - socketFactory, - createSslSocketFactory(), - hostnameVerifier, - connectionSpec, - maxInboundMessageSize, - enableKeepAlive, - keepAliveTimeNanos, - keepAliveTimeoutNanos, - flowControlWindow, - keepAliveWithoutCalls, - maxInboundMetadataSize, - transportTracerFactory, - useGetForSafeMethods); + public ClientTransportFactory buildClientTransportFactory() { + return buildTransportFactory(); } } @@ -412,6 +395,40 @@ public OkHttpChannelBuilder maxInboundMetadataSize(int bytes) { return this; } + /** + * Sets the maximum message size allowed for a single gRPC frame. If an inbound messages + * larger than this limit is received it will not be processed and the RPC will fail with + * RESOURCE_EXHAUSTED. + */ + // Can be overridden by subclasses. + @Override + public OkHttpChannelBuilder maxInboundMessageSize(int max) { + Preconditions.checkArgument(max >= 0, "negative max"); + maxInboundMessageSize = max; + return this; + } + + @Internal + final ClientTransportFactory buildTransportFactory() { + boolean enableKeepAlive = keepAliveTimeNanos != KEEPALIVE_TIME_NANOS_DISABLED; + return new OkHttpTransportFactory( + transportExecutor, + scheduledExecutorService, + socketFactory, + createSslSocketFactory(), + hostnameVerifier, + connectionSpec, + maxInboundMessageSize, + enableKeepAlive, + keepAliveTimeNanos, + keepAliveTimeoutNanos, + flowControlWindow, + keepAliveWithoutCalls, + maxInboundMetadataSize, + transportTracerFactory, + useGetForSafeMethods); + } + protected int getDefaultPort() { // move into the inject thing switch (negotiationType) { From 006792bde134c1be966a215f3696f08a10e27551 Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Tue, 18 Aug 2020 17:58:41 -0400 Subject: [PATCH 26/54] Hide getDefaultPort() --- .../io/grpc/okhttp/OkHttpChannelBuilder.java | 21 +++++++------------ 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java index deeeee51166..4eed3685545 100644 --- a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java +++ b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java @@ -160,7 +160,14 @@ public ClientTransportFactory buildClientTransportFactory() { final class OkHttpChannelDefaultPortProvider implements ChannelBuilderDefaultPortProvider { @Override public int getDefaultPort() { - return OkHttpChannelBuilder.this.getDefaultPort(); + switch (negotiationType) { + case PLAINTEXT: + return GrpcUtil.DEFAULT_PORT_PLAINTEXT; + case TLS: + return GrpcUtil.DEFAULT_PORT_SSL; + default: + throw new AssertionError(negotiationType + " not handled"); + } } } @@ -429,18 +436,6 @@ final ClientTransportFactory buildTransportFactory() { useGetForSafeMethods); } - protected int getDefaultPort() { - // move into the inject thing - switch (negotiationType) { - case PLAINTEXT: - return GrpcUtil.DEFAULT_PORT_PLAINTEXT; - case TLS: - return GrpcUtil.DEFAULT_PORT_SSL; - default: - throw new AssertionError(negotiationType + " not handled"); - } - } - @VisibleForTesting @Nullable SSLSocketFactory createSslSocketFactory() { From 9eda883f769836134d80f9ed4fb99ccd50bbab44 Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Tue, 18 Aug 2020 18:34:08 -0400 Subject: [PATCH 27/54] Make getDefaultPort() package-private for testing --- .../io/grpc/okhttp/OkHttpChannelBuilder.java | 22 ++++++++++++------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java index 4eed3685545..c5b35bc8f66 100644 --- a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java +++ b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java @@ -160,14 +160,7 @@ public ClientTransportFactory buildClientTransportFactory() { final class OkHttpChannelDefaultPortProvider implements ChannelBuilderDefaultPortProvider { @Override public int getDefaultPort() { - switch (negotiationType) { - case PLAINTEXT: - return GrpcUtil.DEFAULT_PORT_PLAINTEXT; - case TLS: - return GrpcUtil.DEFAULT_PORT_SSL; - default: - throw new AssertionError(negotiationType + " not handled"); - } + return OkHttpChannelBuilder.this.getDefaultPort(); } } @@ -436,6 +429,19 @@ final ClientTransportFactory buildTransportFactory() { useGetForSafeMethods); } + @VisibleForTesting + int getDefaultPort() { + // move into the inject thing + switch (negotiationType) { + case PLAINTEXT: + return GrpcUtil.DEFAULT_PORT_PLAINTEXT; + case TLS: + return GrpcUtil.DEFAULT_PORT_SSL; + default: + throw new AssertionError(negotiationType + " not handled"); + } + } + @VisibleForTesting @Nullable SSLSocketFactory createSslSocketFactory() { From 41c5afbcb1c2d3757d66be1cab4b0364b84f5aa2 Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Wed, 19 Aug 2020 14:22:42 -0400 Subject: [PATCH 28/54] disableCheckAuthority() instead of overriding checkAuthority() --- .../internal/ManagedChannelImplBuilder.java | 22 ++++++++++++++++- .../io/grpc/okhttp/OkHttpChannelBuilder.java | 15 +++++++++--- .../grpc/okhttp/OkHttpChannelBuilderTest.java | 24 ++++++++++++------- 3 files changed, 48 insertions(+), 13 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java index 40785bdf6c7..ff0594c500c 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java @@ -18,17 +18,20 @@ import static com.google.common.base.Preconditions.checkNotNull; -import com.google.common.base.Preconditions; import java.net.SocketAddress; // TODO(sergiitk): We won't be able to override checkAuthority anymore - bury it here public final class ManagedChannelImplBuilder extends AbstractManagedChannelImplBuilder { + private boolean authorityCheckerDisabled; + + /** TODO(sergiitk): finish javadoc */ public interface ClientTransportFactoryBuilder { ClientTransportFactory buildClientTransportFactory(); } + /** TODO(sergiitk): finish javadoc */ public interface ChannelBuilderDefaultPortProvider { int getDefaultPort(); } @@ -63,6 +66,23 @@ public ManagedChannelImplBuilder(SocketAddress directServerAddress, String autho "channelBuilderDefaultPortProvider cannot be null"); } + /** TODO(sergiitk): finish javadoc */ + public ManagedChannelImplBuilder disableCheckAuthority() { + authorityCheckerDisabled = true; + return this; + } + + /** TODO(sergiitk): finish javadoc */ + public ManagedChannelImplBuilder enableCheckAuthority() { + authorityCheckerDisabled = false; + return this; + } + + @Override + protected String checkAuthority(String authority) { + return authorityCheckerDisabled ? authority : super.checkAuthority(authority); + } + @Override protected ClientTransportFactory buildTransportFactory() { return clientTransportFactoryBuilder.buildClientTransportFactory(); diff --git a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java index c5b35bc8f66..37c25e31e3c 100644 --- a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java +++ b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java @@ -63,7 +63,7 @@ public class OkHttpChannelBuilder extends SimpleForwardingChannelBuilder Date: Wed, 19 Aug 2020 15:07:53 -0400 Subject: [PATCH 29/54] Start NettyChannelBuilder refactoring --- .../io/grpc/netty/NettyChannelBuilder.java | 68 +++++++++++++++---- .../io/grpc/okhttp/OkHttpChannelBuilder.java | 1 - 2 files changed, 55 insertions(+), 14 deletions(-) diff --git a/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java b/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java index 59e86a117da..1eb7e199b6a 100644 --- a/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java +++ b/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java @@ -23,20 +23,25 @@ import static io.grpc.internal.GrpcUtil.KEEPALIVE_TIME_NANOS_DISABLED; import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.Preconditions; import com.google.errorprone.annotations.CanIgnoreReturnValue; import io.grpc.Attributes; import io.grpc.ChannelLogger; import io.grpc.EquivalentAddressGroup; import io.grpc.ExperimentalApi; +import io.grpc.ForwardingChannelBuilder.SimpleForwardingChannelBuilder; import io.grpc.HttpConnectProxiedSocketAddress; import io.grpc.Internal; -import io.grpc.internal.AbstractManagedChannelImplBuilder; +import io.grpc.ManagedChannelBuilder; import io.grpc.internal.AtomicBackoff; import io.grpc.internal.ClientTransportFactory; import io.grpc.internal.ConnectionClientTransport; import io.grpc.internal.FixedObjectPool; import io.grpc.internal.GrpcUtil; import io.grpc.internal.KeepAliveManager; +import io.grpc.internal.ManagedChannelImplBuilder; +import io.grpc.internal.ManagedChannelImplBuilder.ChannelBuilderDefaultPortProvider; +import io.grpc.internal.ManagedChannelImplBuilder.ClientTransportFactoryBuilder; import io.grpc.internal.ObjectPool; import io.grpc.internal.SharedResourcePool; import io.grpc.internal.TransportTracer; @@ -63,8 +68,7 @@ */ @ExperimentalApi("https://github.com/grpc/grpc-java/issues/1784") @CanIgnoreReturnValue -public final class NettyChannelBuilder - extends AbstractManagedChannelImplBuilder { +public final class NettyChannelBuilder extends SimpleForwardingChannelBuilder { // 1MiB. public static final int DEFAULT_FLOW_CONTROL_WINDOW = 1024 * 1024; @@ -85,9 +89,9 @@ public final class NettyChannelBuilder DEFAULT_AUTO_FLOW_CONTROL = Boolean.parseBoolean(autoFlowControl); } - private final Map, Object> channelOptions = - new HashMap<>(); - + private final ManagedChannelImplBuilder managedChannelImplBuilder; + private TransportTracer.Factory transportTracerFactory = TransportTracer.getDefaultFactory(); + private final Map, Object> channelOptions = new HashMap<>(); private NegotiationType negotiationType = NegotiationType.TLS; private OverrideAuthorityChecker authorityChecker; private ChannelFactory channelFactory = DEFAULT_CHANNEL_FACTORY; @@ -95,6 +99,7 @@ public final class NettyChannelBuilder private SslContext sslContext; private boolean autoFlowControl = DEFAULT_AUTO_FLOW_CONTROL; private int flowControlWindow = DEFAULT_FLOW_CONTROL_WINDOW; + private int maxInboundMessageSize = GrpcUtil.DEFAULT_MAX_MESSAGE_SIZE; private int maxHeaderListSize = GrpcUtil.DEFAULT_MAX_HEADER_LIST_SIZE; private long keepAliveTimeNanos = KEEPALIVE_TIME_NANOS_DISABLED; private long keepAliveTimeoutNanos = DEFAULT_KEEPALIVE_TIMEOUT_NANOS; @@ -142,14 +147,42 @@ public static NettyChannelBuilder forTarget(String target) { this(GrpcUtil.authorityFromHostAndPort(host, port)); } + final private class NettyChannelTransportFactoryBuilder implements ClientTransportFactoryBuilder { + @CheckReturnValue + @Override + public ClientTransportFactory buildClientTransportFactory() { + return NettyChannelBuilder.this.buildTransportFactory(); + } + } + + final private class NettyChannelDefaultPortProvider implements ChannelBuilderDefaultPortProvider { + @Override + public int getDefaultPort() { + return NettyChannelBuilder.this.getDefaultPort(); + } + } + @CheckReturnValue NettyChannelBuilder(String target) { - super(target); + super(); + managedChannelImplBuilder = new ManagedChannelImplBuilder(target, + new NettyChannelTransportFactoryBuilder(), + new NettyChannelDefaultPortProvider()); } @CheckReturnValue NettyChannelBuilder(SocketAddress address) { - super(address, getAuthorityFromAddress(address)); + super(); + managedChannelImplBuilder = new ManagedChannelImplBuilder(address, + getAuthorityFromAddress(address), + new NettyChannelTransportFactoryBuilder(), + new NettyChannelDefaultPortProvider()); + } + + @Internal + @Override + protected ManagedChannelBuilder delegate() { + return managedChannelImplBuilder; } @CheckReturnValue @@ -408,10 +441,20 @@ public SocketAddress createSocketAddress( } } + /** + * Sets the maximum message size allowed for a single gRPC frame. If an inbound messages larger + * than this limit is received it will not be processed and the RPC will fail with + * RESOURCE_EXHAUSTED. + */ @Override + public NettyChannelBuilder maxInboundMessageSize(int max) { + Preconditions.checkArgument(max >= 0, "negative max"); + maxInboundMessageSize = max; + return this; + } + @CheckReturnValue - @Internal - protected ClientTransportFactory buildTransportFactory() { + ClientTransportFactory buildTransportFactory() { assertEventLoopAndChannelType(); ProtocolNegotiator negotiator; @@ -432,7 +475,7 @@ protected ClientTransportFactory buildTransportFactory() { return new NettyTransportFactory( negotiator, channelFactory, channelOptions, - eventLoopGroupPool, autoFlowControl, flowControlWindow, maxInboundMessageSize(), + eventLoopGroupPool, autoFlowControl, flowControlWindow, maxInboundMessageSize, maxHeaderListSize, keepAliveTimeNanos, keepAliveTimeoutNanos, keepAliveWithoutCalls, transportTracerFactory, localSocketPicker, useGetForSafeMethods); } @@ -448,9 +491,8 @@ void assertEventLoopAndChannelType() { "Both EventLoopGroup and ChannelType should be provided or neither should be"); } - @Override @CheckReturnValue - protected int getDefaultPort() { + int getDefaultPort() { switch (negotiationType) { case PLAINTEXT: case PLAINTEXT_UPGRADE: diff --git a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java index 37c25e31e3c..9e5ed7aded0 100644 --- a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java +++ b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java @@ -401,7 +401,6 @@ public OkHttpChannelBuilder maxInboundMetadataSize(int bytes) { * larger than this limit is received it will not be processed and the RPC will fail with * RESOURCE_EXHAUSTED. */ - // Can be overridden by subclasses. @Override public OkHttpChannelBuilder maxInboundMessageSize(int max) { Preconditions.checkArgument(max >= 0, "negative max"); From a35d78610e13f0f5cfb205cfcff0d020e6dcaefd Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Wed, 19 Aug 2020 15:30:54 -0400 Subject: [PATCH 30/54] Fix getOffloadExecutorPool reference in NettyChannelBuilder --- .../java/io/grpc/internal/ManagedChannelImplBuilder.java | 6 ++++++ netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java index ff0594c500c..b4714b70ec9 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java @@ -19,6 +19,7 @@ import static com.google.common.base.Preconditions.checkNotNull; import java.net.SocketAddress; +import java.util.concurrent.Executor; // TODO(sergiitk): We won't be able to override checkAuthority anymore - bury it here public final class ManagedChannelImplBuilder @@ -78,6 +79,11 @@ public ManagedChannelImplBuilder enableCheckAuthority() { return this; } + @Override + public ObjectPool getOffloadExecutorPool() { + return super.getOffloadExecutorPool(); + } + @Override protected String checkAuthority(String authority) { return authorityCheckerDisabled ? authority : super.checkAuthority(authority); diff --git a/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java b/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java index 1eb7e199b6a..85134e79d96 100644 --- a/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java +++ b/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java @@ -470,7 +470,7 @@ ClientTransportFactory buildTransportFactory() { } } negotiator = createProtocolNegotiatorByType(negotiationType, localSslContext, - this.getOffloadExecutorPool()); + this.managedChannelImplBuilder.getOffloadExecutorPool()); } return new NettyTransportFactory( From ec8d8510a3f8c5ede73ed51d2de791d3caa7daed Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Wed, 19 Aug 2020 15:58:09 -0400 Subject: [PATCH 31/54] Replace checkAuthority and overrideAuthorityChecker with disableCheckAuthority() --- .../netty/InternalNettyChannelBuilder.java | 13 ++++------ .../io/grpc/netty/NettyChannelBuilder.java | 24 ++++++------------- .../grpc/netty/NettyChannelBuilderTest.java | 10 +++----- .../io/grpc/okhttp/OkHttpChannelBuilder.java | 1 - 4 files changed, 15 insertions(+), 33 deletions(-) diff --git a/netty/src/main/java/io/grpc/netty/InternalNettyChannelBuilder.java b/netty/src/main/java/io/grpc/netty/InternalNettyChannelBuilder.java index e66f28cac73..efcf3fda81e 100644 --- a/netty/src/main/java/io/grpc/netty/InternalNettyChannelBuilder.java +++ b/netty/src/main/java/io/grpc/netty/InternalNettyChannelBuilder.java @@ -28,15 +28,12 @@ @Internal public final class InternalNettyChannelBuilder { - /** - * Checks authority upon channel construction. The purpose of this interface is to raise the - * visibility of {@link NettyChannelBuilder.OverrideAuthorityChecker}. - */ - public interface OverrideAuthorityChecker extends NettyChannelBuilder.OverrideAuthorityChecker {} + public static void disableCheckAuthority(NettyChannelBuilder builder) { + builder.disableCheckAuthority(); + } - public static void overrideAuthorityChecker( - NettyChannelBuilder channelBuilder, OverrideAuthorityChecker authorityChecker) { - channelBuilder.overrideAuthorityChecker(authorityChecker); + public static void enableCheckAuthority(NettyChannelBuilder builder) { + builder.enableCheckAuthority(); } /** A class that provides a Netty handler to control protocol negotiation. */ diff --git a/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java b/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java index 85134e79d96..88bf6198c92 100644 --- a/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java +++ b/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java @@ -93,7 +93,6 @@ public final class NettyChannelBuilder extends SimpleForwardingChannelBuilder, Object> channelOptions = new HashMap<>(); private NegotiationType negotiationType = NegotiationType.TLS; - private OverrideAuthorityChecker authorityChecker; private ChannelFactory channelFactory = DEFAULT_CHANNEL_FACTORY; private ObjectPool eventLoopGroupPool = DEFAULT_EVENT_LOOP_GROUP_POOL; private SslContext sslContext; @@ -454,7 +453,7 @@ public NettyChannelBuilder maxInboundMessageSize(int max) { } @CheckReturnValue - ClientTransportFactory buildTransportFactory() { + final ClientTransportFactory buildTransportFactory() { assertEventLoopAndChannelType(); ProtocolNegotiator negotiator; @@ -504,10 +503,6 @@ int getDefaultPort() { } } - void overrideAuthorityChecker(@Nullable OverrideAuthorityChecker authorityChecker) { - this.authorityChecker = authorityChecker; - } - @VisibleForTesting @CheckReturnValue static ProtocolNegotiator createProtocolNegotiatorByType( @@ -526,19 +521,14 @@ static ProtocolNegotiator createProtocolNegotiatorByType( } } - @CheckReturnValue - interface OverrideAuthorityChecker { - String checkAuthority(String authority); + NettyChannelBuilder disableCheckAuthority() { + this.managedChannelImplBuilder.disableCheckAuthority(); + return this; } - @Override - @CheckReturnValue - @Internal - protected String checkAuthority(String authority) { - if (authorityChecker != null) { - return authorityChecker.checkAuthority(authority); - } - return super.checkAuthority(authority); + NettyChannelBuilder enableCheckAuthority() { + this.managedChannelImplBuilder.enableCheckAuthority(); + return this; } void protocolNegotiatorFactory(ProtocolNegotiatorFactory protocolNegotiatorFactory) { diff --git a/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java b/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java index 3621e6e2454..03db2867af2 100644 --- a/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java +++ b/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java @@ -22,7 +22,6 @@ import static org.mockito.Mockito.mock; import io.grpc.ManagedChannel; -import io.grpc.netty.InternalNettyChannelBuilder.OverrideAuthorityChecker; import io.grpc.netty.NettyTestUtil.TrackingObjectPoolForTest; import io.netty.channel.Channel; import io.netty.channel.ChannelFactory; @@ -94,17 +93,14 @@ private void overrideAuthorityIsReadableHelper(NettyChannelBuilder builder, @Test public void overrideAllowsInvalidAuthority() { NettyChannelBuilder builder = new NettyChannelBuilder(new SocketAddress(){}); - InternalNettyChannelBuilder.overrideAuthorityChecker(builder, new OverrideAuthorityChecker() { - @Override - public String checkAuthority(String authority) { - return authority; - } - }); + InternalNettyChannelBuilder.disableCheckAuthority(builder); Object unused = builder.overrideAuthority("[invalidauthority") .negotiationType(NegotiationType.PLAINTEXT) .buildTransportFactory(); } + // TODO(sergiitk): Add test for enableCheckAuthority() + @Test public void failOverrideInvalidAuthority() { NettyChannelBuilder builder = new NettyChannelBuilder(new SocketAddress(){}); diff --git a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java index 9e5ed7aded0..ab3ddc6343f 100644 --- a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java +++ b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java @@ -408,7 +408,6 @@ public OkHttpChannelBuilder maxInboundMessageSize(int max) { return this; } - @Internal final ClientTransportFactory buildTransportFactory() { boolean enableKeepAlive = keepAliveTimeNanos != KEEPALIVE_TIME_NANOS_DISABLED; return new OkHttpTransportFactory( From a1c91562664757f46dbe8c9a86a604f82d4ad50b Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Wed, 19 Aug 2020 16:19:20 -0400 Subject: [PATCH 32/54] NettyChannelBuilder builds! --- .../internal/ManagedChannelImplBuilder.java | 33 ++++++++++++++++-- .../netty/InternalNettyChannelBuilder.java | 4 +++ .../io/grpc/netty/NettyChannelBuilder.java | 34 ++++++++++--------- 3 files changed, 53 insertions(+), 18 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java index b4714b70ec9..a5aa2728fd8 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java @@ -67,18 +67,47 @@ public ManagedChannelImplBuilder(SocketAddress directServerAddress, String autho "channelBuilderDefaultPortProvider cannot be null"); } - /** TODO(sergiitk): finish javadoc */ + /** + * TODO(sergiitk): finish javadoc + */ public ManagedChannelImplBuilder disableCheckAuthority() { authorityCheckerDisabled = true; return this; } - /** TODO(sergiitk): finish javadoc */ + /** + * TODO(sergiitk): finish javadoc + */ public ManagedChannelImplBuilder enableCheckAuthority() { authorityCheckerDisabled = false; return this; } + @Override + public void setStatsEnabled(boolean value) { + super.setStatsEnabled(value); + } + + @Override + public void setStatsRecordStartedRpcs(boolean value) { + super.setStatsRecordStartedRpcs(value); + } + + @Override + public void setStatsRecordFinishedRpcs(boolean value) { + super.setStatsRecordFinishedRpcs(value); + } + + @Override + public void setStatsRecordRealTimeMetrics(boolean value) { + super.setStatsRecordRealTimeMetrics(value); + } + + @Override + public void setTracingEnabled(boolean value) { + super.setTracingEnabled(value); + } + @Override public ObjectPool getOffloadExecutorPool() { return super.getOffloadExecutorPool(); diff --git a/netty/src/main/java/io/grpc/netty/InternalNettyChannelBuilder.java b/netty/src/main/java/io/grpc/netty/InternalNettyChannelBuilder.java index efcf3fda81e..ff4d4074ecb 100644 --- a/netty/src/main/java/io/grpc/netty/InternalNettyChannelBuilder.java +++ b/netty/src/main/java/io/grpc/netty/InternalNettyChannelBuilder.java @@ -65,6 +65,10 @@ public static void setStatsRecordStartedRpcs(NettyChannelBuilder builder, boolea builder.setStatsRecordStartedRpcs(value); } + public static void setStatsRecordFinishedRpcs(NettyChannelBuilder builder, boolean value) { + builder.setStatsRecordFinishedRpcs(value); + } + public static void setStatsRecordRealTimeMetrics(NettyChannelBuilder builder, boolean value) { builder.setStatsRecordRealTimeMetrics(value); } diff --git a/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java b/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java index 88bf6198c92..7a94189822e 100644 --- a/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java +++ b/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java @@ -146,7 +146,8 @@ public static NettyChannelBuilder forTarget(String target) { this(GrpcUtil.authorityFromHostAndPort(host, port)); } - final private class NettyChannelTransportFactoryBuilder implements ClientTransportFactoryBuilder { + private final class NettyChannelTransportFactoryBuilder implements ClientTransportFactoryBuilder { + @CheckReturnValue @Override public ClientTransportFactory buildClientTransportFactory() { @@ -154,7 +155,8 @@ public ClientTransportFactory buildClientTransportFactory() { } } - final private class NettyChannelDefaultPortProvider implements ChannelBuilderDefaultPortProvider { + private final class NettyChannelDefaultPortProvider implements ChannelBuilderDefaultPortProvider { + @Override public int getDefaultPort() { return NettyChannelBuilder.this.getDefaultPort(); @@ -521,12 +523,12 @@ static ProtocolNegotiator createProtocolNegotiatorByType( } } - NettyChannelBuilder disableCheckAuthority() { + NettyChannelBuilder disableCheckAuthority() { this.managedChannelImplBuilder.disableCheckAuthority(); return this; } - NettyChannelBuilder enableCheckAuthority() { + NettyChannelBuilder enableCheckAuthority() { this.managedChannelImplBuilder.enableCheckAuthority(); return this; } @@ -536,24 +538,24 @@ void protocolNegotiatorFactory(ProtocolNegotiatorFactory protocolNegotiatorFacto = checkNotNull(protocolNegotiatorFactory, "protocolNegotiatorFactory"); } - @Override - protected void setTracingEnabled(boolean value) { - super.setTracingEnabled(value); + void setTracingEnabled(boolean value) { + this.managedChannelImplBuilder.setTracingEnabled(value); } - @Override - protected void setStatsEnabled(boolean value) { - super.setStatsEnabled(value); + void setStatsEnabled(boolean value) { + this.managedChannelImplBuilder.setStatsEnabled(value); } - @Override - protected void setStatsRecordStartedRpcs(boolean value) { - super.setStatsRecordStartedRpcs(value); + void setStatsRecordStartedRpcs(boolean value) { + this.managedChannelImplBuilder.setStatsRecordStartedRpcs(value); } - @Override - protected void setStatsRecordRealTimeMetrics(boolean value) { - super.setStatsRecordRealTimeMetrics(value); + void setStatsRecordFinishedRpcs(boolean value) { + this.managedChannelImplBuilder.setStatsRecordFinishedRpcs(value); + } + + void setStatsRecordRealTimeMetrics(boolean value) { + this.managedChannelImplBuilder.setStatsRecordRealTimeMetrics(value); } @VisibleForTesting From 97956b9de68e8b127d35bc3d04ca6e973fbf0651 Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Wed, 19 Aug 2020 16:27:40 -0400 Subject: [PATCH 33/54] Fix ManagedChannelImplBuilder lint --- .../internal/ManagedChannelImplBuilder.java | 24 ++++++++++++------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java index a5aa2728fd8..6f5ffbfb57d 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java @@ -21,19 +21,24 @@ import java.net.SocketAddress; import java.util.concurrent.Executor; -// TODO(sergiitk): We won't be able to override checkAuthority anymore - bury it here public final class ManagedChannelImplBuilder extends AbstractManagedChannelImplBuilder { private boolean authorityCheckerDisabled; - /** TODO(sergiitk): finish javadoc */ + /** + * TODO(sergiitk): finish javadoc. + */ public interface ClientTransportFactoryBuilder { + ClientTransportFactory buildClientTransportFactory(); } - /** TODO(sergiitk): finish javadoc */ + /** + * TODO(sergiitk): finish javadoc. + */ public interface ChannelBuilderDefaultPortProvider { + int getDefaultPort(); } @@ -42,9 +47,8 @@ public interface ChannelBuilderDefaultPortProvider { private final ChannelBuilderDefaultPortProvider channelBuilderDefaultPortProvider; /** - * Creates a new builder for the given target that will be resolved by - * {@link io.grpc.NameResolver}. - * TODO(sergiitk): finish javadoc + * Creates a new builder for the given target that will be resolved by {@link + * io.grpc.NameResolver}. TODO(sergiitk): finish javadoc */ public ManagedChannelImplBuilder(String target, ClientTransportFactoryBuilder clientTransportFactoryBuilder, @@ -56,7 +60,9 @@ public ManagedChannelImplBuilder(String target, "channelBuilderDefaultPortProvider cannot be null"); } - /** TODO(sergiitk): finish javadoc */ + /** + * TODO(sergiitk): finish javadoc. + */ public ManagedChannelImplBuilder(SocketAddress directServerAddress, String authority, ClientTransportFactoryBuilder clientTransportFactoryBuilder, ChannelBuilderDefaultPortProvider channelBuilderDefaultPortProvider) { @@ -68,7 +74,7 @@ public ManagedChannelImplBuilder(SocketAddress directServerAddress, String autho } /** - * TODO(sergiitk): finish javadoc + * TODO(sergiitk): finish javadoc. */ public ManagedChannelImplBuilder disableCheckAuthority() { authorityCheckerDisabled = true; @@ -76,7 +82,7 @@ public ManagedChannelImplBuilder disableCheckAuthority() { } /** - * TODO(sergiitk): finish javadoc + * TODO(sergiitk): finish javadoc. */ public ManagedChannelImplBuilder enableCheckAuthority() { authorityCheckerDisabled = false; From 4ffddbbdcbc93a0584d5689bd4c00274c78f0a65 Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Wed, 19 Aug 2020 16:28:47 -0400 Subject: [PATCH 34/54] Minor --- .../io/grpc/internal/ManagedChannelImplBuilder.java | 12 +++++------- .../main/java/io/grpc/netty/NettyChannelBuilder.java | 5 +---- 2 files changed, 6 insertions(+), 11 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java index 6f5ffbfb57d..b8a8dd2d706 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java @@ -30,7 +30,6 @@ public final class ManagedChannelImplBuilder * TODO(sergiitk): finish javadoc. */ public interface ClientTransportFactoryBuilder { - ClientTransportFactory buildClientTransportFactory(); } @@ -38,7 +37,6 @@ public interface ClientTransportFactoryBuilder { * TODO(sergiitk): finish javadoc. */ public interface ChannelBuilderDefaultPortProvider { - int getDefaultPort(); } @@ -73,6 +71,11 @@ public ManagedChannelImplBuilder(SocketAddress directServerAddress, String autho "channelBuilderDefaultPortProvider cannot be null"); } + @Override + protected ClientTransportFactory buildTransportFactory() { + return clientTransportFactoryBuilder.buildClientTransportFactory(); + } + /** * TODO(sergiitk): finish javadoc. */ @@ -124,11 +127,6 @@ protected String checkAuthority(String authority) { return authorityCheckerDisabled ? authority : super.checkAuthority(authority); } - @Override - protected ClientTransportFactory buildTransportFactory() { - return clientTransportFactoryBuilder.buildClientTransportFactory(); - } - @Override protected int getDefaultPort() { return channelBuilderDefaultPortProvider.getDefaultPort(); diff --git a/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java b/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java index 7a94189822e..f10324fdb6d 100644 --- a/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java +++ b/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java @@ -147,16 +147,13 @@ public static NettyChannelBuilder forTarget(String target) { } private final class NettyChannelTransportFactoryBuilder implements ClientTransportFactoryBuilder { - - @CheckReturnValue @Override public ClientTransportFactory buildClientTransportFactory() { - return NettyChannelBuilder.this.buildTransportFactory(); + return buildTransportFactory(); } } private final class NettyChannelDefaultPortProvider implements ChannelBuilderDefaultPortProvider { - @Override public int getDefaultPort() { return NettyChannelBuilder.this.getDefaultPort(); From dc884427e01763b1dfb5b90bbef538a866c60e50 Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Thu, 20 Aug 2020 11:52:03 -0400 Subject: [PATCH 35/54] Remove comments relevant to the server-side refactoring --- .../src/main/java/io/grpc/netty/InternalNettyServerBuilder.java | 2 -- okhttp/src/test/java/io/grpc/internal/AccessProtectedHack.java | 1 - okhttp/src/test/java/io/grpc/okhttp/OkHttpTransportTest.java | 1 - 3 files changed, 4 deletions(-) diff --git a/netty/src/main/java/io/grpc/netty/InternalNettyServerBuilder.java b/netty/src/main/java/io/grpc/netty/InternalNettyServerBuilder.java index 42da95b70f2..e89593364b3 100644 --- a/netty/src/main/java/io/grpc/netty/InternalNettyServerBuilder.java +++ b/netty/src/main/java/io/grpc/netty/InternalNettyServerBuilder.java @@ -47,8 +47,6 @@ public static void setForceHeapBuffer(NettyServerBuilder builder, boolean value) builder.setForceHeapBuffer(value); } - // TODO(sergiitk): set tracer here - /** * Sets {@link io.grpc.Channel} and {@link io.netty.channel.EventLoopGroup}s to Nio. A major * benefit over using existing setters is gRPC will manage the life cycle of {@link diff --git a/okhttp/src/test/java/io/grpc/internal/AccessProtectedHack.java b/okhttp/src/test/java/io/grpc/internal/AccessProtectedHack.java index 698494d6d1f..6b53d821731 100644 --- a/okhttp/src/test/java/io/grpc/internal/AccessProtectedHack.java +++ b/okhttp/src/test/java/io/grpc/internal/AccessProtectedHack.java @@ -19,7 +19,6 @@ import io.grpc.ServerStreamTracer; import java.util.List; -// TODO(sergiitk): remove /** A hack to access protected methods from io.grpc.internal. */ public final class AccessProtectedHack { public static List serverBuilderBuildTransportServer( diff --git a/okhttp/src/test/java/io/grpc/okhttp/OkHttpTransportTest.java b/okhttp/src/test/java/io/grpc/okhttp/OkHttpTransportTest.java index 943a6c688ea..ac6b1122b90 100644 --- a/okhttp/src/test/java/io/grpc/okhttp/OkHttpTransportTest.java +++ b/okhttp/src/test/java/io/grpc/okhttp/OkHttpTransportTest.java @@ -53,7 +53,6 @@ public void releaseClientFactory() { @Override protected List newServer( List streamTracerFactories) { - // TODO(sergiitk): replace AccessProtectedHack with io.grpc.netty.InternalNettyServerBuilder return AccessProtectedHack.serverBuilderBuildTransportServer( NettyServerBuilder .forPort(0) From 66ac3a4389b34e19533dc04fac75fc4488463a53 Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Thu, 20 Aug 2020 13:45:49 -0400 Subject: [PATCH 36/54] NettyChannelBuilder: Add test for enableCheckAuthority() --- .../grpc/netty/NettyChannelBuilderTest.java | 24 +++++++++++++------ 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java b/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java index 03db2867af2..16a5f3c3823 100644 --- a/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java +++ b/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java @@ -91,23 +91,33 @@ private void overrideAuthorityIsReadableHelper(NettyChannelBuilder builder, } @Test - public void overrideAllowsInvalidAuthority() { + public void failOverrideInvalidAuthority() { NettyChannelBuilder builder = new NettyChannelBuilder(new SocketAddress(){}); - InternalNettyChannelBuilder.disableCheckAuthority(builder); + + thrown.expect(IllegalArgumentException.class); + thrown.expectMessage("Invalid authority:"); + + builder.overrideAuthority("[invalidauthority"); + } + + @Test + public void disableCheckAuthorityAllowsInvalidAuthority() { + NettyChannelBuilder builder = new NettyChannelBuilder(new SocketAddress() {}) + .disableCheckAuthority(); + Object unused = builder.overrideAuthority("[invalidauthority") .negotiationType(NegotiationType.PLAINTEXT) .buildTransportFactory(); } - // TODO(sergiitk): Add test for enableCheckAuthority() - @Test - public void failOverrideInvalidAuthority() { - NettyChannelBuilder builder = new NettyChannelBuilder(new SocketAddress(){}); + public void enableCheckAuthorityFailOverrideInvalidAuthority() { + NettyChannelBuilder builder = new NettyChannelBuilder(new SocketAddress() {}) + .disableCheckAuthority() + .enableCheckAuthority(); thrown.expect(IllegalArgumentException.class); thrown.expectMessage("Invalid authority:"); - builder.overrideAuthority("[invalidauthority"); } From 5277dbc4f4539f99121ab57beac7621a8bc1e57f Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Thu, 20 Aug 2020 14:44:41 -0400 Subject: [PATCH 37/54] Bring back NettyChannelBuilder.overrideAuthorityChecker(), but mark for deprecation --- .../internal/ManagedChannelImplBuilder.java | 28 +++++++++++--- .../netty/InternalNettyChannelBuilder.java | 20 ++++++++++ .../io/grpc/netty/NettyChannelBuilder.java | 11 +++++- .../grpc/netty/NettyChannelBuilderTest.java | 37 ++++++++++++++++++- 4 files changed, 87 insertions(+), 9 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java index b8a8dd2d706..0a3e785f5cd 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java @@ -20,11 +20,13 @@ import java.net.SocketAddress; import java.util.concurrent.Executor; +import javax.annotation.Nullable; public final class ManagedChannelImplBuilder extends AbstractManagedChannelImplBuilder { private boolean authorityCheckerDisabled; + @Deprecated @Nullable private OverrideAuthorityChecker authorityChecker; /** * TODO(sergiitk): finish javadoc. @@ -92,6 +94,27 @@ public ManagedChannelImplBuilder enableCheckAuthority() { return this; } + @Deprecated + public interface OverrideAuthorityChecker { + String checkAuthority(String authority); + } + + @Deprecated + public void overrideAuthorityChecker(@Nullable OverrideAuthorityChecker authorityChecker) { + this.authorityChecker = authorityChecker; + } + + @Override + protected String checkAuthority(String authority) { + if (authorityCheckerDisabled) { + return authority; + } + if (authorityChecker != null) { + return authorityChecker.checkAuthority(authority); + } + return super.checkAuthority(authority); + } + @Override public void setStatsEnabled(boolean value) { super.setStatsEnabled(value); @@ -122,11 +145,6 @@ public ObjectPool getOffloadExecutorPool() { return super.getOffloadExecutorPool(); } - @Override - protected String checkAuthority(String authority) { - return authorityCheckerDisabled ? authority : super.checkAuthority(authority); - } - @Override protected int getDefaultPort() { return channelBuilderDefaultPortProvider.getDefaultPort(); diff --git a/netty/src/main/java/io/grpc/netty/InternalNettyChannelBuilder.java b/netty/src/main/java/io/grpc/netty/InternalNettyChannelBuilder.java index ff4d4074ecb..29423389193 100644 --- a/netty/src/main/java/io/grpc/netty/InternalNettyChannelBuilder.java +++ b/netty/src/main/java/io/grpc/netty/InternalNettyChannelBuilder.java @@ -28,6 +28,26 @@ @Internal public final class InternalNettyChannelBuilder { + /** + * Checks authority upon channel construction. The purpose of this interface is to raise the + * visibility of {@link NettyChannelBuilder.OverrideAuthorityChecker}. + * @deprecated To be removed, use {@link #disableCheckAuthority(NettyChannelBuilder builder)} to + * disable authority check. + */ + @Deprecated + public interface OverrideAuthorityChecker extends NettyChannelBuilder.OverrideAuthorityChecker {} + + /** + * Overrides authority checker. + * @deprecated To be removed, use {@link #disableCheckAuthority(NettyChannelBuilder builder)} to + * disable authority check. + */ + @Deprecated + public static void overrideAuthorityChecker( + NettyChannelBuilder channelBuilder, OverrideAuthorityChecker authorityChecker) { + channelBuilder.overrideAuthorityChecker(authorityChecker); + } + public static void disableCheckAuthority(NettyChannelBuilder builder) { builder.disableCheckAuthority(); } diff --git a/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java b/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java index f10324fdb6d..5e64a5e3237 100644 --- a/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java +++ b/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java @@ -23,7 +23,6 @@ import static io.grpc.internal.GrpcUtil.KEEPALIVE_TIME_NANOS_DISABLED; import com.google.common.annotations.VisibleForTesting; -import com.google.common.base.Preconditions; import com.google.errorprone.annotations.CanIgnoreReturnValue; import io.grpc.Attributes; import io.grpc.ChannelLogger; @@ -446,7 +445,7 @@ public SocketAddress createSocketAddress( */ @Override public NettyChannelBuilder maxInboundMessageSize(int max) { - Preconditions.checkArgument(max >= 0, "negative max"); + checkArgument(max >= 0, "negative max"); maxInboundMessageSize = max; return this; } @@ -520,6 +519,14 @@ static ProtocolNegotiator createProtocolNegotiatorByType( } } + @Deprecated + interface OverrideAuthorityChecker extends ManagedChannelImplBuilder.OverrideAuthorityChecker {} + + @Deprecated + void overrideAuthorityChecker(@Nullable OverrideAuthorityChecker authorityChecker) { + this.managedChannelImplBuilder.overrideAuthorityChecker(authorityChecker); + } + NettyChannelBuilder disableCheckAuthority() { this.managedChannelImplBuilder.disableCheckAuthority(); return this; diff --git a/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java b/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java index 16a5f3c3823..fe753eed63a 100644 --- a/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java +++ b/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java @@ -22,6 +22,8 @@ import static org.mockito.Mockito.mock; import io.grpc.ManagedChannel; +import io.grpc.internal.GrpcUtil; +import io.grpc.netty.InternalNettyChannelBuilder.OverrideAuthorityChecker; import io.grpc.netty.NettyTestUtil.TrackingObjectPoolForTest; import io.netty.channel.Channel; import io.netty.channel.ChannelFactory; @@ -90,6 +92,37 @@ private void overrideAuthorityIsReadableHelper(NettyChannelBuilder builder, } } + @Test + public void overrideAllowsInvalidAuthority() { + NettyChannelBuilder builder = new NettyChannelBuilder(new SocketAddress(){}); + InternalNettyChannelBuilder.overrideAuthorityChecker(builder, new OverrideAuthorityChecker() { + @Override + public String checkAuthority(String authority) { + return authority; + } + }); + Object unused = builder.overrideAuthority("[invalidauthority") + .negotiationType(NegotiationType.PLAINTEXT) + .buildTransportFactory(); + } + + @Test + public void overrideFailsInvalidAuthority() { + NettyChannelBuilder builder = new NettyChannelBuilder(new SocketAddress(){}); + InternalNettyChannelBuilder.overrideAuthorityChecker(builder, new OverrideAuthorityChecker() { + @Override + public String checkAuthority(String authority) { + return GrpcUtil.checkAuthority(authority); + } + }); + + thrown.expect(IllegalArgumentException.class); + thrown.expectMessage("Invalid authority:"); + Object unused = builder.overrideAuthority("[invalidauthority") + .negotiationType(NegotiationType.PLAINTEXT) + .buildTransportFactory(); + } + @Test public void failOverrideInvalidAuthority() { NettyChannelBuilder builder = new NettyChannelBuilder(new SocketAddress(){}); @@ -102,7 +135,7 @@ public void failOverrideInvalidAuthority() { @Test public void disableCheckAuthorityAllowsInvalidAuthority() { - NettyChannelBuilder builder = new NettyChannelBuilder(new SocketAddress() {}) + NettyChannelBuilder builder = new NettyChannelBuilder(new SocketAddress(){}) .disableCheckAuthority(); Object unused = builder.overrideAuthority("[invalidauthority") @@ -112,7 +145,7 @@ public void disableCheckAuthorityAllowsInvalidAuthority() { @Test public void enableCheckAuthorityFailOverrideInvalidAuthority() { - NettyChannelBuilder builder = new NettyChannelBuilder(new SocketAddress() {}) + NettyChannelBuilder builder = new NettyChannelBuilder(new SocketAddress(){}) .disableCheckAuthority() .enableCheckAuthority(); From 964ef77e738d46175d73d3983fcf9ccaa2be729b Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Thu, 20 Aug 2020 17:21:11 -0400 Subject: [PATCH 38/54] Refactor InProcessChannelBuilder! --- .../inprocess/InProcessChannelBuilder.java | 35 +++++++++++++---- .../internal/ManagedChannelImplBuilder.java | 38 +++++++++++++------ 2 files changed, 53 insertions(+), 20 deletions(-) diff --git a/core/src/main/java/io/grpc/inprocess/InProcessChannelBuilder.java b/core/src/main/java/io/grpc/inprocess/InProcessChannelBuilder.java index 3546e33357c..431c10e9650 100644 --- a/core/src/main/java/io/grpc/inprocess/InProcessChannelBuilder.java +++ b/core/src/main/java/io/grpc/inprocess/InProcessChannelBuilder.java @@ -21,11 +21,14 @@ import io.grpc.ChannelLogger; import io.grpc.ExperimentalApi; +import io.grpc.ForwardingChannelBuilder.SimpleForwardingChannelBuilder; import io.grpc.Internal; -import io.grpc.internal.AbstractManagedChannelImplBuilder; +import io.grpc.ManagedChannelBuilder; import io.grpc.internal.ClientTransportFactory; import io.grpc.internal.ConnectionClientTransport; import io.grpc.internal.GrpcUtil; +import io.grpc.internal.ManagedChannelImplBuilder; +import io.grpc.internal.ManagedChannelImplBuilder.ClientTransportFactoryBuilder; import io.grpc.internal.SharedResourceHolder; import java.net.SocketAddress; import java.util.concurrent.ScheduledExecutorService; @@ -42,7 +45,7 @@ */ @ExperimentalApi("https://github.com/grpc/grpc-java/issues/1783") public final class InProcessChannelBuilder extends - AbstractManagedChannelImplBuilder { + SimpleForwardingChannelBuilder { /** * Create a channel builder that will connect to the server with the given name. * @@ -67,18 +70,36 @@ public static InProcessChannelBuilder forAddress(String name, int port) { throw new UnsupportedOperationException("call forName() instead"); } + private final ManagedChannelImplBuilder managedChannelImplBuilder; private final String name; private ScheduledExecutorService scheduledExecutorService; private int maxInboundMetadataSize = Integer.MAX_VALUE; private boolean transportIncludeStatusCause = false; private InProcessChannelBuilder(String name) { - super(new InProcessSocketAddress(name), "localhost"); + super(); this.name = checkNotNull(name, "name"); + + final class InProcessChannelTransportFactoryBuilder implements ClientTransportFactoryBuilder { + @Override + public ClientTransportFactory buildClientTransportFactory() { + return buildTransportFactory(); + } + } + + managedChannelImplBuilder = new ManagedChannelImplBuilder(new InProcessSocketAddress(name), + "localhost", new InProcessChannelTransportFactoryBuilder(), null); + // In-process transport should not record its traffic to the stats module. // https://github.com/grpc/grpc-java/issues/2284 - setStatsRecordStartedRpcs(false); - setStatsRecordFinishedRpcs(false); + managedChannelImplBuilder.setStatsRecordStartedRpcs(false); + managedChannelImplBuilder.setStatsRecordFinishedRpcs(false); + } + + @Internal + @Override + protected ManagedChannelBuilder delegate() { + return managedChannelImplBuilder; } @Override @@ -177,9 +198,7 @@ public InProcessChannelBuilder propagateCauseWithStatus(boolean enable) { return this; } - @Override - @Internal - protected ClientTransportFactory buildTransportFactory() { + ClientTransportFactory buildTransportFactory() { return new InProcessClientTransportFactory( name, scheduledExecutorService, maxInboundMetadataSize, transportIncludeStatusCause); } diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java index 0a3e785f5cd..00f54698137 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java @@ -42,8 +42,14 @@ public interface ChannelBuilderDefaultPortProvider { int getDefaultPort(); } + private class ManagedChannelDefaultPortProvider implements ChannelBuilderDefaultPortProvider { + @Override + public int getDefaultPort() { + return ManagedChannelImplBuilder.super.getDefaultPort(); + } + } + private final ClientTransportFactoryBuilder clientTransportFactoryBuilder; - // TODO(sergiitk): see where getDefaultPort() not overridden, Might be in InProcess private final ChannelBuilderDefaultPortProvider channelBuilderDefaultPortProvider; /** @@ -52,12 +58,16 @@ public interface ChannelBuilderDefaultPortProvider { */ public ManagedChannelImplBuilder(String target, ClientTransportFactoryBuilder clientTransportFactoryBuilder, - ChannelBuilderDefaultPortProvider channelBuilderDefaultPortProvider) { + @Nullable ChannelBuilderDefaultPortProvider channelBuilderDefaultPortProvider) { super(target); this.clientTransportFactoryBuilder = checkNotNull(clientTransportFactoryBuilder, "clientTransportFactoryBuilder cannot be null"); - this.channelBuilderDefaultPortProvider = checkNotNull(channelBuilderDefaultPortProvider, - "channelBuilderDefaultPortProvider cannot be null"); + + if (channelBuilderDefaultPortProvider != null) { + this.channelBuilderDefaultPortProvider = channelBuilderDefaultPortProvider; + } else { + this.channelBuilderDefaultPortProvider = new ManagedChannelDefaultPortProvider(); + } } /** @@ -65,12 +75,16 @@ public ManagedChannelImplBuilder(String target, */ public ManagedChannelImplBuilder(SocketAddress directServerAddress, String authority, ClientTransportFactoryBuilder clientTransportFactoryBuilder, - ChannelBuilderDefaultPortProvider channelBuilderDefaultPortProvider) { + @Nullable ChannelBuilderDefaultPortProvider channelBuilderDefaultPortProvider) { super(directServerAddress, authority); this.clientTransportFactoryBuilder = checkNotNull(clientTransportFactoryBuilder, "clientTransportFactoryBuilder cannot be null"); - this.channelBuilderDefaultPortProvider = checkNotNull(channelBuilderDefaultPortProvider, - "channelBuilderDefaultPortProvider cannot be null"); + + if (channelBuilderDefaultPortProvider != null) { + this.channelBuilderDefaultPortProvider = channelBuilderDefaultPortProvider; + } else { + this.channelBuilderDefaultPortProvider = new ManagedChannelDefaultPortProvider(); + } } @Override @@ -78,6 +92,11 @@ protected ClientTransportFactory buildTransportFactory() { return clientTransportFactoryBuilder.buildClientTransportFactory(); } + @Override + protected int getDefaultPort() { + return channelBuilderDefaultPortProvider.getDefaultPort(); + } + /** * TODO(sergiitk): finish javadoc. */ @@ -144,9 +163,4 @@ public void setTracingEnabled(boolean value) { public ObjectPool getOffloadExecutorPool() { return super.getOffloadExecutorPool(); } - - @Override - protected int getDefaultPort() { - return channelBuilderDefaultPortProvider.getDefaultPort(); - } } From f20a75b25564af9e3f950883f971ea6b0d90eb57 Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Thu, 20 Aug 2020 17:25:16 -0400 Subject: [PATCH 39/54] m --- .../io/grpc/internal/ManagedChannelImplBuilder.java | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java index 00f54698137..33687d47681 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java @@ -16,8 +16,7 @@ package io.grpc.internal; -import static com.google.common.base.Preconditions.checkNotNull; - +import com.google.common.base.Preconditions; import java.net.SocketAddress; import java.util.concurrent.Executor; import javax.annotation.Nullable; @@ -26,7 +25,9 @@ public final class ManagedChannelImplBuilder extends AbstractManagedChannelImplBuilder { private boolean authorityCheckerDisabled; - @Deprecated @Nullable private OverrideAuthorityChecker authorityChecker; + @Deprecated + @Nullable + private OverrideAuthorityChecker authorityChecker; /** * TODO(sergiitk): finish javadoc. @@ -60,7 +61,7 @@ public ManagedChannelImplBuilder(String target, ClientTransportFactoryBuilder clientTransportFactoryBuilder, @Nullable ChannelBuilderDefaultPortProvider channelBuilderDefaultPortProvider) { super(target); - this.clientTransportFactoryBuilder = checkNotNull(clientTransportFactoryBuilder, + this.clientTransportFactoryBuilder = Preconditions.checkNotNull(clientTransportFactoryBuilder, "clientTransportFactoryBuilder cannot be null"); if (channelBuilderDefaultPortProvider != null) { @@ -77,7 +78,7 @@ public ManagedChannelImplBuilder(SocketAddress directServerAddress, String autho ClientTransportFactoryBuilder clientTransportFactoryBuilder, @Nullable ChannelBuilderDefaultPortProvider channelBuilderDefaultPortProvider) { super(directServerAddress, authority); - this.clientTransportFactoryBuilder = checkNotNull(clientTransportFactoryBuilder, + this.clientTransportFactoryBuilder = Preconditions.checkNotNull(clientTransportFactoryBuilder, "clientTransportFactoryBuilder cannot be null"); if (channelBuilderDefaultPortProvider != null) { From df9ee3173b22deee82931ed86cc254076cccd9c1 Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Thu, 20 Aug 2020 18:06:31 -0400 Subject: [PATCH 40/54] ManagedChannelImplBuilder unit test WIP --- .../ManagedChannelImplBuilderTest.java | 123 ++++++++++++++++++ 1 file changed, 123 insertions(+) create mode 100644 core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java new file mode 100644 index 00000000000..554869aad94 --- /dev/null +++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java @@ -0,0 +1,123 @@ +/* + * Copyright 2020 The gRPC Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.grpc.internal; + +import static com.google.common.truth.Truth.assertThat; +import static junit.framework.TestCase.assertFalse; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; +import static org.mockito.Mockito.mock; + +import io.grpc.internal.ManagedChannelImplBuilder.ChannelBuilderDefaultPortProvider; +import io.grpc.internal.ManagedChannelImplBuilder.ClientTransportFactoryBuilder; +import org.junit.After; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.ExpectedException; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** Unit tests for {@link ManagedChannelImplBuilder}. */ +@RunWith(JUnit4.class) +public class ManagedChannelImplBuilderTest { + +// @Rule +// public final ExpectedException thrown = ExpectedException.none(); + + private ManagedChannelImplBuilder builder = new ManagedChannelImplBuilder("fake", + new ClientTransportFactoryBuilder() { + @Override + public ClientTransportFactory buildClientTransportFactory() { + return null; + } + }, + new ChannelBuilderDefaultPortProvider() { + @Override + public int getDefaultPort() { + return 42; + } + } + ); + + +// @Before +// public void setUp() throws Exception { +// } +// +// @After +// public void tearDown() throws Exception { +// } +// +// @Test +// public void buildTransportFactory() { +// } + + @Test + public void getDefaultPort_default() { + assertEquals(builder.getDefaultPort(), GrpcUtil.DEFAULT_PORT_SSL); + } + + @Test + public void getDefaultPort_overridden() { + assertEquals(builder.getDefaultPort(), 42); + } + +// @Test +// public void disableCheckAuthority() { +// } +// +// @Test +// public void enableCheckAuthority() { +// } +// +// @Test +// public void overrideAuthorityChecker() { +// } +// +// @Test +// public void checkAuthority() { +// } +// +// @Test +// public void setStatsEnabled() { +// } +// +// @Test +// public void setStatsRecordStartedRpcs() { +// } +// +// @Test +// public void setStatsRecordFinishedRpcs() { +// } +// +// @Test +// public void setStatsRecordRealTimeMetrics() { +// } +// +// @Test +// public void setTracingEnabled() { +// } +// +// @Test +// public void getOffloadExecutorPool() { +// } +} \ No newline at end of file From 540d5ddf2b6c6cc60d27e836deb6c2e7838b73d1 Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Fri, 21 Aug 2020 14:15:40 -0400 Subject: [PATCH 41/54] Figuring out unit testing --- .../ManagedChannelImplBuilderTest.java | 73 ++++++++++++------- 1 file changed, 48 insertions(+), 25 deletions(-) diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java index 554869aad94..8b9ef4da770 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java +++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java @@ -25,41 +25,54 @@ import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; import io.grpc.internal.ManagedChannelImplBuilder.ChannelBuilderDefaultPortProvider; import io.grpc.internal.ManagedChannelImplBuilder.ClientTransportFactoryBuilder; -import org.junit.After; -import org.junit.Before; import org.junit.Rule; import org.junit.Test; -import org.junit.rules.ExpectedException; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; +import org.mockito.Mock; +import org.mockito.junit.MockitoJUnit; +import org.mockito.junit.MockitoRule; -/** Unit tests for {@link ManagedChannelImplBuilder}. */ +/** + * Unit tests for {@link ManagedChannelImplBuilder}. + */ @RunWith(JUnit4.class) public class ManagedChannelImplBuilderTest { + @Rule + public final MockitoRule mocks = MockitoJUnit.rule(); + + private final static String DUMMY_TARGET = "fake"; + // @Rule // public final ExpectedException thrown = ExpectedException.none(); - private ManagedChannelImplBuilder builder = new ManagedChannelImplBuilder("fake", - new ClientTransportFactoryBuilder() { - @Override - public ClientTransportFactory buildClientTransportFactory() { - return null; - } - }, - new ChannelBuilderDefaultPortProvider() { - @Override - public int getDefaultPort() { - return 42; - } - } - ); - - -// @Before +// private final ManagedChannelImplBuilder builder = new ManagedChannelImplBuilder("fake", +// new ClientTransportFactoryBuilder() { +// @Override +// public ClientTransportFactory buildClientTransportFactory() { +// return null; +// } +// }, +// new ChannelBuilderDefaultPortProvider() { +// @Override +// public int getDefaultPort() { +// return 42; +// } +// } +// ); + + @Mock private ClientTransportFactoryBuilder mockClientTransportFactoryBuilder; + @Mock private ChannelBuilderDefaultPortProvider mockChannelBuilderDefaultPortProvider; +// private final ManagedChannelImplBuilder builder = new ManagedChannelImplBuilder("fake", +// clientTransportFactoryBuilderMock, +// channelBuilderDefaultPortProviderMock); + // public void setUp() throws Exception { // } // @@ -73,12 +86,22 @@ public int getDefaultPort() { @Test public void getDefaultPort_default() { - assertEquals(builder.getDefaultPort(), GrpcUtil.DEFAULT_PORT_SSL); + final ManagedChannelImplBuilder builderNoPortProvider = new ManagedChannelImplBuilder( + DUMMY_TARGET, mockClientTransportFactoryBuilder, null); + + assertEquals(GrpcUtil.DEFAULT_PORT_SSL, builderNoPortProvider.getDefaultPort()); } @Test - public void getDefaultPort_overridden() { - assertEquals(builder.getDefaultPort(), 42); + public void getDefaultPort_custom() { + final int DUMMY_PORT = 42; + when(mockChannelBuilderDefaultPortProvider.getDefaultPort()).thenReturn(DUMMY_PORT); + + final ManagedChannelImplBuilder builder = new ManagedChannelImplBuilder( + DUMMY_TARGET, mockClientTransportFactoryBuilder, mockChannelBuilderDefaultPortProvider); + + assertEquals(DUMMY_PORT, builder.getDefaultPort()); + verify(mockChannelBuilderDefaultPortProvider).getDefaultPort(); } // @Test @@ -120,4 +143,4 @@ public void getDefaultPort_overridden() { // @Test // public void getOffloadExecutorPool() { // } -} \ No newline at end of file +} From a7abed126f211c658d45d394a0e2936217e3e24b Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Fri, 21 Aug 2020 15:15:52 -0400 Subject: [PATCH 42/54] Shared builder, test buildTransportFactory() --- .../ManagedChannelImplBuilderTest.java | 62 +++++++++---------- 1 file changed, 28 insertions(+), 34 deletions(-) diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java index 8b9ef4da770..3231c40614d 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java +++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java @@ -30,6 +30,7 @@ import io.grpc.internal.ManagedChannelImplBuilder.ChannelBuilderDefaultPortProvider; import io.grpc.internal.ManagedChannelImplBuilder.ClientTransportFactoryBuilder; +import org.junit.Before; import org.junit.Rule; import org.junit.Test; import org.junit.runner.RunWith; @@ -48,42 +49,37 @@ public class ManagedChannelImplBuilderTest { public final MockitoRule mocks = MockitoJUnit.rule(); private final static String DUMMY_TARGET = "fake"; + private final static ClientTransportFactory DUMMY_CLIENT_TRANSPORT_FACTORY = mock( + ClientTransportFactory.class); // @Rule // public final ExpectedException thrown = ExpectedException.none(); -// private final ManagedChannelImplBuilder builder = new ManagedChannelImplBuilder("fake", -// new ClientTransportFactoryBuilder() { -// @Override -// public ClientTransportFactory buildClientTransportFactory() { -// return null; -// } -// }, -// new ChannelBuilderDefaultPortProvider() { -// @Override -// public int getDefaultPort() { -// return 42; -// } -// } -// ); - - @Mock private ClientTransportFactoryBuilder mockClientTransportFactoryBuilder; - @Mock private ChannelBuilderDefaultPortProvider mockChannelBuilderDefaultPortProvider; -// private final ManagedChannelImplBuilder builder = new ManagedChannelImplBuilder("fake", -// clientTransportFactoryBuilderMock, -// channelBuilderDefaultPortProviderMock); - -// public void setUp() throws Exception { -// } -// -// @After -// public void tearDown() throws Exception { -// } -// -// @Test -// public void buildTransportFactory() { -// } + @Mock + private ClientTransportFactoryBuilder mockClientTransportFactoryBuilder; + @Mock + private ChannelBuilderDefaultPortProvider mockChannelBuilderDefaultPortProvider; + private ManagedChannelImplBuilder builder; + + @Before + public void setUp() throws Exception { + builder = new ManagedChannelImplBuilder( + DUMMY_TARGET, + mockClientTransportFactoryBuilder, + mockChannelBuilderDefaultPortProvider); + } + + /** Ensure buildTransportFactory() delegates to the custom implementation. */ + @Test + public void buildTransportFactory() { + when(mockClientTransportFactoryBuilder.buildClientTransportFactory()) + .thenReturn(DUMMY_CLIENT_TRANSPORT_FACTORY); + assertEquals(DUMMY_CLIENT_TRANSPORT_FACTORY, builder.buildTransportFactory()); + verify(mockClientTransportFactoryBuilder).buildClientTransportFactory(); + } + + /** Ensure getDefaultPort() returns default port when no custom implementation provided */ @Test public void getDefaultPort_default() { final ManagedChannelImplBuilder builderNoPortProvider = new ManagedChannelImplBuilder( @@ -92,14 +88,12 @@ public void getDefaultPort_default() { assertEquals(GrpcUtil.DEFAULT_PORT_SSL, builderNoPortProvider.getDefaultPort()); } + /** Ensure getDefaultPort() delegates to the custom implementation. */ @Test public void getDefaultPort_custom() { final int DUMMY_PORT = 42; when(mockChannelBuilderDefaultPortProvider.getDefaultPort()).thenReturn(DUMMY_PORT); - final ManagedChannelImplBuilder builder = new ManagedChannelImplBuilder( - DUMMY_TARGET, mockClientTransportFactoryBuilder, mockChannelBuilderDefaultPortProvider); - assertEquals(DUMMY_PORT, builder.getDefaultPort()); verify(mockChannelBuilderDefaultPortProvider).getDefaultPort(); } From 6fb2f42881c024567c1ddb6c78e30bc73fd6ded5 Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Fri, 21 Aug 2020 16:46:28 -0400 Subject: [PATCH 43/54] Finish ManagedChannelImplBuilderTest tests --- .../ManagedChannelImplBuilderTest.java | 141 ++++++++++-------- 1 file changed, 76 insertions(+), 65 deletions(-) diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java index 3231c40614d..68c2d2b3720 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java +++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java @@ -16,49 +16,36 @@ package io.grpc.internal; -import static com.google.common.truth.Truth.assertThat; -import static junit.framework.TestCase.assertFalse; import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertNotEquals; -import static org.junit.Assert.assertNotNull; -import static org.junit.Assert.assertNull; -import static org.junit.Assert.assertTrue; -import static org.junit.Assert.fail; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import io.grpc.internal.ManagedChannelImplBuilder.ChannelBuilderDefaultPortProvider; import io.grpc.internal.ManagedChannelImplBuilder.ClientTransportFactoryBuilder; +import io.grpc.internal.ManagedChannelImplBuilder.OverrideAuthorityChecker; import org.junit.Before; import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ExpectedException; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; import org.mockito.Mock; import org.mockito.junit.MockitoJUnit; import org.mockito.junit.MockitoRule; -/** - * Unit tests for {@link ManagedChannelImplBuilder}. - */ +/** Unit tests for {@link ManagedChannelImplBuilder}. */ @RunWith(JUnit4.class) public class ManagedChannelImplBuilderTest { + private final static String DUMMY_TARGET = "fake-target"; + private final static String DUMMY_AUTHORITY_VALID = "valid:1234"; + private final static String DUMMY_AUTHORITY_INVALID = "[ : : 1]"; - @Rule - public final MockitoRule mocks = MockitoJUnit.rule(); - - private final static String DUMMY_TARGET = "fake"; - private final static ClientTransportFactory DUMMY_CLIENT_TRANSPORT_FACTORY = mock( - ClientTransportFactory.class); + @Rule public final MockitoRule mocks = MockitoJUnit.rule(); + @Rule public final ExpectedException thrown = ExpectedException.none(); -// @Rule -// public final ExpectedException thrown = ExpectedException.none(); - - @Mock - private ClientTransportFactoryBuilder mockClientTransportFactoryBuilder; - @Mock - private ChannelBuilderDefaultPortProvider mockChannelBuilderDefaultPortProvider; + @Mock private ClientTransportFactoryBuilder mockClientTransportFactoryBuilder; + @Mock private ChannelBuilderDefaultPortProvider mockChannelBuilderDefaultPortProvider; private ManagedChannelImplBuilder builder; @Before @@ -72,14 +59,15 @@ public void setUp() throws Exception { /** Ensure buildTransportFactory() delegates to the custom implementation. */ @Test public void buildTransportFactory() { + final ClientTransportFactory clientTransportFactory = mock(ClientTransportFactory.class); when(mockClientTransportFactoryBuilder.buildClientTransportFactory()) - .thenReturn(DUMMY_CLIENT_TRANSPORT_FACTORY); + .thenReturn(clientTransportFactory); - assertEquals(DUMMY_CLIENT_TRANSPORT_FACTORY, builder.buildTransportFactory()); + assertEquals(clientTransportFactory, builder.buildTransportFactory()); verify(mockClientTransportFactoryBuilder).buildClientTransportFactory(); } - /** Ensure getDefaultPort() returns default port when no custom implementation provided */ + /** Ensure getDefaultPort() returns default port when no custom implementation provided. */ @Test public void getDefaultPort_default() { final ManagedChannelImplBuilder builderNoPortProvider = new ManagedChannelImplBuilder( @@ -98,43 +86,66 @@ public void getDefaultPort_custom() { verify(mockChannelBuilderDefaultPortProvider).getDefaultPort(); } -// @Test -// public void disableCheckAuthority() { -// } -// -// @Test -// public void enableCheckAuthority() { -// } -// -// @Test -// public void overrideAuthorityChecker() { -// } -// -// @Test -// public void checkAuthority() { -// } -// -// @Test -// public void setStatsEnabled() { -// } -// -// @Test -// public void setStatsRecordStartedRpcs() { -// } -// -// @Test -// public void setStatsRecordFinishedRpcs() { -// } -// -// @Test -// public void setStatsRecordRealTimeMetrics() { -// } -// -// @Test -// public void setTracingEnabled() { -// } -// -// @Test -// public void getOffloadExecutorPool() { -// } + @Test + public void checkAuthority_validAuthorityAllowed() { + assertEquals(DUMMY_AUTHORITY_VALID, builder.checkAuthority(DUMMY_AUTHORITY_VALID)); + } + + @Test + public void checkAuthority_invalidAuthorityFailed() { + thrown.expect(IllegalArgumentException.class); + thrown.expectMessage("Invalid authority"); + + builder.checkAuthority(DUMMY_AUTHORITY_INVALID); + } + + @Test + public void disableCheckAuthority_validAuthorityAllowed() { + builder.disableCheckAuthority(); + assertEquals(DUMMY_AUTHORITY_VALID, builder.checkAuthority(DUMMY_AUTHORITY_VALID)); + } + + @Test + public void disableCheckAuthority_invalidAuthorityAllowed() { + builder.disableCheckAuthority(); + assertEquals(DUMMY_AUTHORITY_INVALID, builder.checkAuthority(DUMMY_AUTHORITY_INVALID)); + } + + @Test + public void enableCheckAuthority_validAuthorityAllowed() { + builder.disableCheckAuthority().enableCheckAuthority(); + assertEquals(DUMMY_AUTHORITY_VALID, builder.checkAuthority(DUMMY_AUTHORITY_VALID)); + } + + @Test + public void disableCheckAuthority_invalidAuthorityFailed() { + thrown.expect(IllegalArgumentException.class); + thrown.expectMessage("Invalid authority"); + + builder.disableCheckAuthority().enableCheckAuthority(); + builder.checkAuthority(DUMMY_AUTHORITY_INVALID); + } + + /** Ensure authority check can disabled with custom authority check implementation. */ + @Test + public void overrideAuthorityChecker_default() { + builder.overrideAuthorityChecker(new OverrideAuthorityChecker() { + @Override public String checkAuthority(String authority) { + return authority; + } + }); + assertEquals(DUMMY_AUTHORITY_INVALID, builder.checkAuthority(DUMMY_AUTHORITY_INVALID)); + } + + /** Ensure custom authority is ignored after disableCheckAuthority(). */ + @Test + public void overrideAuthorityChecker_ignored() { + builder.overrideAuthorityChecker(new OverrideAuthorityChecker() { + @Override public String checkAuthority(String authority) { + throw new IllegalArgumentException(); + } + }); + builder.disableCheckAuthority(); + assertEquals(DUMMY_AUTHORITY_INVALID, builder.checkAuthority(DUMMY_AUTHORITY_INVALID)); + } } From d8b9c5209d46f5852f2385120b74d109a27b0e94 Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Fri, 21 Aug 2020 17:50:49 -0400 Subject: [PATCH 44/54] Fix ManagedChannelImplBuilderTest style --- .../grpc/internal/ManagedChannelImplBuilderTest.java | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java index 68c2d2b3720..de980dd5256 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java +++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java @@ -37,9 +37,10 @@ /** Unit tests for {@link ManagedChannelImplBuilder}. */ @RunWith(JUnit4.class) public class ManagedChannelImplBuilderTest { - private final static String DUMMY_TARGET = "fake-target"; - private final static String DUMMY_AUTHORITY_VALID = "valid:1234"; - private final static String DUMMY_AUTHORITY_INVALID = "[ : : 1]"; + private static final int DUMMY_PORT = 42; + private static final String DUMMY_TARGET = "fake-target"; + private static final String DUMMY_AUTHORITY_VALID = "valid:1234"; + private static final String DUMMY_AUTHORITY_INVALID = "[ : : 1]"; @Rule public final MockitoRule mocks = MockitoJUnit.rule(); @Rule public final ExpectedException thrown = ExpectedException.none(); @@ -62,7 +63,6 @@ public void buildTransportFactory() { final ClientTransportFactory clientTransportFactory = mock(ClientTransportFactory.class); when(mockClientTransportFactoryBuilder.buildClientTransportFactory()) .thenReturn(clientTransportFactory); - assertEquals(clientTransportFactory, builder.buildTransportFactory()); verify(mockClientTransportFactoryBuilder).buildClientTransportFactory(); } @@ -72,16 +72,13 @@ public void buildTransportFactory() { public void getDefaultPort_default() { final ManagedChannelImplBuilder builderNoPortProvider = new ManagedChannelImplBuilder( DUMMY_TARGET, mockClientTransportFactoryBuilder, null); - assertEquals(GrpcUtil.DEFAULT_PORT_SSL, builderNoPortProvider.getDefaultPort()); } /** Ensure getDefaultPort() delegates to the custom implementation. */ @Test public void getDefaultPort_custom() { - final int DUMMY_PORT = 42; when(mockChannelBuilderDefaultPortProvider.getDefaultPort()).thenReturn(DUMMY_PORT); - assertEquals(DUMMY_PORT, builder.getDefaultPort()); verify(mockChannelBuilderDefaultPortProvider).getDefaultPort(); } From 097a6fb9a6994180fbd70f4b2ae9689e5d3efc23 Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Fri, 21 Aug 2020 17:51:09 -0400 Subject: [PATCH 45/54] Replace TestingAccessor.setStatsEnabled() Replace TestingAccessor.setStatsEnabled(AbstractManagedChannelImplBuilder) with individual internal channel builders --- .../grpc/benchmarks/TransportBenchmark.java | 4 +-- .../inprocess/InProcessChannelBuilder.java | 4 +++ .../InternalInProcessChannelBuilder.java | 33 +++++++++++++++++++ .../integration/TestServiceClient.java | 10 +++--- .../integration/AutoWindowSizingOnTest.java | 3 +- .../Http2NettyLocalChannelTest.java | 3 +- .../testing/integration/Http2NettyTest.java | 3 +- .../testing/integration/Http2OkHttpTest.java | 3 +- .../testing/integration/InProcessTest.java | 3 +- .../integration/TransportCompressionTest.java | 3 +- .../okhttp/InternalOkHttpChannelBuilder.java | 33 +++++++++++++++++++ .../io/grpc/okhttp/OkHttpChannelBuilder.java | 4 +++ .../io/grpc/internal/TestingAccessor.java | 7 ---- 13 files changed, 94 insertions(+), 19 deletions(-) create mode 100644 core/src/main/java/io/grpc/inprocess/InternalInProcessChannelBuilder.java create mode 100644 okhttp/src/main/java/io/grpc/okhttp/InternalOkHttpChannelBuilder.java diff --git a/benchmarks/src/jmh/java/io/grpc/benchmarks/TransportBenchmark.java b/benchmarks/src/jmh/java/io/grpc/benchmarks/TransportBenchmark.java index 9f01b5e9143..ec4646bff44 100644 --- a/benchmarks/src/jmh/java/io/grpc/benchmarks/TransportBenchmark.java +++ b/benchmarks/src/jmh/java/io/grpc/benchmarks/TransportBenchmark.java @@ -20,6 +20,7 @@ import com.google.protobuf.ByteString; import io.grpc.ManagedChannel; +import io.grpc.ManagedChannelBuilder; import io.grpc.Server; import io.grpc.Status; import io.grpc.StatusRuntimeException; @@ -30,7 +31,6 @@ import io.grpc.benchmarks.qps.AsyncServer; import io.grpc.inprocess.InProcessChannelBuilder; import io.grpc.inprocess.InProcessServerBuilder; -import io.grpc.internal.AbstractManagedChannelImplBuilder; import io.grpc.internal.AbstractServerImplBuilder; import io.grpc.netty.NegotiationType; import io.grpc.netty.NettyChannelBuilder; @@ -81,7 +81,7 @@ public enum Transport { @Setup public void setUp() throws Exception { AbstractServerImplBuilder serverBuilder; - AbstractManagedChannelImplBuilder channelBuilder; + ManagedChannelBuilder channelBuilder; switch (transport) { case INPROCESS: { diff --git a/core/src/main/java/io/grpc/inprocess/InProcessChannelBuilder.java b/core/src/main/java/io/grpc/inprocess/InProcessChannelBuilder.java index 431c10e9650..6a99d36c03e 100644 --- a/core/src/main/java/io/grpc/inprocess/InProcessChannelBuilder.java +++ b/core/src/main/java/io/grpc/inprocess/InProcessChannelBuilder.java @@ -203,6 +203,10 @@ ClientTransportFactory buildTransportFactory() { name, scheduledExecutorService, maxInboundMetadataSize, transportIncludeStatusCause); } + void setStatsEnabled(boolean value) { + this.managedChannelImplBuilder.setStatsEnabled(value); + } + /** * Creates InProcess transports. Exposed for internal use, as it should be private. */ diff --git a/core/src/main/java/io/grpc/inprocess/InternalInProcessChannelBuilder.java b/core/src/main/java/io/grpc/inprocess/InternalInProcessChannelBuilder.java new file mode 100644 index 00000000000..96c99ebf9e8 --- /dev/null +++ b/core/src/main/java/io/grpc/inprocess/InternalInProcessChannelBuilder.java @@ -0,0 +1,33 @@ +/* + * Copyright 2016 The gRPC Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.grpc.inprocess; + +import io.grpc.Internal; + +/** + * Internal {@link InProcessChannelBuilder} accessor. This is intended for usage internal to the + * gRPC team. If you *really* think you need to use this, contact the gRPC team first. + */ +@Internal +public final class InternalInProcessChannelBuilder { + + public static void setStatsEnabled(InProcessChannelBuilder builder, boolean value) { + builder.setStatsEnabled(value); + } + + private InternalInProcessChannelBuilder() {} +} diff --git a/interop-testing/src/main/java/io/grpc/testing/integration/TestServiceClient.java b/interop-testing/src/main/java/io/grpc/testing/integration/TestServiceClient.java index fa7010d19a8..5823435ae94 100644 --- a/interop-testing/src/main/java/io/grpc/testing/integration/TestServiceClient.java +++ b/interop-testing/src/main/java/io/grpc/testing/integration/TestServiceClient.java @@ -23,12 +23,13 @@ import io.grpc.alts.AltsChannelBuilder; import io.grpc.alts.ComputeEngineChannelBuilder; import io.grpc.alts.GoogleDefaultChannelBuilder; -import io.grpc.internal.AbstractManagedChannelImplBuilder; import io.grpc.internal.GrpcUtil; import io.grpc.internal.testing.TestUtils; import io.grpc.netty.GrpcSslContexts; +import io.grpc.netty.InternalNettyChannelBuilder; import io.grpc.netty.NegotiationType; import io.grpc.netty.NettyChannelBuilder; +import io.grpc.okhttp.InternalOkHttpChannelBuilder; import io.grpc.okhttp.OkHttpChannelBuilder; import io.grpc.okhttp.internal.Platform; import io.netty.handler.ssl.SslContext; @@ -402,7 +403,7 @@ protected ManagedChannelBuilder createChannelBuilder() { if (useAlts) { return AltsChannelBuilder.forAddress(serverHost, serverPort); } - AbstractManagedChannelImplBuilder builder; + ManagedChannelBuilder builder; if (!useOkHttp) { SslContext sslContext = null; if (useTestCa) { @@ -425,6 +426,7 @@ protected ManagedChannelBuilder createChannelBuilder() { if (fullStreamDecompression) { nettyBuilder.enableFullStreamDecompression(); } + InternalNettyChannelBuilder.setStatsEnabled(nettyBuilder, false); builder = nettyBuilder; } else { OkHttpChannelBuilder okBuilder = OkHttpChannelBuilder.forAddress(serverHost, serverPort); @@ -449,10 +451,10 @@ protected ManagedChannelBuilder createChannelBuilder() { if (fullStreamDecompression) { okBuilder.enableFullStreamDecompression(); } + InternalOkHttpChannelBuilder.setStatsEnabled(okBuilder, false); builder = okBuilder; } - // Disable the default census stats interceptor, use testing interceptor instead. - io.grpc.internal.TestingAccessor.setStatsEnabled(builder, false); + // Use testing interceptor instead. return builder.intercept(createCensusStatsClientInterceptor()); } diff --git a/interop-testing/src/test/java/io/grpc/testing/integration/AutoWindowSizingOnTest.java b/interop-testing/src/test/java/io/grpc/testing/integration/AutoWindowSizingOnTest.java index a68181be831..a2036ecea91 100644 --- a/interop-testing/src/test/java/io/grpc/testing/integration/AutoWindowSizingOnTest.java +++ b/interop-testing/src/test/java/io/grpc/testing/integration/AutoWindowSizingOnTest.java @@ -17,6 +17,7 @@ package io.grpc.testing.integration; import io.grpc.internal.AbstractServerImplBuilder; +import io.grpc.netty.InternalNettyChannelBuilder; import io.grpc.netty.NegotiationType; import io.grpc.netty.NettyChannelBuilder; import io.grpc.netty.NettyServerBuilder; @@ -39,7 +40,7 @@ protected NettyChannelBuilder createChannelBuilder() { .maxInboundMessageSize(AbstractInteropTest.MAX_MESSAGE_SIZE) .initialFlowControlWindow(NettyChannelBuilder.DEFAULT_FLOW_CONTROL_WINDOW); // Disable the default census stats interceptor, use testing interceptor instead. - io.grpc.internal.TestingAccessor.setStatsEnabled(builder, false); + InternalNettyChannelBuilder.setStatsEnabled(builder, false); return builder.intercept(createCensusStatsClientInterceptor()); } } diff --git a/interop-testing/src/test/java/io/grpc/testing/integration/Http2NettyLocalChannelTest.java b/interop-testing/src/test/java/io/grpc/testing/integration/Http2NettyLocalChannelTest.java index 5592801755b..8ed7dc76900 100644 --- a/interop-testing/src/test/java/io/grpc/testing/integration/Http2NettyLocalChannelTest.java +++ b/interop-testing/src/test/java/io/grpc/testing/integration/Http2NettyLocalChannelTest.java @@ -17,6 +17,7 @@ package io.grpc.testing.integration; import io.grpc.internal.AbstractServerImplBuilder; +import io.grpc.netty.InternalNettyChannelBuilder; import io.grpc.netty.NegotiationType; import io.grpc.netty.NettyChannelBuilder; import io.grpc.netty.NettyServerBuilder; @@ -57,7 +58,7 @@ protected NettyChannelBuilder createChannelBuilder() { .flowControlWindow(65 * 1024) .maxInboundMessageSize(AbstractInteropTest.MAX_MESSAGE_SIZE); // Disable the default census stats interceptor, use testing interceptor instead. - io.grpc.internal.TestingAccessor.setStatsEnabled(builder, false); + InternalNettyChannelBuilder.setStatsEnabled(builder, false); return builder.intercept(createCensusStatsClientInterceptor()); } diff --git a/interop-testing/src/test/java/io/grpc/testing/integration/Http2NettyTest.java b/interop-testing/src/test/java/io/grpc/testing/integration/Http2NettyTest.java index 1819b9f3008..353180cbafb 100644 --- a/interop-testing/src/test/java/io/grpc/testing/integration/Http2NettyTest.java +++ b/interop-testing/src/test/java/io/grpc/testing/integration/Http2NettyTest.java @@ -22,6 +22,7 @@ import io.grpc.internal.AbstractServerImplBuilder; import io.grpc.internal.testing.TestUtils; import io.grpc.netty.GrpcSslContexts; +import io.grpc.netty.InternalNettyChannelBuilder; import io.grpc.netty.NettyChannelBuilder; import io.grpc.netty.NettyServerBuilder; import io.netty.handler.ssl.ClientAuth; @@ -71,7 +72,7 @@ protected NettyChannelBuilder createChannelBuilder() { .ciphers(TestUtils.preferredTestCiphers(), SupportedCipherSuiteFilter.INSTANCE) .build()); // Disable the default census stats interceptor, use testing interceptor instead. - io.grpc.internal.TestingAccessor.setStatsEnabled(builder, false); + InternalNettyChannelBuilder.setStatsEnabled(builder, false); return builder.intercept(createCensusStatsClientInterceptor()); } catch (Exception ex) { throw new RuntimeException(ex); diff --git a/interop-testing/src/test/java/io/grpc/testing/integration/Http2OkHttpTest.java b/interop-testing/src/test/java/io/grpc/testing/integration/Http2OkHttpTest.java index 041c7cafaa6..fc6e600789d 100644 --- a/interop-testing/src/test/java/io/grpc/testing/integration/Http2OkHttpTest.java +++ b/interop-testing/src/test/java/io/grpc/testing/integration/Http2OkHttpTest.java @@ -29,6 +29,7 @@ import io.grpc.internal.testing.TestUtils; import io.grpc.netty.GrpcSslContexts; import io.grpc.netty.NettyServerBuilder; +import io.grpc.okhttp.InternalOkHttpChannelBuilder; import io.grpc.okhttp.OkHttpChannelBuilder; import io.grpc.okhttp.internal.Platform; import io.grpc.stub.StreamObserver; @@ -102,7 +103,7 @@ protected OkHttpChannelBuilder createChannelBuilder() { throw new RuntimeException(e); } // Disable the default census stats interceptor, use testing interceptor instead. - io.grpc.internal.TestingAccessor.setStatsEnabled(builder, false); + InternalOkHttpChannelBuilder.setStatsEnabled(builder, false); return builder.intercept(createCensusStatsClientInterceptor()); } diff --git a/interop-testing/src/test/java/io/grpc/testing/integration/InProcessTest.java b/interop-testing/src/test/java/io/grpc/testing/integration/InProcessTest.java index fd403affcf5..66894834b95 100644 --- a/interop-testing/src/test/java/io/grpc/testing/integration/InProcessTest.java +++ b/interop-testing/src/test/java/io/grpc/testing/integration/InProcessTest.java @@ -18,6 +18,7 @@ import io.grpc.inprocess.InProcessChannelBuilder; import io.grpc.inprocess.InProcessServerBuilder; +import io.grpc.inprocess.InternalInProcessChannelBuilder; import io.grpc.internal.AbstractServerImplBuilder; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -38,7 +39,7 @@ protected AbstractServerImplBuilder getServerBuilder() { protected InProcessChannelBuilder createChannelBuilder() { InProcessChannelBuilder builder = InProcessChannelBuilder.forName(SERVER_NAME); // Disable the default census stats interceptor, use testing interceptor instead. - io.grpc.internal.TestingAccessor.setStatsEnabled(builder, false); + InternalInProcessChannelBuilder.setStatsEnabled(builder, false); return builder.intercept(createCensusStatsClientInterceptor()); } diff --git a/interop-testing/src/test/java/io/grpc/testing/integration/TransportCompressionTest.java b/interop-testing/src/test/java/io/grpc/testing/integration/TransportCompressionTest.java index 8bc0494b79d..1144c75073c 100644 --- a/interop-testing/src/test/java/io/grpc/testing/integration/TransportCompressionTest.java +++ b/interop-testing/src/test/java/io/grpc/testing/integration/TransportCompressionTest.java @@ -37,6 +37,7 @@ import io.grpc.ServerInterceptor; import io.grpc.internal.AbstractServerImplBuilder; import io.grpc.internal.GrpcUtil; +import io.grpc.netty.InternalNettyChannelBuilder; import io.grpc.netty.NettyChannelBuilder; import io.grpc.netty.NettyServerBuilder; import io.grpc.testing.integration.Messages.BoolValue; @@ -165,7 +166,7 @@ public void onHeaders(Metadata headers) { }) .usePlaintext(); // Disable the default census stats interceptor, use testing interceptor instead. - io.grpc.internal.TestingAccessor.setStatsEnabled(builder, false); + InternalNettyChannelBuilder.setStatsEnabled(builder, false); return builder.intercept(createCensusStatsClientInterceptor()); } diff --git a/okhttp/src/main/java/io/grpc/okhttp/InternalOkHttpChannelBuilder.java b/okhttp/src/main/java/io/grpc/okhttp/InternalOkHttpChannelBuilder.java new file mode 100644 index 00000000000..306f620d25a --- /dev/null +++ b/okhttp/src/main/java/io/grpc/okhttp/InternalOkHttpChannelBuilder.java @@ -0,0 +1,33 @@ +/* + * Copyright 2016 The gRPC Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.grpc.okhttp; + +import io.grpc.Internal; + +/** + * Internal {@link OkHttpChannelBuilder} accessor. This is intended for usage internal to the gRPC + * team. If you *really* think you need to use this, contact the gRPC team first. + */ +@Internal +public final class InternalOkHttpChannelBuilder { + + public static void setStatsEnabled(OkHttpChannelBuilder builder, boolean value) { + builder.setStatsEnabled(value); + } + + private InternalOkHttpChannelBuilder() {} +} diff --git a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java index ab3ddc6343f..15e5afabbc5 100644 --- a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java +++ b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java @@ -449,6 +449,10 @@ int getDefaultPort() { } } + void setStatsEnabled(boolean value) { + this.managedChannelImplBuilder.setStatsEnabled(value); + } + @VisibleForTesting @Nullable SSLSocketFactory createSslSocketFactory() { diff --git a/testing/src/main/java/io/grpc/internal/TestingAccessor.java b/testing/src/main/java/io/grpc/internal/TestingAccessor.java index d5b00e025fc..92dc114d9c6 100644 --- a/testing/src/main/java/io/grpc/internal/TestingAccessor.java +++ b/testing/src/main/java/io/grpc/internal/TestingAccessor.java @@ -20,13 +20,6 @@ * Test helper that allows accessing package-private stuff. */ public final class TestingAccessor { - /** - * Disable or enable client side census stats features. - */ - public static void setStatsEnabled( - AbstractManagedChannelImplBuilder builder, boolean statsEnabled) { - builder.setStatsEnabled(statsEnabled); - } /** * Disable or enable server side census stats features. From e1419fc1173b7fb0de65029fb653eb637131e22e Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Fri, 21 Aug 2020 18:29:31 -0400 Subject: [PATCH 46/54] Fix channelBuilderHidesMethod_for*() tests --- .../java/io/grpc/internal/ManagedChannelImplBuilder.java | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java index 33687d47681..6faa3efb6e4 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java @@ -17,6 +17,7 @@ package io.grpc.internal; import com.google.common.base.Preconditions; +import io.grpc.ManagedChannelBuilder; import java.net.SocketAddress; import java.util.concurrent.Executor; import javax.annotation.Nullable; @@ -164,4 +165,12 @@ public void setTracingEnabled(boolean value) { public ObjectPool getOffloadExecutorPool() { return super.getOffloadExecutorPool(); } + + public static ManagedChannelBuilder forAddress(String name, int port) { + throw new UnsupportedOperationException("ClientTransportFactoryBuilder is required"); + } + + public static ManagedChannelBuilder forTarget(String target) { + throw new UnsupportedOperationException("ClientTransportFactoryBuilder is required"); + } } From c8f5595093fee0152f6489a3d08719cea37d10cd Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Fri, 21 Aug 2020 18:36:53 -0400 Subject: [PATCH 47/54] Suppress deprecation - full build works! --- .../ManagedChannelImplBuilderTest.java | 25 ++++++++------- .../grpc/netty/NettyChannelBuilderTest.java | 31 ++++++++++--------- 2 files changed, 31 insertions(+), 25 deletions(-) diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java index de980dd5256..3cea10a94ce 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java +++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java @@ -23,7 +23,6 @@ import io.grpc.internal.ManagedChannelImplBuilder.ChannelBuilderDefaultPortProvider; import io.grpc.internal.ManagedChannelImplBuilder.ClientTransportFactoryBuilder; -import io.grpc.internal.ManagedChannelImplBuilder.OverrideAuthorityChecker; import org.junit.Before; import org.junit.Rule; import org.junit.Test; @@ -125,23 +124,27 @@ public void disableCheckAuthority_invalidAuthorityFailed() { /** Ensure authority check can disabled with custom authority check implementation. */ @Test + @SuppressWarnings("deprecation") public void overrideAuthorityChecker_default() { - builder.overrideAuthorityChecker(new OverrideAuthorityChecker() { - @Override public String checkAuthority(String authority) { - return authority; - } - }); + builder.overrideAuthorityChecker( + new io.grpc.internal.ManagedChannelImplBuilder.OverrideAuthorityChecker() { + @Override public String checkAuthority(String authority) { + return authority; + } + }); assertEquals(DUMMY_AUTHORITY_INVALID, builder.checkAuthority(DUMMY_AUTHORITY_INVALID)); } /** Ensure custom authority is ignored after disableCheckAuthority(). */ @Test + @SuppressWarnings("deprecation") public void overrideAuthorityChecker_ignored() { - builder.overrideAuthorityChecker(new OverrideAuthorityChecker() { - @Override public String checkAuthority(String authority) { - throw new IllegalArgumentException(); - } - }); + builder.overrideAuthorityChecker( + new io.grpc.internal.ManagedChannelImplBuilder.OverrideAuthorityChecker() { + @Override public String checkAuthority(String authority) { + throw new IllegalArgumentException(); + } + }); builder.disableCheckAuthority(); assertEquals(DUMMY_AUTHORITY_INVALID, builder.checkAuthority(DUMMY_AUTHORITY_INVALID)); } diff --git a/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java b/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java index fe753eed63a..9b31f529f11 100644 --- a/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java +++ b/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java @@ -23,7 +23,6 @@ import io.grpc.ManagedChannel; import io.grpc.internal.GrpcUtil; -import io.grpc.netty.InternalNettyChannelBuilder.OverrideAuthorityChecker; import io.grpc.netty.NettyTestUtil.TrackingObjectPoolForTest; import io.netty.channel.Channel; import io.netty.channel.ChannelFactory; @@ -45,7 +44,7 @@ public class NettyChannelBuilderTest { @Rule public final ExpectedException thrown = ExpectedException.none(); private final SslContext noSslContext = null; - + private void shutdown(ManagedChannel mc) throws Exception { mc.shutdownNow(); assertTrue(mc.awaitTermination(1, TimeUnit.SECONDS)); @@ -93,28 +92,32 @@ private void overrideAuthorityIsReadableHelper(NettyChannelBuilder builder, } @Test + @SuppressWarnings("deprecation") public void overrideAllowsInvalidAuthority() { NettyChannelBuilder builder = new NettyChannelBuilder(new SocketAddress(){}); - InternalNettyChannelBuilder.overrideAuthorityChecker(builder, new OverrideAuthorityChecker() { - @Override - public String checkAuthority(String authority) { - return authority; - } - }); + InternalNettyChannelBuilder.overrideAuthorityChecker(builder, + new io.grpc.netty.InternalNettyChannelBuilder.OverrideAuthorityChecker() { + @Override + public String checkAuthority(String authority) { + return authority; + } + }); Object unused = builder.overrideAuthority("[invalidauthority") .negotiationType(NegotiationType.PLAINTEXT) .buildTransportFactory(); } @Test + @SuppressWarnings("deprecation") public void overrideFailsInvalidAuthority() { NettyChannelBuilder builder = new NettyChannelBuilder(new SocketAddress(){}); - InternalNettyChannelBuilder.overrideAuthorityChecker(builder, new OverrideAuthorityChecker() { - @Override - public String checkAuthority(String authority) { - return GrpcUtil.checkAuthority(authority); - } - }); + InternalNettyChannelBuilder.overrideAuthorityChecker(builder, + new io.grpc.netty.InternalNettyChannelBuilder.OverrideAuthorityChecker() { + @Override + public String checkAuthority(String authority) { + return GrpcUtil.checkAuthority(authority); + } + }); thrown.expect(IllegalArgumentException.class); thrown.expectMessage("Invalid authority:"); From 52e0060728bcda0efc47a272d36b1d26cd60b1f5 Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Mon, 24 Aug 2020 14:03:49 -0400 Subject: [PATCH 48/54] Refactor CronetChannelBuilder --- .../io/grpc/cronet/CronetChannelBuilder.java | 35 +++++++++++++++---- 1 file changed, 29 insertions(+), 6 deletions(-) diff --git a/cronet/src/main/java/io/grpc/cronet/CronetChannelBuilder.java b/cronet/src/main/java/io/grpc/cronet/CronetChannelBuilder.java index 1a87de58507..205f152097e 100644 --- a/cronet/src/main/java/io/grpc/cronet/CronetChannelBuilder.java +++ b/cronet/src/main/java/io/grpc/cronet/CronetChannelBuilder.java @@ -26,10 +26,14 @@ import com.google.common.util.concurrent.MoreExecutors; import io.grpc.ChannelLogger; import io.grpc.ExperimentalApi; -import io.grpc.internal.AbstractManagedChannelImplBuilder; +import io.grpc.ForwardingChannelBuilder.SimpleForwardingChannelBuilder; +import io.grpc.Internal; +import io.grpc.ManagedChannelBuilder; import io.grpc.internal.ClientTransportFactory; import io.grpc.internal.ConnectionClientTransport; import io.grpc.internal.GrpcUtil; +import io.grpc.internal.ManagedChannelImplBuilder; +import io.grpc.internal.ManagedChannelImplBuilder.ClientTransportFactoryBuilder; import io.grpc.internal.SharedResourceHolder; import io.grpc.internal.TransportTracer; import java.lang.reflect.InvocationTargetException; @@ -47,7 +51,7 @@ /** Convenience class for building channels with the cronet transport. */ @ExperimentalApi("There is no plan to make this API stable, given transport API instability") public final class CronetChannelBuilder extends - AbstractManagedChannelImplBuilder { + SimpleForwardingChannelBuilder { private static final String LOG_TAG = "CronetChannelBuilder"; @@ -81,6 +85,8 @@ public static CronetChannelBuilder forAddress(String name, int port) { private ScheduledExecutorService scheduledExecutorService; private final CronetEngine cronetEngine; + private final ManagedChannelImplBuilder managedChannelImplBuilder; + private TransportTracer.Factory transportTracerFactory = TransportTracer.getDefaultFactory(); private boolean alwaysUsePut = false; @@ -103,12 +109,30 @@ public static CronetChannelBuilder forAddress(String name, int port) { private int trafficStatsUid; private CronetChannelBuilder(String host, int port, CronetEngine cronetEngine) { - super( + super(); + + // An anonymous class to inject client transport factory builder. + final class CronetChannelTransportFactoryBuilder implements ClientTransportFactoryBuilder { + @Override + public ClientTransportFactory buildClientTransportFactory() { + return buildTransportFactory(); + } + } + + managedChannelImplBuilder = new ManagedChannelImplBuilder( InetSocketAddress.createUnresolved(host, port), - GrpcUtil.authorityFromHostAndPort(host, port)); + GrpcUtil.authorityFromHostAndPort(host, port), + new CronetChannelTransportFactoryBuilder(), + null); this.cronetEngine = Preconditions.checkNotNull(cronetEngine, "cronetEngine"); } + @Internal + @Override + protected ManagedChannelBuilder delegate() { + return managedChannelImplBuilder; + } + /** * Sets the maximum message size allowed to be received on the channel. If not called, * defaults to {@link io.grpc.internal.GrpcUtil#DEFAULT_MAX_MESSAGE_SIZE}. @@ -188,8 +212,7 @@ public final CronetChannelBuilder scheduledExecutorService( return this; } - @Override - protected final ClientTransportFactory buildTransportFactory() { + final ClientTransportFactory buildTransportFactory() { return new CronetTransportFactory( new TaggingStreamFactory( cronetEngine, trafficStatsTagSet, trafficStatsTag, trafficStatsUidSet, trafficStatsUid), From 6d3a5e1f7eacfaaca8a31bfed0e8ce89b12cf551 Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Mon, 24 Aug 2020 14:15:46 -0400 Subject: [PATCH 49/54] Get rid of SimpleForwardingChannelBuilder to preserve delegate private --- api/src/main/java/io/grpc/ForwardingChannelBuilder.java | 7 ------- .../java/io/grpc/inprocess/InProcessChannelBuilder.java | 4 ++-- .../src/main/java/io/grpc/cronet/CronetChannelBuilder.java | 5 ++--- netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java | 4 ++-- .../src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java | 4 ++-- 5 files changed, 8 insertions(+), 16 deletions(-) diff --git a/api/src/main/java/io/grpc/ForwardingChannelBuilder.java b/api/src/main/java/io/grpc/ForwardingChannelBuilder.java index d7bd5cf0d85..ca3f11d1c10 100644 --- a/api/src/main/java/io/grpc/ForwardingChannelBuilder.java +++ b/api/src/main/java/io/grpc/ForwardingChannelBuilder.java @@ -269,11 +269,4 @@ protected final T thisT() { return thisT; } - /** - * A simplified version of {@link ForwardingChannelBuilder}. - */ - public abstract static class SimpleForwardingChannelBuilder> - extends ForwardingChannelBuilder { - // TODO(sergiitk): implement - } } diff --git a/core/src/main/java/io/grpc/inprocess/InProcessChannelBuilder.java b/core/src/main/java/io/grpc/inprocess/InProcessChannelBuilder.java index 6a99d36c03e..6caef698da0 100644 --- a/core/src/main/java/io/grpc/inprocess/InProcessChannelBuilder.java +++ b/core/src/main/java/io/grpc/inprocess/InProcessChannelBuilder.java @@ -21,7 +21,7 @@ import io.grpc.ChannelLogger; import io.grpc.ExperimentalApi; -import io.grpc.ForwardingChannelBuilder.SimpleForwardingChannelBuilder; +import io.grpc.ForwardingChannelBuilder; import io.grpc.Internal; import io.grpc.ManagedChannelBuilder; import io.grpc.internal.ClientTransportFactory; @@ -45,7 +45,7 @@ */ @ExperimentalApi("https://github.com/grpc/grpc-java/issues/1783") public final class InProcessChannelBuilder extends - SimpleForwardingChannelBuilder { + ForwardingChannelBuilder { /** * Create a channel builder that will connect to the server with the given name. * diff --git a/cronet/src/main/java/io/grpc/cronet/CronetChannelBuilder.java b/cronet/src/main/java/io/grpc/cronet/CronetChannelBuilder.java index 205f152097e..e72a4a11fb7 100644 --- a/cronet/src/main/java/io/grpc/cronet/CronetChannelBuilder.java +++ b/cronet/src/main/java/io/grpc/cronet/CronetChannelBuilder.java @@ -26,7 +26,7 @@ import com.google.common.util.concurrent.MoreExecutors; import io.grpc.ChannelLogger; import io.grpc.ExperimentalApi; -import io.grpc.ForwardingChannelBuilder.SimpleForwardingChannelBuilder; +import io.grpc.ForwardingChannelBuilder; import io.grpc.Internal; import io.grpc.ManagedChannelBuilder; import io.grpc.internal.ClientTransportFactory; @@ -50,8 +50,7 @@ /** Convenience class for building channels with the cronet transport. */ @ExperimentalApi("There is no plan to make this API stable, given transport API instability") -public final class CronetChannelBuilder extends - SimpleForwardingChannelBuilder { +public final class CronetChannelBuilder extends ForwardingChannelBuilder { private static final String LOG_TAG = "CronetChannelBuilder"; diff --git a/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java b/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java index 5e64a5e3237..d0b232710ff 100644 --- a/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java +++ b/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java @@ -28,7 +28,7 @@ import io.grpc.ChannelLogger; import io.grpc.EquivalentAddressGroup; import io.grpc.ExperimentalApi; -import io.grpc.ForwardingChannelBuilder.SimpleForwardingChannelBuilder; +import io.grpc.ForwardingChannelBuilder; import io.grpc.HttpConnectProxiedSocketAddress; import io.grpc.Internal; import io.grpc.ManagedChannelBuilder; @@ -67,7 +67,7 @@ */ @ExperimentalApi("https://github.com/grpc/grpc-java/issues/1784") @CanIgnoreReturnValue -public final class NettyChannelBuilder extends SimpleForwardingChannelBuilder { +public final class NettyChannelBuilder extends ForwardingChannelBuilder { // 1MiB. public static final int DEFAULT_FLOW_CONTROL_WINDOW = 1024 * 1024; diff --git a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java index 15e5afabbc5..f169b395183 100644 --- a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java +++ b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java @@ -24,7 +24,7 @@ import com.google.common.base.Preconditions; import io.grpc.ChannelLogger; import io.grpc.ExperimentalApi; -import io.grpc.ForwardingChannelBuilder.SimpleForwardingChannelBuilder; +import io.grpc.ForwardingChannelBuilder; import io.grpc.Internal; import io.grpc.ManagedChannelBuilder; import io.grpc.internal.AtomicBackoff; @@ -58,7 +58,7 @@ /** Convenience class for building channels with the OkHttp transport. */ @ExperimentalApi("https://github.com/grpc/grpc-java/issues/1785") -public class OkHttpChannelBuilder extends SimpleForwardingChannelBuilder { +public class OkHttpChannelBuilder extends ForwardingChannelBuilder { public static final int DEFAULT_FLOW_CONTROL_WINDOW = 65535; private final ManagedChannelImplBuilder managedChannelImplBuilder; From 51d04cdb2327a77ea735d4a54b7a325b0d47d775 Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Mon, 24 Aug 2020 14:47:34 -0400 Subject: [PATCH 50/54] Replace AbstractManagedChannelImplBuilder in ServiceConfigErrorHandlingTest --- .../ServiceConfigErrorHandlingTest.java | 45 +++++++++---------- 1 file changed, 20 insertions(+), 25 deletions(-) diff --git a/core/src/test/java/io/grpc/internal/ServiceConfigErrorHandlingTest.java b/core/src/test/java/io/grpc/internal/ServiceConfigErrorHandlingTest.java index b8790ad34dc..0fac069fd7c 100644 --- a/core/src/test/java/io/grpc/internal/ServiceConfigErrorHandlingTest.java +++ b/core/src/test/java/io/grpc/internal/ServiceConfigErrorHandlingTest.java @@ -46,6 +46,8 @@ import io.grpc.NameResolver; import io.grpc.NameResolver.ConfigOrError; import io.grpc.Status; +import io.grpc.internal.ManagedChannelImplBuilder.ChannelBuilderDefaultPortProvider; +import io.grpc.internal.ManagedChannelImplBuilder.ClientTransportFactoryBuilder; import java.net.SocketAddress; import java.net.URI; import java.util.ArrayList; @@ -153,7 +155,18 @@ public ConfigOrError parseLoadBalancingPolicyConfig( private ObjectPool balancerRpcExecutorPool; @Mock private Executor blockingExecutor; - private ChannelBuilder channelBuilder; + + private ManagedChannelImplBuilder channelBuilder = new ManagedChannelImplBuilder(TARGET, + new ClientTransportFactoryBuilder() { + @Override public ClientTransportFactory buildClientTransportFactory() { + throw new UnsupportedOperationException(); + } + }, + new ChannelBuilderDefaultPortProvider() { + @Override public int getDefaultPort() { + return DEFAULT_PORT; + } + }); private void createChannel(ClientInterceptor... interceptors) { checkState(channel == null); @@ -199,14 +212,12 @@ public void setUp() throws Exception { .thenReturn(timer.getScheduledExecutorService()); when(executorPool.getObject()).thenReturn(executor.getScheduledExecutorService()); - channelBuilder = - new ChannelBuilder() - .nameResolverFactory(new FakeNameResolverFactory.Builder(expectedUri).build()) - .defaultLoadBalancingPolicy(MOCK_POLICY_NAME) - .userAgent(USER_AGENT) - .idleTimeout( - AbstractManagedChannelImplBuilder.IDLE_MODE_MAX_TIMEOUT_DAYS, TimeUnit.DAYS) - .offloadExecutor(blockingExecutor); + channelBuilder + .nameResolverFactory(new FakeNameResolverFactory.Builder(expectedUri).build()) + .defaultLoadBalancingPolicy(MOCK_POLICY_NAME) + .userAgent(USER_AGENT) + .idleTimeout(ManagedChannelImplBuilder.IDLE_MODE_MAX_TIMEOUT_DAYS, TimeUnit.DAYS) + .offloadExecutor(blockingExecutor); channelBuilder.executorPool = executorPool; channelBuilder.binlog = null; channelBuilder.channelz = channelz; @@ -529,22 +540,6 @@ public void validConfig_thenNoConfig_withDefaultConfig() throws Exception { verify(mockLoadBalancer, never()).handleNameResolutionError(any(Status.class)); } - private static final class ChannelBuilder - extends AbstractManagedChannelImplBuilder { - - ChannelBuilder() { - super(TARGET); - } - - @Override protected ClientTransportFactory buildTransportFactory() { - throw new UnsupportedOperationException(); - } - - @Override protected int getDefaultPort() { - return DEFAULT_PORT; - } - } - private static final class FakeBackoffPolicyProvider implements BackoffPolicy.Provider { @Override public BackoffPolicy get() { From 5ab7d1b08b4ef985c2b6e2b8bf9b7e98673ad1fb Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Mon, 24 Aug 2020 14:52:45 -0400 Subject: [PATCH 51/54] m --- api/src/main/java/io/grpc/ForwardingChannelBuilder.java | 1 - .../java/io/grpc/inprocess/InternalInProcessChannelBuilder.java | 2 +- .../main/java/io/grpc/okhttp/InternalOkHttpChannelBuilder.java | 2 +- 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/api/src/main/java/io/grpc/ForwardingChannelBuilder.java b/api/src/main/java/io/grpc/ForwardingChannelBuilder.java index ca3f11d1c10..612779a7848 100644 --- a/api/src/main/java/io/grpc/ForwardingChannelBuilder.java +++ b/api/src/main/java/io/grpc/ForwardingChannelBuilder.java @@ -268,5 +268,4 @@ protected final T thisT() { T thisT = (T) this; return thisT; } - } diff --git a/core/src/main/java/io/grpc/inprocess/InternalInProcessChannelBuilder.java b/core/src/main/java/io/grpc/inprocess/InternalInProcessChannelBuilder.java index 96c99ebf9e8..1a017fe564c 100644 --- a/core/src/main/java/io/grpc/inprocess/InternalInProcessChannelBuilder.java +++ b/core/src/main/java/io/grpc/inprocess/InternalInProcessChannelBuilder.java @@ -1,5 +1,5 @@ /* - * Copyright 2016 The gRPC Authors + * Copyright 2020 The gRPC Authors * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. diff --git a/okhttp/src/main/java/io/grpc/okhttp/InternalOkHttpChannelBuilder.java b/okhttp/src/main/java/io/grpc/okhttp/InternalOkHttpChannelBuilder.java index 306f620d25a..d328efd7145 100644 --- a/okhttp/src/main/java/io/grpc/okhttp/InternalOkHttpChannelBuilder.java +++ b/okhttp/src/main/java/io/grpc/okhttp/InternalOkHttpChannelBuilder.java @@ -1,5 +1,5 @@ /* - * Copyright 2016 The gRPC Authors + * Copyright 2020 The gRPC Authors * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. From 927b43d1aa13b8eab8cd477d8cbaa0c60f01bb17 Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Mon, 24 Aug 2020 15:46:16 -0400 Subject: [PATCH 52/54] Replace AbstractManagedChannelImplBuilder in ManagedChannelImplTest --- .../grpc/internal/ManagedChannelImplTest.java | 68 ++++++++----------- 1 file changed, 29 insertions(+), 39 deletions(-) diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java index 3650911f142..141b84a7ad2 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java +++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java @@ -107,6 +107,8 @@ import io.grpc.internal.ClientTransportFactory.ClientTransportOptions; import io.grpc.internal.InternalSubchannel.TransportLogger; import io.grpc.internal.ManagedChannelImpl.ScParser; +import io.grpc.internal.ManagedChannelImplBuilder.ChannelBuilderDefaultPortProvider; +import io.grpc.internal.ManagedChannelImplBuilder.ClientTransportFactoryBuilder; import io.grpc.internal.ServiceConfigUtil.PolicySelection; import io.grpc.internal.TestUtils.MockClientTransportInfo; import io.grpc.stub.ClientCalls; @@ -269,7 +271,17 @@ public String getPolicyName() { private CallCredentials creds; @Mock private Executor offloadExecutor; - private ChannelBuilder channelBuilder; + private ManagedChannelImplBuilder channelBuilder = new ManagedChannelImplBuilder(TARGET, + new ClientTransportFactoryBuilder() { + @Override public ClientTransportFactory buildClientTransportFactory() { + throw new UnsupportedOperationException(); + } + }, + new ChannelBuilderDefaultPortProvider() { + @Override public int getDefaultPort() { + return DEFAULT_PORT; + } + }); private boolean requestConnection = true; private BlockingQueue transports; private boolean panicExpected; @@ -325,14 +337,12 @@ public void setUp() throws Exception { when(balancerRpcExecutorPool.getObject()) .thenReturn(balancerRpcExecutor.getScheduledExecutorService()); - channelBuilder = - new ChannelBuilder() - .nameResolverFactory(new FakeNameResolverFactory.Builder(expectedUri).build()) - .defaultLoadBalancingPolicy(MOCK_POLICY_NAME) - .userAgent(USER_AGENT) - .idleTimeout( - AbstractManagedChannelImplBuilder.IDLE_MODE_MAX_TIMEOUT_DAYS, TimeUnit.DAYS) - .offloadExecutor(offloadExecutor); + channelBuilder + .nameResolverFactory(new FakeNameResolverFactory.Builder(expectedUri).build()) + .defaultLoadBalancingPolicy(MOCK_POLICY_NAME) + .userAgent(USER_AGENT) + .idleTimeout(ManagedChannelImplBuilder.IDLE_MODE_MAX_TIMEOUT_DAYS, TimeUnit.DAYS) + .offloadExecutor(offloadExecutor); channelBuilder.executorPool = executorPool; channelBuilder.binlog = null; channelBuilder.channelz = channelz; @@ -3466,21 +3476,17 @@ public String getDefaultScheme() { } FakeNameResolverFactory2 factory = new FakeNameResolverFactory2(); - final class CustomBuilder extends AbstractManagedChannelImplBuilder { - - CustomBuilder() { - super(TARGET); - this.executorPool = ManagedChannelImplTest.this.executorPool; - this.channelz = ManagedChannelImplTest.this.channelz; - } - - @Override - protected ClientTransportFactory buildTransportFactory() { - return mockTransportFactory; - } - } - ManagedChannel mychannel = new CustomBuilder().nameResolverFactory(factory).build(); + ManagedChannelImplBuilder customBuilder = new ManagedChannelImplBuilder(TARGET, + new ClientTransportFactoryBuilder() { + @Override public ClientTransportFactory buildClientTransportFactory() { + return mockTransportFactory; + } + }, + null); + customBuilder.executorPool = executorPool; + customBuilder.channelz = channelz; + ManagedChannel mychannel = customBuilder.nameResolverFactory(factory).build(); ClientCall call1 = mychannel.newCall(TestMethodDescriptors.voidMethod(), CallOptions.DEFAULT); @@ -4025,22 +4031,6 @@ public void createResolvingOobChannel() throws Exception { } } - private static final class ChannelBuilder - extends AbstractManagedChannelImplBuilder { - - ChannelBuilder() { - super(TARGET); - } - - @Override protected ClientTransportFactory buildTransportFactory() { - throw new UnsupportedOperationException(); - } - - @Override protected int getDefaultPort() { - return DEFAULT_PORT; - } - } - private static final class FakeBackoffPolicyProvider implements BackoffPolicy.Provider { @Override public BackoffPolicy get() { From 398fb92562625a29d73823c1f91c39973a0f0070 Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Mon, 24 Aug 2020 16:08:58 -0400 Subject: [PATCH 53/54] Replace AbstractManagedChannelImplBuilder in ManagedChannelImplIdlenessTest --- .../ManagedChannelImplIdlenessTest.java | 23 ++++++++----------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplIdlenessTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplIdlenessTest.java index 2295dcb0ff2..c551d26449c 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplIdlenessTest.java +++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplIdlenessTest.java @@ -62,6 +62,7 @@ import io.grpc.Status; import io.grpc.StringMarshaller; import io.grpc.internal.FakeClock.ScheduledTask; +import io.grpc.internal.ManagedChannelImplBuilder.ClientTransportFactoryBuilder; import io.grpc.internal.TestUtils.MockClientTransportInfo; import java.net.SocketAddress; import java.net.URI; @@ -159,21 +160,15 @@ public void setUp() { when(mockTransportFactory.getScheduledExecutorService()) .thenReturn(timer.getScheduledExecutorService()); - class Builder extends AbstractManagedChannelImplBuilder { - Builder(String target) { - super(target); - } - - @Override protected ClientTransportFactory buildTransportFactory() { - throw new UnsupportedOperationException(); - } - - @Override public Builder usePlaintext() { - throw new UnsupportedOperationException(); - } - } + ManagedChannelImplBuilder builder = new ManagedChannelImplBuilder("fake://target", + new ClientTransportFactoryBuilder() { + @Override public ClientTransportFactory buildClientTransportFactory() { + throw new UnsupportedOperationException(); + } + }, + null); - Builder builder = new Builder("fake://target") + builder .nameResolverFactory(mockNameResolverFactory) .defaultLoadBalancingPolicy(MOCK_POLICY_NAME) .idleTimeout(IDLE_TIMEOUT_SECONDS, TimeUnit.SECONDS) From 92effa9e4501cef3a1ea9d7bd44756919fb9f2a9 Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Tue, 25 Aug 2020 11:02:11 -0400 Subject: [PATCH 54/54] Finish javadocs --- .../AbstractManagedChannelImplBuilder.java | 2 +- .../internal/ManagedChannelImplBuilder.java | 27 +++++++++++-------- 2 files changed, 17 insertions(+), 12 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java index b92fdf5410a..aac6c25a8ee 100644 --- a/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java @@ -51,7 +51,7 @@ import javax.annotation.Nullable; /** - * The base class for channel builders. + * Abstract base class for channel builders. * * @param The concrete type of this builder. */ diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java index 6faa3efb6e4..4db733dfadb 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java @@ -22,6 +22,9 @@ import java.util.concurrent.Executor; import javax.annotation.Nullable; +/** + * The base class for channel builders. + */ public final class ManagedChannelImplBuilder extends AbstractManagedChannelImplBuilder { @@ -31,14 +34,17 @@ public final class ManagedChannelImplBuilder private OverrideAuthorityChecker authorityChecker; /** - * TODO(sergiitk): finish javadoc. + * An interface for Transport implementors to provide the {@link ClientTransportFactory} + * appropriate for the channel. */ public interface ClientTransportFactoryBuilder { ClientTransportFactory buildClientTransportFactory(); } /** - * TODO(sergiitk): finish javadoc. + * An interface for Transport implementors to provide a default port to {@link + * io.grpc.NameResolver} for use in cases where the target string doesn't include a port. The + * default implementation returns {@link GrpcUtil#DEFAULT_PORT_SSL}. */ public interface ChannelBuilderDefaultPortProvider { int getDefaultPort(); @@ -55,8 +61,9 @@ public int getDefaultPort() { private final ChannelBuilderDefaultPortProvider channelBuilderDefaultPortProvider; /** - * Creates a new builder for the given target that will be resolved by {@link - * io.grpc.NameResolver}. TODO(sergiitk): finish javadoc + * Creates a new managed channel builder with a target string, which can be either a valid {@link + * io.grpc.NameResolver}-compliant URI, or an authority string. Transport implementors must + * provide client transport factory builder, and may set custom channel default port provider. */ public ManagedChannelImplBuilder(String target, ClientTransportFactoryBuilder clientTransportFactoryBuilder, @@ -73,7 +80,9 @@ public ManagedChannelImplBuilder(String target, } /** - * TODO(sergiitk): finish javadoc. + * Creates a new managed channel builder with the given server address, authority string of the + * channel. Transport implementors must provide client transport factory builder, and may set + * custom channel default port provider. */ public ManagedChannelImplBuilder(SocketAddress directServerAddress, String authority, ClientTransportFactoryBuilder clientTransportFactoryBuilder, @@ -99,17 +108,13 @@ protected int getDefaultPort() { return channelBuilderDefaultPortProvider.getDefaultPort(); } - /** - * TODO(sergiitk): finish javadoc. - */ + /** Disable the check whether the authority is valid. */ public ManagedChannelImplBuilder disableCheckAuthority() { authorityCheckerDisabled = true; return this; } - /** - * TODO(sergiitk): finish javadoc. - */ + /** Enable previously disabled authority check. */ public ManagedChannelImplBuilder enableCheckAuthority() { authorityCheckerDisabled = false; return this;