From 110bb7bf6e0a85d3dbeb4cd498b860caad78600e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dariusz=20J=C4=99drzejczyk?= Date: Mon, 4 Oct 2021 19:54:55 +0200 Subject: [PATCH 1/8] Untangling `GrpcServerBuilder` from `HttpServerBuilder` Motivation: The methods in `GrpcServerBuilder` are often only delegating to the underlying HTTP transport builder. It will be beneficial if the API can be simplified and have only gRPC specific settings. The first step is to deprecate the methods which can be called directly on the underlying `HttpServerBuilder` and to introduce a method for configuring that builder. Modifications: - Introduce a `GrpcServerBuilder.HttpInitializer` interface and a corresponding `GrpcServerBuilder#initialize` method which accepts the initializer for the underlying `HttpServerBuilder` instance, - Deprecate methods which have only served the purpose of delegating to the underlying HTTP transport builder, - Refactor tests to use the new approach to showcase it works. Result: The API of `GrpcServerBuilder` is prepared for removal of the deprecated methods in 0.42 release and the API is more specific, while maintaining the same level of configurability. --- .../grpc/api/GrpcServerBuilder.java | 70 ++++++++++++++++++- .../grpc/netty/DefaultGrpcServerBuilder.java | 14 ++++ .../grpc/netty/ErrorHandlingTest.java | 4 +- .../netty/GrpcClientRequiresTrailersTest.java | 4 +- .../GrpcClientValidatesContentTypeTest.java | 4 +- .../grpc/netty/GrpcLifecycleObserverTest.java | 7 +- .../netty/GrpcServiceContextProtocolTest.java | 2 +- .../GrpcSslAndNonSslConnectionsTest.java | 12 ++-- .../servicetalk/grpc/netty/KeepAliveTest.java | 20 +++--- .../grpc/netty/ProtocolCompatibilityTest.java | 31 ++++---- .../grpc/netty/TrailersOnlyErrorTest.java | 4 +- 11 files changed, 129 insertions(+), 43 deletions(-) diff --git a/servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcServerBuilder.java b/servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcServerBuilder.java index 001e8aae32..20ceb276e8 100644 --- a/servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcServerBuilder.java +++ b/servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcServerBuilder.java @@ -21,6 +21,7 @@ import io.servicetalk.http.api.HttpExecutionStrategy; import io.servicetalk.http.api.HttpProtocolConfig; import io.servicetalk.http.api.HttpRequest; +import io.servicetalk.http.api.HttpServerBuilder; import io.servicetalk.http.api.HttpServiceContext; import io.servicetalk.http.api.StreamingHttpRequest; import io.servicetalk.http.api.StreamingHttpResponse; @@ -55,8 +56,15 @@ */ public abstract class GrpcServerBuilder { + @FunctionalInterface + public interface HttpInitializer { + void initialize(HttpServerBuilder builder); + } + private boolean appendedCatchAllFilter; + public abstract GrpcServerBuilder initializer(HttpInitializer initializer); + /** * Configurations of various underlying protocol versions. *

@@ -65,7 +73,12 @@ public abstract class GrpcServerBuilder { * * @param protocols {@link HttpProtocolConfig} for each protocol that should be supported. * @return {@code this}. + * @deprecated Call {@link #initializer(HttpInitializer)} and use + * {@link HttpServerBuilder#protocols(HttpProtocolConfig...)} + * on the {@code builder} instance by implementing {@link HttpInitializer#initialize(HttpServerBuilder)} + * functional interface. */ + @Deprecated public abstract GrpcServerBuilder protocols(HttpProtocolConfig... protocols); /** @@ -81,7 +94,12 @@ public abstract class GrpcServerBuilder { * Set the SSL/TLS configuration. * @param config The configuration to use. * @return {@code this}. + * @deprecated Call {@link #initializer(HttpInitializer)} and use + * {@link HttpServerBuilder#sslConfig(ServerSslConfig)} + * on the {@code builder} instance by implementing {@link HttpInitializer#initialize(HttpServerBuilder)} + * functional interface. */ + @Deprecated public abstract GrpcServerBuilder sslConfig(ServerSslConfig config); /** @@ -91,7 +109,12 @@ public abstract class GrpcServerBuilder { * @param sniMap A map where the keys are matched against the client certificate's SNI extension value in order * to provide the corresponding {@link ServerSslConfig}. * @return {@code this}. + * @deprecated Call {@link #initializer(HttpInitializer)} and use + * {@link HttpServerBuilder#sslConfig(ServerSslConfig, Map)} + * on the {@code builder} instance by implementing {@link HttpInitializer#initialize(HttpServerBuilder)} + * functional interface. */ + @Deprecated public abstract GrpcServerBuilder sslConfig(ServerSslConfig defaultConfig, Map sniMap); /** @@ -103,7 +126,12 @@ public abstract class GrpcServerBuilder { * @return {@code this}. * @see StandardSocketOptions * @see ServiceTalkSocketOptions + * @deprecated Call {@link #initializer(HttpInitializer)} and use + * {@link HttpServerBuilder#socketOption(SocketOption, Object)} + * on the {@code builder} instance by implementing {@link HttpInitializer#initialize(HttpServerBuilder)} + * functional interface. */ + @Deprecated public abstract GrpcServerBuilder socketOption(SocketOption option, T value); /** @@ -114,7 +142,12 @@ public abstract class GrpcServerBuilder { * @return this. * @see StandardSocketOptions * @see ServiceTalkSocketOptions + * @deprecated Call {@link #initializer(HttpInitializer)} and use + * {@link HttpServerBuilder#listenSocketOption(SocketOption, Object)} + * on the {@code builder} instance by implementing {@link HttpInitializer#initialize(HttpServerBuilder)} + * functional interface. */ + @Deprecated public abstract GrpcServerBuilder listenSocketOption(SocketOption option, T value); /** @@ -125,7 +158,12 @@ public abstract class GrpcServerBuilder { * @param logUserData {@code true} to include user data (e.g. data, headers, etc.). {@code false} to exclude user * data and log only network events. * @return {@code this}. + * @deprecated Call {@link #initializer(HttpInitializer)} and use + * {@link HttpServerBuilder#enableWireLogging(String, LogLevel, BooleanSupplier)} + * on the {@code builder} instance by implementing {@link HttpInitializer#initialize(HttpServerBuilder)} + * functional interface. */ + @Deprecated public abstract GrpcServerBuilder enableWireLogging(String loggerName, LogLevel logLevel, BooleanSupplier logUserData); @@ -134,7 +172,12 @@ public abstract GrpcServerBuilder enableWireLogging(String loggerName, LogLevel * * @param transportObserver A {@link TransportObserver} that provides visibility into transport events. * @return {@code this}. + * @deprecated Call {@link #initializer(HttpInitializer)} and use + * {@link HttpServerBuilder#transportObserver(TransportObserver)} + * on the {@code builder} instance by implementing {@link HttpInitializer#initialize(HttpServerBuilder)} + * functional interface. */ + @Deprecated public abstract GrpcServerBuilder transportObserver(TransportObserver transportObserver); /** @@ -160,7 +203,12 @@ public abstract GrpcServerBuilder enableWireLogging(String loggerName, LogLevel * @param enable When {@code false} it will disable the automatic consumption of request * {@link StreamingHttpRequest#payloadBody()}. * @return {@code this}. + * @deprecated Call {@link #initializer(HttpInitializer)} and use + * {@link HttpServerBuilder#drainRequestPayloadBody(boolean)} + * on the {@code builder} instance by implementing {@link HttpInitializer#initialize(HttpServerBuilder)} + * functional interface. */ + @Deprecated public abstract GrpcServerBuilder drainRequestPayloadBody(boolean enable); /** @@ -179,7 +227,12 @@ public abstract GrpcServerBuilder enableWireLogging(String loggerName, LogLevel * @param factory {@link ConnectionAcceptorFactory} to append. Lifetime of this * {@link ConnectionAcceptorFactory} is managed by this builder and the server started thereof. * @return {@code this}. + * @deprecated Call {@link #initializer(HttpInitializer)} and use + * {@link HttpServerBuilder#appendConnectionAcceptorFilter(ConnectionAcceptorFactory)} + * on the {@code builder} instance by implementing {@link HttpInitializer#initialize(HttpServerBuilder)} + * functional interface. */ + @Deprecated public abstract GrpcServerBuilder appendConnectionAcceptorFilter(ConnectionAcceptorFactory factory); /** @@ -195,9 +248,13 @@ public abstract GrpcServerBuilder enableWireLogging(String loggerName, LogLevel * * @param factory {@link StreamingHttpServiceFilterFactory} to append. * @return {@code this}. + * @deprecated Call {@link #initializer(HttpInitializer)} and use + * {@link HttpServerBuilder#appendServiceFilter(StreamingHttpServiceFilterFactory)} + * on the {@code builder} instance by implementing {@link HttpInitializer#initialize(HttpServerBuilder)} + * functional interface. */ + @Deprecated public final GrpcServerBuilder appendHttpServiceFilter(StreamingHttpServiceFilterFactory factory) { - appendCatchAllFilterIfRequired(); doAppendHttpServiceFilter(factory); return this; } @@ -217,10 +274,14 @@ public final GrpcServerBuilder appendHttpServiceFilter(StreamingHttpServiceFilte * @param predicate the {@link Predicate} to test if the filter must be applied. * @param factory {@link StreamingHttpServiceFilterFactory} to append. * @return {@code this}. + * @deprecated Call {@link #initializer(HttpInitializer)} and use + * {@link HttpServerBuilder#appendServiceFilter(Predicate, StreamingHttpServiceFilterFactory)} + * on the {@code builder} instance by implementing {@link HttpInitializer#initialize(HttpServerBuilder)} + * functional interface. */ + @Deprecated public final GrpcServerBuilder appendHttpServiceFilter(Predicate predicate, StreamingHttpServiceFilterFactory factory) { - appendCatchAllFilterIfRequired(); doAppendHttpServiceFilter(predicate, factory); return this; } @@ -361,7 +422,10 @@ public final ServerContext listenAndAwait(GrpcBindableService... servic protected abstract void doAppendHttpServiceFilter(Predicate predicate, StreamingHttpServiceFilterFactory factory); - private void appendCatchAllFilterIfRequired() { + protected void appendCatchAllFilterIfRequired() { + // TODO(dj): Move to DefaultGrpcServerBuilder and remove the check as the call is in the constructor. + // This code depends on GrpcUtils which is inaccessible from the servicetalk-grpc-netty module. + // When this class is converted to an interface we can also refactor that part. if (!appendedCatchAllFilter) { doAppendHttpServiceFilter(CatchAllHttpServiceFilter::new); appendedCatchAllFilter = true; diff --git a/servicetalk-grpc-netty/src/main/java/io/servicetalk/grpc/netty/DefaultGrpcServerBuilder.java b/servicetalk-grpc-netty/src/main/java/io/servicetalk/grpc/netty/DefaultGrpcServerBuilder.java index bb31d7f202..023b29ec62 100644 --- a/servicetalk-grpc-netty/src/main/java/io/servicetalk/grpc/netty/DefaultGrpcServerBuilder.java +++ b/servicetalk-grpc-netty/src/main/java/io/servicetalk/grpc/netty/DefaultGrpcServerBuilder.java @@ -65,6 +65,8 @@ final class DefaultGrpcServerBuilder extends GrpcServerBuilder implements Server private static final Logger LOGGER = LoggerFactory.getLogger(DefaultGrpcServerBuilder.class); private final HttpServerBuilder httpServerBuilder; + @Nullable + private GrpcServerBuilder.HttpInitializer initializer; private final ExecutionContextBuilder contextBuilder = new ExecutionContextBuilder() // Make sure we always set a strategy so that ExecutionContextBuilder does not create a strategy which is // not compatible with gRPC. @@ -79,6 +81,13 @@ final class DefaultGrpcServerBuilder extends GrpcServerBuilder implements Server DefaultGrpcServerBuilder(final HttpServerBuilder httpServerBuilder) { this.httpServerBuilder = httpServerBuilder.protocols(h2Default()).allowDropRequestTrailers(true); + appendCatchAllFilterIfRequired(); + } + + @Override + public GrpcServerBuilder initializer(final GrpcServerBuilder.HttpInitializer initializer) { + this.initializer = initializer; + return this; } @Override @@ -197,6 +206,11 @@ protected void doAppendHttpServiceFilter(final Predicate p private HttpServerBuilder preBuild() { if (!invokedBuild) { + // TODO(dj): Consider logging a warning in every method that changes the state of the builder once it has + // been used to build a client. + if (initializer != null) { + initializer.initialize(httpServerBuilder); + } doAppendHttpServiceFilter(new TimeoutHttpServiceFilter(grpcDetermineTimeout(defaultTimeout), true)); } invokedBuild = true; diff --git a/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/ErrorHandlingTest.java b/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/ErrorHandlingTest.java index 841108de5c..0f818a3437 100644 --- a/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/ErrorHandlingTest.java +++ b/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/ErrorHandlingTest.java @@ -261,7 +261,9 @@ private void setUp(TestMode testMode, GrpcExecutionStrategy serverStrategy, throw new IllegalArgumentException("Unknown mode: " + testMode); } this.requestPublisher = requestPublisher; - serverContext = GrpcServers.forAddress(localAddress(0)).appendHttpServiceFilter(serviceFilterFactory) + final StreamingHttpServiceFilterFactory filterFactory = serviceFilterFactory; + serverContext = GrpcServers.forAddress(localAddress(0)) + .initializer(builder -> builder.appendServiceFilter(filterFactory)) .executionStrategy(serverStrategy).listenAndAwait(serviceFactory); GrpcClientBuilder clientBuilder = GrpcClients.forAddress(serverHostAndPort(serverContext)).appendHttpClientFilter(clientFilterFactory) diff --git a/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/GrpcClientRequiresTrailersTest.java b/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/GrpcClientRequiresTrailersTest.java index 580e51420f..c0aeebaf58 100644 --- a/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/GrpcClientRequiresTrailersTest.java +++ b/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/GrpcClientRequiresTrailersTest.java @@ -61,7 +61,7 @@ class GrpcClientRequiresTrailersTest { private void setUp(boolean hasTrailers) throws Exception { serverContext = GrpcServers.forAddress(localAddress(0)) - .appendHttpServiceFilter(service -> new StreamingHttpServiceFilter(service) { + .initializer(builder -> builder.appendServiceFilter(service -> new StreamingHttpServiceFilter(service) { @Override public Single handle( final HttpServiceContext ctx, final StreamingHttpRequest request, @@ -78,7 +78,7 @@ protected HttpHeaders payloadComplete(final HttpHeaders trailers) { return resp; }); } - }) + })) .listenAndAwait(new TesterProto.Tester.TesterService() { @Override public Single testRequestStream(final GrpcServiceContext ctx, diff --git a/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/GrpcClientValidatesContentTypeTest.java b/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/GrpcClientValidatesContentTypeTest.java index a74408eba3..e4a2f337d6 100644 --- a/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/GrpcClientValidatesContentTypeTest.java +++ b/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/GrpcClientValidatesContentTypeTest.java @@ -49,7 +49,7 @@ final class GrpcClientValidatesContentTypeTest { void setUp(boolean withCharset) throws Exception { serverContext = GrpcServers.forAddress(localAddress(0)) - .appendHttpServiceFilter(service -> new StreamingHttpServiceFilter(service) { + .initializer(builder -> builder.appendServiceFilter(service -> new StreamingHttpServiceFilter(service) { @Override public Single handle( final HttpServiceContext ctx, final StreamingHttpRequest request, @@ -60,7 +60,7 @@ public Single handle( return resp; }); } - }) + })) .listenAndAwait(new TesterProto.Tester.TesterService() { @Override public Single testRequestStream(final GrpcServiceContext ctx, diff --git a/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/GrpcLifecycleObserverTest.java b/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/GrpcLifecycleObserverTest.java index 81eaf8494f..e6463fe45f 100644 --- a/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/GrpcLifecycleObserverTest.java +++ b/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/GrpcLifecycleObserverTest.java @@ -142,8 +142,11 @@ private void setUp(boolean error) throws Exception { server = forAddress(localAddress(0)) .ioExecutor(SERVER_CTX.ioExecutor()) .executor(SERVER_CTX.executor()) - .enableWireLogging("servicetalk-tests-wire-logger", TRACE, () -> true) - .protocols(h2().enableFrameLogging("servicetalk-tests-h2-frame-logger", TRACE, () -> true).build()) + .initializer(builder -> builder + .enableWireLogging("servicetalk-tests-wire-logger", TRACE, () -> true) + .protocols(h2().enableFrameLogging("servicetalk-tests-h2-frame-logger", TRACE, () -> true) + .build()) + ) .lifecycleObserver(combine(serverLifecycleObserver, LOGGING)) .listenAndAwait(new EchoService(error)); diff --git a/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/GrpcServiceContextProtocolTest.java b/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/GrpcServiceContextProtocolTest.java index 64180c53ba..cc083ba2e7 100644 --- a/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/GrpcServiceContextProtocolTest.java +++ b/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/GrpcServiceContextProtocolTest.java @@ -67,7 +67,7 @@ private void setUp(HttpProtocolVersion httpProtocol, boolean streamingService) t expectedValue = "gRPC over " + httpProtocol; serverContext = GrpcServers.forAddress(localAddress(0)) - .protocols(protocolConfig(httpProtocol)) + .initializer(builder -> builder.protocols(protocolConfig(httpProtocol))) .listenAndAwait(streamingService ? new ServiceFactory(new TesterServiceImpl()) : new ServiceFactory(new BlockingTesterServiceImpl())); diff --git a/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/GrpcSslAndNonSslConnectionsTest.java b/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/GrpcSslAndNonSslConnectionsTest.java index 11c2f8afda..7af8c2152b 100644 --- a/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/GrpcSslAndNonSslConnectionsTest.java +++ b/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/GrpcSslAndNonSslConnectionsTest.java @@ -55,9 +55,7 @@ private ServerContext nonSecureGrpcServer() throws Exception { private ServerContext secureGrpcServer() throws Exception { return GrpcServers.forAddress(localAddress(0)) - .sslConfig( - trustedServerConfig() - ) + .initializer(builder -> builder.sslConfig(trustedServerConfig())) .listenAndAwait(serviceFactory()); } @@ -143,10 +141,10 @@ void secureClientToSecureServerWithoutPeerHostSucceeds() throws Exception { @Test void noSniClientDefaultServerFallbackSuccess() throws Exception { try (ServerContext serverContext = GrpcServers.forAddress(localAddress(0)) - .sslConfig( + .initializer(builder -> builder.sslConfig( trustedServerConfig(), singletonMap(getLoopbackAddress().getHostName(), untrustedServerConfig()) - ) + )) .listenAndAwait(serviceFactory()); BlockingTesterClient client = GrpcClients.forAddress( getLoopbackAddress().getHostName(), serverHostAndPort(serverContext).port()) @@ -164,10 +162,10 @@ void noSniClientDefaultServerFallbackSuccess() throws Exception { @Test void noSniClientDefaultServerFallbackFailExpected() throws Exception { try (ServerContext serverContext = GrpcServers.forAddress(localAddress(0)) - .sslConfig( + .initializer(builder -> builder.sslConfig( untrustedServerConfig(), singletonMap(getLoopbackAddress().getHostName(), trustedServerConfig()) - ) + )) .listenAndAwait(serviceFactory()); BlockingTesterClient client = GrpcClients.forAddress( getLoopbackAddress().getHostName(), serverHostAndPort(serverContext).port()) diff --git a/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/KeepAliveTest.java b/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/KeepAliveTest.java index 3b9464cf71..3101eb1cbd 100644 --- a/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/KeepAliveTest.java +++ b/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/KeepAliveTest.java @@ -84,15 +84,17 @@ private void setUp(final boolean keepAlivesFromClient, final Duration idleTimeout) throws Exception { this.idleTimeoutMillis = idleTimeout.toMillis(); GrpcServerBuilder serverBuilder = forAddress(localAddress(0)) - .executor(SERVER_CTX.executor()) - .ioExecutor(SERVER_CTX.ioExecutor()) - .executionStrategy(defaultStrategy()); - if (!keepAlivesFromClient) { - serverBuilder.protocols(h2Config(keepAliveIdleFor)); - } else { - serverBuilder.socketOption(IDLE_TIMEOUT, idleTimeoutMillis) - .protocols(h2Config(null)); - } + .executor(SERVER_CTX.executor()) + .ioExecutor(SERVER_CTX.ioExecutor()) + .executionStrategy(defaultStrategy()) + .initializer(builder -> { + if (!keepAlivesFromClient) { + builder.protocols(h2Config(keepAliveIdleFor)); + } else { + builder.socketOption(IDLE_TIMEOUT, idleTimeoutMillis) + .protocols(h2Config(null)); + } + }); ctx = serverBuilder.listenAndAwait(new ServiceFactory(new InfiniteStreamsService())); GrpcClientBuilder clientBuilder = GrpcClients.forAddress(serverHostAndPort(ctx)) diff --git a/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/ProtocolCompatibilityTest.java b/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/ProtocolCompatibilityTest.java index e7fe77605e..2a7e847433 100644 --- a/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/ProtocolCompatibilityTest.java +++ b/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/ProtocolCompatibilityTest.java @@ -1063,26 +1063,29 @@ private static GrpcServerBuilder serviceTalkServerBuilder(final ErrorMode errorM @Nullable final Duration timeout) { final GrpcServerBuilder serverBuilder = GrpcServers.forAddress(localAddress(0)) - .appendHttpServiceFilter(service -> new StreamingHttpServiceFilter(service) { - @Override - public Single handle(final HttpServiceContext ctx, - final StreamingHttpRequest req, - final StreamingHttpResponseFactory resFactory) { - if (errorMode == ErrorMode.SIMPLE_IN_SERVER_FILTER) { - throwGrpcStatusException(); - } else if (errorMode == ErrorMode.STATUS_IN_SERVER_FILTER) { - throwGrpcStatusExceptionWithStatus(); + .initializer(builder -> { + builder.appendServiceFilter(service -> new StreamingHttpServiceFilter(service) { + @Override + public Single handle(final HttpServiceContext ctx, + final StreamingHttpRequest req, + final StreamingHttpResponseFactory resFactory) { + if (errorMode == ErrorMode.SIMPLE_IN_SERVER_FILTER) { + throwGrpcStatusException(); + } else if (errorMode == ErrorMode.STATUS_IN_SERVER_FILTER) { + throwGrpcStatusExceptionWithStatus(); + } + return delegate().handle(ctx, req, resFactory); } - return delegate().handle(ctx, req, resFactory); + }); + if (ssl) { + builder.sslConfig(new ServerSslConfigBuilder(DefaultTestCerts::loadServerPem, + DefaultTestCerts::loadServerKey).provider(OPENSSL).build()); } }); if (null != timeout) { serverBuilder.defaultTimeout(timeout); } - return ssl ? - serverBuilder.sslConfig(new ServerSslConfigBuilder(DefaultTestCerts::loadServerPem, - DefaultTestCerts::loadServerKey).provider(OPENSSL).build()) : - serverBuilder; + return serverBuilder; } private static TestServerContext serviceTalkServerBlocking(final ErrorMode errorMode, final boolean ssl, diff --git a/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/TrailersOnlyErrorTest.java b/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/TrailersOnlyErrorTest.java index 5af6cf436b..18ff3dee04 100644 --- a/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/TrailersOnlyErrorTest.java +++ b/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/TrailersOnlyErrorTest.java @@ -151,14 +151,14 @@ void testServiceFilterThrows() throws Exception { final TesterService service = mockTesterService(); final GrpcServerBuilder serverBuilder = GrpcServers.forAddress(localAddress(0)) - .appendHttpServiceFilter(svc -> new StreamingHttpServiceFilter(svc) { + .initializer(builder -> builder.appendServiceFilter(svc -> new StreamingHttpServiceFilter(svc) { @Override public Single handle( final HttpServiceContext ctx, final StreamingHttpRequest request, final StreamingHttpResponseFactory responseFactory) { throw DELIBERATE_EXCEPTION; } - }); + })); try (ServerContext serverContext = serverBuilder.listenAndAwait(new Tester.ServiceFactory(service))) { From 9d2e8530445f88f7c0d2ecf1b350907680518f1b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dariusz=20J=C4=99drzejczyk?= Date: Tue, 5 Oct 2021 14:37:03 +0200 Subject: [PATCH 2/8] Protection against use of deprecated methods together with initializer --- .../grpc/api/GrpcServerBuilder.java | 22 ++++++++++++++++++- .../grpc/netty/DefaultGrpcServerBuilder.java | 18 +++++++++++++++ 2 files changed, 39 insertions(+), 1 deletion(-) diff --git a/servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcServerBuilder.java b/servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcServerBuilder.java index 20ceb276e8..d4ecd66388 100644 --- a/servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcServerBuilder.java +++ b/servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcServerBuilder.java @@ -56,14 +56,34 @@ */ public abstract class GrpcServerBuilder { + /** + * Initializes the underlying {@link HttpServerBuilder} used for the transport layer. + */ @FunctionalInterface public interface HttpInitializer { + + /** + * Configures the underlying {@link HttpServerBuilder}. + * @param builder The builder to customize the HTTP layer. + */ void initialize(HttpServerBuilder builder); } private boolean appendedCatchAllFilter; - public abstract GrpcServerBuilder initializer(HttpInitializer initializer); + /** + * Set a function which can configure the underlying {@link HttpServerBuilder} used for the transport layer. + *

+ * Please note that this method can't be mixed with the {@link Deprecated} methods of this class as the order + * of operations would not be the same as the order in which the calls are made. Please migrate all of the calls + * to this method. + * @param initializer Initializes the underlying HTTP transport builder. + * @return {@code this}. + */ + public GrpcServerBuilder initializer(HttpInitializer initializer) { + throw new UnsupportedOperationException("Initializing the HttpServerBuilder using this method is not yet" + + "supported by " + getClass().getName()); + } /** * Configurations of various underlying protocol versions. diff --git a/servicetalk-grpc-netty/src/main/java/io/servicetalk/grpc/netty/DefaultGrpcServerBuilder.java b/servicetalk-grpc-netty/src/main/java/io/servicetalk/grpc/netty/DefaultGrpcServerBuilder.java index 023b29ec62..18061212cd 100644 --- a/servicetalk-grpc-netty/src/main/java/io/servicetalk/grpc/netty/DefaultGrpcServerBuilder.java +++ b/servicetalk-grpc-netty/src/main/java/io/servicetalk/grpc/netty/DefaultGrpcServerBuilder.java @@ -78,10 +78,13 @@ final class DefaultGrpcServerBuilder extends GrpcServerBuilder implements Server @Nullable private Duration defaultTimeout; private boolean invokedBuild; + private boolean directAppendCalled; DefaultGrpcServerBuilder(final HttpServerBuilder httpServerBuilder) { this.httpServerBuilder = httpServerBuilder.protocols(h2Default()).allowDropRequestTrailers(true); appendCatchAllFilterIfRequired(); + // Reset the flag guarding proper use of underlying http builder as the above call sets it to true. + this.directAppendCalled = false; } @Override @@ -101,30 +104,35 @@ public GrpcServerBuilder defaultTimeout(Duration defaultTimeout) { @Override public GrpcServerBuilder protocols(final HttpProtocolConfig... protocols) { + directAppendCalled = true; httpServerBuilder.protocols(protocols); return this; } @Override public GrpcServerBuilder sslConfig(final ServerSslConfig config) { + directAppendCalled = true; httpServerBuilder.sslConfig(config); return this; } @Override public GrpcServerBuilder sslConfig(final ServerSslConfig defaultConfig, final Map sniMap) { + directAppendCalled = true; httpServerBuilder.sslConfig(defaultConfig, sniMap); return this; } @Override public GrpcServerBuilder socketOption(final SocketOption option, final T value) { + directAppendCalled = true; httpServerBuilder.socketOption(option, value); return this; } @Override public GrpcServerBuilder listenSocketOption(final SocketOption option, final T value) { + directAppendCalled = true; httpServerBuilder.listenSocketOption(option, value); return this; } @@ -132,12 +140,14 @@ public GrpcServerBuilder listenSocketOption(final SocketOption option, fi @Override public GrpcServerBuilder enableWireLogging(final String loggerName, final LogLevel logLevel, final BooleanSupplier logUserData) { + directAppendCalled = true; httpServerBuilder.enableWireLogging(loggerName, logLevel, logUserData); return this; } @Override public GrpcServerBuilder transportObserver(final TransportObserver transportObserver) { + directAppendCalled = true; httpServerBuilder.transportObserver(transportObserver); return this; } @@ -150,12 +160,14 @@ public GrpcServerBuilder lifecycleObserver(final GrpcLifecycleObserver lifecycle @Override public GrpcServerBuilder drainRequestPayloadBody(boolean enable) { + directAppendCalled = true; httpServerBuilder.drainRequestPayloadBody(enable); return this; } @Override public GrpcServerBuilder appendConnectionAcceptorFilter(final ConnectionAcceptorFactory factory) { + directAppendCalled = true; httpServerBuilder.appendConnectionAcceptorFilter(factory); return this; } @@ -195,6 +207,7 @@ protected Single doListen(final GrpcServiceFactory servi @Override protected void doAppendHttpServiceFilter(final StreamingHttpServiceFilterFactory factory) { + directAppendCalled = true; httpServerBuilder.appendServiceFilter(factory); } @@ -209,6 +222,11 @@ private HttpServerBuilder preBuild() { // TODO(dj): Consider logging a warning in every method that changes the state of the builder once it has // been used to build a client. if (initializer != null) { + if (directAppendCalled) { + throw new IllegalStateException("Builder configured with " + HttpInitializer.class.getName() + + " but deprecated methods were also used. The order of operations can not be guaranteed." + + " Please don't mix the initializer with deprecated methods."); + } initializer.initialize(httpServerBuilder); } doAppendHttpServiceFilter(new TimeoutHttpServiceFilter(grpcDetermineTimeout(defaultTimeout), true)); From 4bd3921697a5810ce02c72bf657c2250a54cb529 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dariusz=20J=C4=99drzejczyk?= Date: Tue, 5 Oct 2021 14:55:04 +0200 Subject: [PATCH 3/8] Renamed private variable --- .../grpc/netty/DefaultGrpcServerBuilder.java | 26 +++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/servicetalk-grpc-netty/src/main/java/io/servicetalk/grpc/netty/DefaultGrpcServerBuilder.java b/servicetalk-grpc-netty/src/main/java/io/servicetalk/grpc/netty/DefaultGrpcServerBuilder.java index 18061212cd..c21ea72f14 100644 --- a/servicetalk-grpc-netty/src/main/java/io/servicetalk/grpc/netty/DefaultGrpcServerBuilder.java +++ b/servicetalk-grpc-netty/src/main/java/io/servicetalk/grpc/netty/DefaultGrpcServerBuilder.java @@ -78,13 +78,13 @@ final class DefaultGrpcServerBuilder extends GrpcServerBuilder implements Server @Nullable private Duration defaultTimeout; private boolean invokedBuild; - private boolean directAppendCalled; + private boolean underlyingBuilderCalledDirectly; DefaultGrpcServerBuilder(final HttpServerBuilder httpServerBuilder) { this.httpServerBuilder = httpServerBuilder.protocols(h2Default()).allowDropRequestTrailers(true); appendCatchAllFilterIfRequired(); // Reset the flag guarding proper use of underlying http builder as the above call sets it to true. - this.directAppendCalled = false; + this.underlyingBuilderCalledDirectly = false; } @Override @@ -104,35 +104,35 @@ public GrpcServerBuilder defaultTimeout(Duration defaultTimeout) { @Override public GrpcServerBuilder protocols(final HttpProtocolConfig... protocols) { - directAppendCalled = true; + underlyingBuilderCalledDirectly = true; httpServerBuilder.protocols(protocols); return this; } @Override public GrpcServerBuilder sslConfig(final ServerSslConfig config) { - directAppendCalled = true; + underlyingBuilderCalledDirectly = true; httpServerBuilder.sslConfig(config); return this; } @Override public GrpcServerBuilder sslConfig(final ServerSslConfig defaultConfig, final Map sniMap) { - directAppendCalled = true; + underlyingBuilderCalledDirectly = true; httpServerBuilder.sslConfig(defaultConfig, sniMap); return this; } @Override public GrpcServerBuilder socketOption(final SocketOption option, final T value) { - directAppendCalled = true; + underlyingBuilderCalledDirectly = true; httpServerBuilder.socketOption(option, value); return this; } @Override public GrpcServerBuilder listenSocketOption(final SocketOption option, final T value) { - directAppendCalled = true; + underlyingBuilderCalledDirectly = true; httpServerBuilder.listenSocketOption(option, value); return this; } @@ -140,14 +140,14 @@ public GrpcServerBuilder listenSocketOption(final SocketOption option, fi @Override public GrpcServerBuilder enableWireLogging(final String loggerName, final LogLevel logLevel, final BooleanSupplier logUserData) { - directAppendCalled = true; + underlyingBuilderCalledDirectly = true; httpServerBuilder.enableWireLogging(loggerName, logLevel, logUserData); return this; } @Override public GrpcServerBuilder transportObserver(final TransportObserver transportObserver) { - directAppendCalled = true; + underlyingBuilderCalledDirectly = true; httpServerBuilder.transportObserver(transportObserver); return this; } @@ -160,14 +160,14 @@ public GrpcServerBuilder lifecycleObserver(final GrpcLifecycleObserver lifecycle @Override public GrpcServerBuilder drainRequestPayloadBody(boolean enable) { - directAppendCalled = true; + underlyingBuilderCalledDirectly = true; httpServerBuilder.drainRequestPayloadBody(enable); return this; } @Override public GrpcServerBuilder appendConnectionAcceptorFilter(final ConnectionAcceptorFactory factory) { - directAppendCalled = true; + underlyingBuilderCalledDirectly = true; httpServerBuilder.appendConnectionAcceptorFilter(factory); return this; } @@ -207,7 +207,7 @@ protected Single doListen(final GrpcServiceFactory servi @Override protected void doAppendHttpServiceFilter(final StreamingHttpServiceFilterFactory factory) { - directAppendCalled = true; + underlyingBuilderCalledDirectly = true; httpServerBuilder.appendServiceFilter(factory); } @@ -222,7 +222,7 @@ private HttpServerBuilder preBuild() { // TODO(dj): Consider logging a warning in every method that changes the state of the builder once it has // been used to build a client. if (initializer != null) { - if (directAppendCalled) { + if (underlyingBuilderCalledDirectly) { throw new IllegalStateException("Builder configured with " + HttpInitializer.class.getName() + " but deprecated methods were also used. The order of operations can not be guaranteed." + " Please don't mix the initializer with deprecated methods."); From 944a9cb5ff278e57052d976e52b270e625f9ac00 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dariusz=20J=C4=99drzejczyk?= Date: Wed, 6 Oct 2021 17:12:17 +0200 Subject: [PATCH 4/8] Removed guard against mixing initializer with deprecated method and renamed initializer to initializeHttp --- .../grpc/api/GrpcServerBuilder.java | 26 +++++++++---------- .../grpc/netty/DefaultGrpcServerBuilder.java | 20 +------------- .../grpc/netty/ErrorHandlingTest.java | 2 +- .../netty/GrpcClientRequiresTrailersTest.java | 4 +-- .../GrpcClientValidatesContentTypeTest.java | 4 +-- .../grpc/netty/GrpcLifecycleObserverTest.java | 2 +- .../netty/GrpcServiceContextProtocolTest.java | 2 +- .../GrpcSslAndNonSslConnectionsTest.java | 6 ++--- .../servicetalk/grpc/netty/KeepAliveTest.java | 2 +- .../grpc/netty/ProtocolCompatibilityTest.java | 2 +- .../grpc/netty/TrailersOnlyErrorTest.java | 2 +- 11 files changed, 27 insertions(+), 45 deletions(-) diff --git a/servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcServerBuilder.java b/servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcServerBuilder.java index d4ecd66388..88e3ad3f74 100644 --- a/servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcServerBuilder.java +++ b/servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcServerBuilder.java @@ -74,13 +74,13 @@ public interface HttpInitializer { /** * Set a function which can configure the underlying {@link HttpServerBuilder} used for the transport layer. *

- * Please note that this method can't be mixed with the {@link Deprecated} methods of this class as the order + * Please note that this method shouldn't be mixed with the {@link Deprecated} methods of this class as the order * of operations would not be the same as the order in which the calls are made. Please migrate all of the calls * to this method. * @param initializer Initializes the underlying HTTP transport builder. * @return {@code this}. */ - public GrpcServerBuilder initializer(HttpInitializer initializer) { + public GrpcServerBuilder initializeHttp(HttpInitializer initializer) { throw new UnsupportedOperationException("Initializing the HttpServerBuilder using this method is not yet" + "supported by " + getClass().getName()); } @@ -93,7 +93,7 @@ public GrpcServerBuilder initializer(HttpInitializer initializer) { * * @param protocols {@link HttpProtocolConfig} for each protocol that should be supported. * @return {@code this}. - * @deprecated Call {@link #initializer(HttpInitializer)} and use + * @deprecated Call {@link #initializeHttp(HttpInitializer)} and use * {@link HttpServerBuilder#protocols(HttpProtocolConfig...)} * on the {@code builder} instance by implementing {@link HttpInitializer#initialize(HttpServerBuilder)} * functional interface. @@ -114,7 +114,7 @@ public GrpcServerBuilder initializer(HttpInitializer initializer) { * Set the SSL/TLS configuration. * @param config The configuration to use. * @return {@code this}. - * @deprecated Call {@link #initializer(HttpInitializer)} and use + * @deprecated Call {@link #initializeHttp(HttpInitializer)} and use * {@link HttpServerBuilder#sslConfig(ServerSslConfig)} * on the {@code builder} instance by implementing {@link HttpInitializer#initialize(HttpServerBuilder)} * functional interface. @@ -129,7 +129,7 @@ public GrpcServerBuilder initializer(HttpInitializer initializer) { * @param sniMap A map where the keys are matched against the client certificate's SNI extension value in order * to provide the corresponding {@link ServerSslConfig}. * @return {@code this}. - * @deprecated Call {@link #initializer(HttpInitializer)} and use + * @deprecated Call {@link #initializeHttp(HttpInitializer)} and use * {@link HttpServerBuilder#sslConfig(ServerSslConfig, Map)} * on the {@code builder} instance by implementing {@link HttpInitializer#initialize(HttpServerBuilder)} * functional interface. @@ -146,7 +146,7 @@ public GrpcServerBuilder initializer(HttpInitializer initializer) { * @return {@code this}. * @see StandardSocketOptions * @see ServiceTalkSocketOptions - * @deprecated Call {@link #initializer(HttpInitializer)} and use + * @deprecated Call {@link #initializeHttp(HttpInitializer)} and use * {@link HttpServerBuilder#socketOption(SocketOption, Object)} * on the {@code builder} instance by implementing {@link HttpInitializer#initialize(HttpServerBuilder)} * functional interface. @@ -162,7 +162,7 @@ public GrpcServerBuilder initializer(HttpInitializer initializer) { * @return this. * @see StandardSocketOptions * @see ServiceTalkSocketOptions - * @deprecated Call {@link #initializer(HttpInitializer)} and use + * @deprecated Call {@link #initializeHttp(HttpInitializer)} and use * {@link HttpServerBuilder#listenSocketOption(SocketOption, Object)} * on the {@code builder} instance by implementing {@link HttpInitializer#initialize(HttpServerBuilder)} * functional interface. @@ -178,7 +178,7 @@ public GrpcServerBuilder initializer(HttpInitializer initializer) { * @param logUserData {@code true} to include user data (e.g. data, headers, etc.). {@code false} to exclude user * data and log only network events. * @return {@code this}. - * @deprecated Call {@link #initializer(HttpInitializer)} and use + * @deprecated Call {@link #initializeHttp(HttpInitializer)} and use * {@link HttpServerBuilder#enableWireLogging(String, LogLevel, BooleanSupplier)} * on the {@code builder} instance by implementing {@link HttpInitializer#initialize(HttpServerBuilder)} * functional interface. @@ -192,7 +192,7 @@ public abstract GrpcServerBuilder enableWireLogging(String loggerName, LogLevel * * @param transportObserver A {@link TransportObserver} that provides visibility into transport events. * @return {@code this}. - * @deprecated Call {@link #initializer(HttpInitializer)} and use + * @deprecated Call {@link #initializeHttp(HttpInitializer)} and use * {@link HttpServerBuilder#transportObserver(TransportObserver)} * on the {@code builder} instance by implementing {@link HttpInitializer#initialize(HttpServerBuilder)} * functional interface. @@ -223,7 +223,7 @@ public abstract GrpcServerBuilder enableWireLogging(String loggerName, LogLevel * @param enable When {@code false} it will disable the automatic consumption of request * {@link StreamingHttpRequest#payloadBody()}. * @return {@code this}. - * @deprecated Call {@link #initializer(HttpInitializer)} and use + * @deprecated Call {@link #initializeHttp(HttpInitializer)} and use * {@link HttpServerBuilder#drainRequestPayloadBody(boolean)} * on the {@code builder} instance by implementing {@link HttpInitializer#initialize(HttpServerBuilder)} * functional interface. @@ -247,7 +247,7 @@ public abstract GrpcServerBuilder enableWireLogging(String loggerName, LogLevel * @param factory {@link ConnectionAcceptorFactory} to append. Lifetime of this * {@link ConnectionAcceptorFactory} is managed by this builder and the server started thereof. * @return {@code this}. - * @deprecated Call {@link #initializer(HttpInitializer)} and use + * @deprecated Call {@link #initializeHttp(HttpInitializer)} and use * {@link HttpServerBuilder#appendConnectionAcceptorFilter(ConnectionAcceptorFactory)} * on the {@code builder} instance by implementing {@link HttpInitializer#initialize(HttpServerBuilder)} * functional interface. @@ -268,7 +268,7 @@ public abstract GrpcServerBuilder enableWireLogging(String loggerName, LogLevel * * @param factory {@link StreamingHttpServiceFilterFactory} to append. * @return {@code this}. - * @deprecated Call {@link #initializer(HttpInitializer)} and use + * @deprecated Call {@link #initializeHttp(HttpInitializer)} and use * {@link HttpServerBuilder#appendServiceFilter(StreamingHttpServiceFilterFactory)} * on the {@code builder} instance by implementing {@link HttpInitializer#initialize(HttpServerBuilder)} * functional interface. @@ -294,7 +294,7 @@ public final GrpcServerBuilder appendHttpServiceFilter(StreamingHttpServiceFilte * @param predicate the {@link Predicate} to test if the filter must be applied. * @param factory {@link StreamingHttpServiceFilterFactory} to append. * @return {@code this}. - * @deprecated Call {@link #initializer(HttpInitializer)} and use + * @deprecated Call {@link #initializeHttp(HttpInitializer)} and use * {@link HttpServerBuilder#appendServiceFilter(Predicate, StreamingHttpServiceFilterFactory)} * on the {@code builder} instance by implementing {@link HttpInitializer#initialize(HttpServerBuilder)} * functional interface. diff --git a/servicetalk-grpc-netty/src/main/java/io/servicetalk/grpc/netty/DefaultGrpcServerBuilder.java b/servicetalk-grpc-netty/src/main/java/io/servicetalk/grpc/netty/DefaultGrpcServerBuilder.java index c21ea72f14..8d31755231 100644 --- a/servicetalk-grpc-netty/src/main/java/io/servicetalk/grpc/netty/DefaultGrpcServerBuilder.java +++ b/servicetalk-grpc-netty/src/main/java/io/servicetalk/grpc/netty/DefaultGrpcServerBuilder.java @@ -78,17 +78,14 @@ final class DefaultGrpcServerBuilder extends GrpcServerBuilder implements Server @Nullable private Duration defaultTimeout; private boolean invokedBuild; - private boolean underlyingBuilderCalledDirectly; DefaultGrpcServerBuilder(final HttpServerBuilder httpServerBuilder) { this.httpServerBuilder = httpServerBuilder.protocols(h2Default()).allowDropRequestTrailers(true); appendCatchAllFilterIfRequired(); - // Reset the flag guarding proper use of underlying http builder as the above call sets it to true. - this.underlyingBuilderCalledDirectly = false; } @Override - public GrpcServerBuilder initializer(final GrpcServerBuilder.HttpInitializer initializer) { + public GrpcServerBuilder initializeHttp(final GrpcServerBuilder.HttpInitializer initializer) { this.initializer = initializer; return this; } @@ -104,35 +101,30 @@ public GrpcServerBuilder defaultTimeout(Duration defaultTimeout) { @Override public GrpcServerBuilder protocols(final HttpProtocolConfig... protocols) { - underlyingBuilderCalledDirectly = true; httpServerBuilder.protocols(protocols); return this; } @Override public GrpcServerBuilder sslConfig(final ServerSslConfig config) { - underlyingBuilderCalledDirectly = true; httpServerBuilder.sslConfig(config); return this; } @Override public GrpcServerBuilder sslConfig(final ServerSslConfig defaultConfig, final Map sniMap) { - underlyingBuilderCalledDirectly = true; httpServerBuilder.sslConfig(defaultConfig, sniMap); return this; } @Override public GrpcServerBuilder socketOption(final SocketOption option, final T value) { - underlyingBuilderCalledDirectly = true; httpServerBuilder.socketOption(option, value); return this; } @Override public GrpcServerBuilder listenSocketOption(final SocketOption option, final T value) { - underlyingBuilderCalledDirectly = true; httpServerBuilder.listenSocketOption(option, value); return this; } @@ -140,14 +132,12 @@ public GrpcServerBuilder listenSocketOption(final SocketOption option, fi @Override public GrpcServerBuilder enableWireLogging(final String loggerName, final LogLevel logLevel, final BooleanSupplier logUserData) { - underlyingBuilderCalledDirectly = true; httpServerBuilder.enableWireLogging(loggerName, logLevel, logUserData); return this; } @Override public GrpcServerBuilder transportObserver(final TransportObserver transportObserver) { - underlyingBuilderCalledDirectly = true; httpServerBuilder.transportObserver(transportObserver); return this; } @@ -160,14 +150,12 @@ public GrpcServerBuilder lifecycleObserver(final GrpcLifecycleObserver lifecycle @Override public GrpcServerBuilder drainRequestPayloadBody(boolean enable) { - underlyingBuilderCalledDirectly = true; httpServerBuilder.drainRequestPayloadBody(enable); return this; } @Override public GrpcServerBuilder appendConnectionAcceptorFilter(final ConnectionAcceptorFactory factory) { - underlyingBuilderCalledDirectly = true; httpServerBuilder.appendConnectionAcceptorFilter(factory); return this; } @@ -207,7 +195,6 @@ protected Single doListen(final GrpcServiceFactory servi @Override protected void doAppendHttpServiceFilter(final StreamingHttpServiceFilterFactory factory) { - underlyingBuilderCalledDirectly = true; httpServerBuilder.appendServiceFilter(factory); } @@ -222,11 +209,6 @@ private HttpServerBuilder preBuild() { // TODO(dj): Consider logging a warning in every method that changes the state of the builder once it has // been used to build a client. if (initializer != null) { - if (underlyingBuilderCalledDirectly) { - throw new IllegalStateException("Builder configured with " + HttpInitializer.class.getName() + - " but deprecated methods were also used. The order of operations can not be guaranteed." + - " Please don't mix the initializer with deprecated methods."); - } initializer.initialize(httpServerBuilder); } doAppendHttpServiceFilter(new TimeoutHttpServiceFilter(grpcDetermineTimeout(defaultTimeout), true)); diff --git a/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/ErrorHandlingTest.java b/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/ErrorHandlingTest.java index 0f818a3437..9150743d2e 100644 --- a/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/ErrorHandlingTest.java +++ b/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/ErrorHandlingTest.java @@ -263,7 +263,7 @@ private void setUp(TestMode testMode, GrpcExecutionStrategy serverStrategy, this.requestPublisher = requestPublisher; final StreamingHttpServiceFilterFactory filterFactory = serviceFilterFactory; serverContext = GrpcServers.forAddress(localAddress(0)) - .initializer(builder -> builder.appendServiceFilter(filterFactory)) + .initializeHttp(builder -> builder.appendServiceFilter(filterFactory)) .executionStrategy(serverStrategy).listenAndAwait(serviceFactory); GrpcClientBuilder clientBuilder = GrpcClients.forAddress(serverHostAndPort(serverContext)).appendHttpClientFilter(clientFilterFactory) diff --git a/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/GrpcClientRequiresTrailersTest.java b/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/GrpcClientRequiresTrailersTest.java index c0aeebaf58..2f4c85fcb7 100644 --- a/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/GrpcClientRequiresTrailersTest.java +++ b/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/GrpcClientRequiresTrailersTest.java @@ -60,8 +60,8 @@ class GrpcClientRequiresTrailersTest { private BlockingTesterClient client; private void setUp(boolean hasTrailers) throws Exception { - serverContext = GrpcServers.forAddress(localAddress(0)) - .initializer(builder -> builder.appendServiceFilter(service -> new StreamingHttpServiceFilter(service) { + serverContext = GrpcServers.forAddress(localAddress(0)).initializeHttp(builder -> builder + .appendServiceFilter(service -> new StreamingHttpServiceFilter(service) { @Override public Single handle( final HttpServiceContext ctx, final StreamingHttpRequest request, diff --git a/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/GrpcClientValidatesContentTypeTest.java b/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/GrpcClientValidatesContentTypeTest.java index e4a2f337d6..65d0378e20 100644 --- a/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/GrpcClientValidatesContentTypeTest.java +++ b/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/GrpcClientValidatesContentTypeTest.java @@ -48,8 +48,8 @@ final class GrpcClientValidatesContentTypeTest { private TesterProto.Tester.BlockingTesterClient client; void setUp(boolean withCharset) throws Exception { - serverContext = GrpcServers.forAddress(localAddress(0)) - .initializer(builder -> builder.appendServiceFilter(service -> new StreamingHttpServiceFilter(service) { + serverContext = GrpcServers.forAddress(localAddress(0)).initializeHttp(builder -> builder + .appendServiceFilter(service -> new StreamingHttpServiceFilter(service) { @Override public Single handle( final HttpServiceContext ctx, final StreamingHttpRequest request, diff --git a/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/GrpcLifecycleObserverTest.java b/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/GrpcLifecycleObserverTest.java index e6463fe45f..57129b4695 100644 --- a/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/GrpcLifecycleObserverTest.java +++ b/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/GrpcLifecycleObserverTest.java @@ -142,7 +142,7 @@ private void setUp(boolean error) throws Exception { server = forAddress(localAddress(0)) .ioExecutor(SERVER_CTX.ioExecutor()) .executor(SERVER_CTX.executor()) - .initializer(builder -> builder + .initializeHttp(builder -> builder .enableWireLogging("servicetalk-tests-wire-logger", TRACE, () -> true) .protocols(h2().enableFrameLogging("servicetalk-tests-h2-frame-logger", TRACE, () -> true) .build()) diff --git a/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/GrpcServiceContextProtocolTest.java b/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/GrpcServiceContextProtocolTest.java index cc083ba2e7..fead6a9c01 100644 --- a/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/GrpcServiceContextProtocolTest.java +++ b/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/GrpcServiceContextProtocolTest.java @@ -67,7 +67,7 @@ private void setUp(HttpProtocolVersion httpProtocol, boolean streamingService) t expectedValue = "gRPC over " + httpProtocol; serverContext = GrpcServers.forAddress(localAddress(0)) - .initializer(builder -> builder.protocols(protocolConfig(httpProtocol))) + .initializeHttp(builder -> builder.protocols(protocolConfig(httpProtocol))) .listenAndAwait(streamingService ? new ServiceFactory(new TesterServiceImpl()) : new ServiceFactory(new BlockingTesterServiceImpl())); diff --git a/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/GrpcSslAndNonSslConnectionsTest.java b/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/GrpcSslAndNonSslConnectionsTest.java index 7af8c2152b..f0c7b2d6e8 100644 --- a/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/GrpcSslAndNonSslConnectionsTest.java +++ b/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/GrpcSslAndNonSslConnectionsTest.java @@ -55,7 +55,7 @@ private ServerContext nonSecureGrpcServer() throws Exception { private ServerContext secureGrpcServer() throws Exception { return GrpcServers.forAddress(localAddress(0)) - .initializer(builder -> builder.sslConfig(trustedServerConfig())) + .initializeHttp(builder -> builder.sslConfig(trustedServerConfig())) .listenAndAwait(serviceFactory()); } @@ -141,7 +141,7 @@ void secureClientToSecureServerWithoutPeerHostSucceeds() throws Exception { @Test void noSniClientDefaultServerFallbackSuccess() throws Exception { try (ServerContext serverContext = GrpcServers.forAddress(localAddress(0)) - .initializer(builder -> builder.sslConfig( + .initializeHttp(builder -> builder.sslConfig( trustedServerConfig(), singletonMap(getLoopbackAddress().getHostName(), untrustedServerConfig()) )) @@ -162,7 +162,7 @@ void noSniClientDefaultServerFallbackSuccess() throws Exception { @Test void noSniClientDefaultServerFallbackFailExpected() throws Exception { try (ServerContext serverContext = GrpcServers.forAddress(localAddress(0)) - .initializer(builder -> builder.sslConfig( + .initializeHttp(builder -> builder.sslConfig( untrustedServerConfig(), singletonMap(getLoopbackAddress().getHostName(), trustedServerConfig()) )) diff --git a/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/KeepAliveTest.java b/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/KeepAliveTest.java index 3101eb1cbd..0faf3ce63b 100644 --- a/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/KeepAliveTest.java +++ b/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/KeepAliveTest.java @@ -87,7 +87,7 @@ private void setUp(final boolean keepAlivesFromClient, .executor(SERVER_CTX.executor()) .ioExecutor(SERVER_CTX.ioExecutor()) .executionStrategy(defaultStrategy()) - .initializer(builder -> { + .initializeHttp(builder -> { if (!keepAlivesFromClient) { builder.protocols(h2Config(keepAliveIdleFor)); } else { diff --git a/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/ProtocolCompatibilityTest.java b/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/ProtocolCompatibilityTest.java index 2a7e847433..1bac2244b1 100644 --- a/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/ProtocolCompatibilityTest.java +++ b/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/ProtocolCompatibilityTest.java @@ -1063,7 +1063,7 @@ private static GrpcServerBuilder serviceTalkServerBuilder(final ErrorMode errorM @Nullable final Duration timeout) { final GrpcServerBuilder serverBuilder = GrpcServers.forAddress(localAddress(0)) - .initializer(builder -> { + .initializeHttp(builder -> { builder.appendServiceFilter(service -> new StreamingHttpServiceFilter(service) { @Override public Single handle(final HttpServiceContext ctx, diff --git a/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/TrailersOnlyErrorTest.java b/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/TrailersOnlyErrorTest.java index 18ff3dee04..96987e7e18 100644 --- a/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/TrailersOnlyErrorTest.java +++ b/servicetalk-grpc-netty/src/test/java/io/servicetalk/grpc/netty/TrailersOnlyErrorTest.java @@ -151,7 +151,7 @@ void testServiceFilterThrows() throws Exception { final TesterService service = mockTesterService(); final GrpcServerBuilder serverBuilder = GrpcServers.forAddress(localAddress(0)) - .initializer(builder -> builder.appendServiceFilter(svc -> new StreamingHttpServiceFilter(svc) { + .initializeHttp(builder -> builder.appendServiceFilter(svc -> new StreamingHttpServiceFilter(svc) { @Override public Single handle( final HttpServiceContext ctx, final StreamingHttpRequest request, From c432f1cf4e3868bfb4fd695e793c4950b8858a88 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dariusz=20J=C4=99drzejczyk?= Date: Thu, 7 Oct 2021 10:25:57 +0200 Subject: [PATCH 5/8] Added default implementations for deprecated methods --- .../grpc/api/GrpcServerBuilder.java | 42 ++++++++++++++----- 1 file changed, 32 insertions(+), 10 deletions(-) diff --git a/servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcServerBuilder.java b/servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcServerBuilder.java index 88e3ad3f74..121ecd372a 100644 --- a/servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcServerBuilder.java +++ b/servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcServerBuilder.java @@ -99,7 +99,9 @@ public GrpcServerBuilder initializeHttp(HttpInitializer initializer) { * functional interface. */ @Deprecated - public abstract GrpcServerBuilder protocols(HttpProtocolConfig... protocols); + public GrpcServerBuilder protocols(HttpProtocolConfig... protocols) { + throw new UnsupportedOperationException("Method protocols is not supported by " + getClass().getName()); + } /** * Set a default timeout during which gRPC calls are expected to complete. This default will be used only if the @@ -120,7 +122,9 @@ public GrpcServerBuilder initializeHttp(HttpInitializer initializer) { * functional interface. */ @Deprecated - public abstract GrpcServerBuilder sslConfig(ServerSslConfig config); + public GrpcServerBuilder sslConfig(ServerSslConfig config) { + throw new UnsupportedOperationException("Method sslConfig is not supported by " + getClass().getName()); + } /** * Set the SSL/TLS and SNI configuration. @@ -135,7 +139,9 @@ public GrpcServerBuilder initializeHttp(HttpInitializer initializer) { * functional interface. */ @Deprecated - public abstract GrpcServerBuilder sslConfig(ServerSslConfig defaultConfig, Map sniMap); + public GrpcServerBuilder sslConfig(ServerSslConfig defaultConfig, Map sniMap) { + throw new UnsupportedOperationException("Method sslConfig is not supported by " + getClass().getName()); + } /** * Add a {@link SocketOption} that is applied. @@ -152,7 +158,9 @@ public GrpcServerBuilder initializeHttp(HttpInitializer initializer) { * functional interface. */ @Deprecated - public abstract GrpcServerBuilder socketOption(SocketOption option, T value); + public GrpcServerBuilder socketOption(SocketOption option, T value) { + throw new UnsupportedOperationException("Method socketOption is not supported by " + getClass().getName()); + } /** * Adds a {@link SocketOption} that is applied to the server socket channel which listens/accepts socket channels. @@ -168,7 +176,10 @@ public GrpcServerBuilder initializeHttp(HttpInitializer initializer) { * functional interface. */ @Deprecated - public abstract GrpcServerBuilder listenSocketOption(SocketOption option, T value); + public GrpcServerBuilder listenSocketOption(SocketOption option, T value) { + throw new UnsupportedOperationException("Method listenSocketOption is not supported by " + + getClass().getName()); + } /** * Enables wire-logging for connections created by this builder. @@ -184,8 +195,10 @@ public GrpcServerBuilder initializeHttp(HttpInitializer initializer) { * functional interface. */ @Deprecated - public abstract GrpcServerBuilder enableWireLogging(String loggerName, LogLevel logLevel, - BooleanSupplier logUserData); + public GrpcServerBuilder enableWireLogging(String loggerName, LogLevel logLevel, BooleanSupplier logUserData) { + throw new UnsupportedOperationException("Method enableWireLogging is not supported by " + + getClass().getName()); + } /** * Sets a {@link TransportObserver} that provides visibility into transport events. @@ -198,7 +211,10 @@ public abstract GrpcServerBuilder enableWireLogging(String loggerName, LogLevel * functional interface. */ @Deprecated - public abstract GrpcServerBuilder transportObserver(TransportObserver transportObserver); + public GrpcServerBuilder transportObserver(TransportObserver transportObserver) { + throw new UnsupportedOperationException("Method transportObserver is not supported by " + + getClass().getName()); + } /** * Sets a {@link GrpcLifecycleObserver} that provides visibility into gRPC lifecycle events. @@ -229,7 +245,10 @@ public abstract GrpcServerBuilder enableWireLogging(String loggerName, LogLevel * functional interface. */ @Deprecated - public abstract GrpcServerBuilder drainRequestPayloadBody(boolean enable); + public GrpcServerBuilder drainRequestPayloadBody(boolean enable) { + throw new UnsupportedOperationException("Method drainRequestPayloadBody is not supported by " + + getClass().getName()); + } /** * Append the filter to the chain of filters used to decorate the {@link ConnectionAcceptor} used by this builder. @@ -253,7 +272,10 @@ public abstract GrpcServerBuilder enableWireLogging(String loggerName, LogLevel * functional interface. */ @Deprecated - public abstract GrpcServerBuilder appendConnectionAcceptorFilter(ConnectionAcceptorFactory factory); + public GrpcServerBuilder appendConnectionAcceptorFilter(ConnectionAcceptorFactory factory) { + throw new UnsupportedOperationException("Method appendConnectionAcceptorFilter is not supported by " + + getClass().getName()); + } /** * Append the filter to the chain of filters used to decorate the service used by this builder. From b8fb579765e718ef60b9c60200c9bcbad49cf606 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dariusz=20J=C4=99drzejczyk?= Date: Thu, 7 Oct 2021 11:27:04 +0200 Subject: [PATCH 6/8] Made the builder reconfigurable after first build --- .../grpc/api/GrpcServerBuilder.java | 18 +++-- .../grpc/netty/DefaultGrpcServerBuilder.java | 78 ++++++++++--------- .../servicetalk/grpc/netty/GrpcServers.java | 4 +- 3 files changed, 54 insertions(+), 46 deletions(-) diff --git a/servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcServerBuilder.java b/servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcServerBuilder.java index 121ecd372a..7fb66100ec 100644 --- a/servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcServerBuilder.java +++ b/servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcServerBuilder.java @@ -67,9 +67,14 @@ public interface HttpInitializer { * @param builder The builder to customize the HTTP layer. */ void initialize(HttpServerBuilder builder); - } - private boolean appendedCatchAllFilter; + default HttpInitializer append(HttpInitializer toAppend) { + return builder -> { + initialize(builder); + toAppend.initialize(builder); + }; + } + } /** * Set a function which can configure the underlying {@link HttpServerBuilder} used for the transport layer. @@ -464,14 +469,11 @@ public final ServerContext listenAndAwait(GrpcBindableService... servic protected abstract void doAppendHttpServiceFilter(Predicate predicate, StreamingHttpServiceFilterFactory factory); - protected void appendCatchAllFilterIfRequired() { - // TODO(dj): Move to DefaultGrpcServerBuilder and remove the check as the call is in the constructor. + protected static void appendCatchAllFilter(HttpServerBuilder httpServerBuilder) { + // TODO(dj): Move to DefaultGrpcServerBuilder // This code depends on GrpcUtils which is inaccessible from the servicetalk-grpc-netty module. // When this class is converted to an interface we can also refactor that part. - if (!appendedCatchAllFilter) { - doAppendHttpServiceFilter(CatchAllHttpServiceFilter::new); - appendedCatchAllFilter = true; - } + httpServerBuilder.appendServiceFilter(CatchAllHttpServiceFilter::new); } static final class CatchAllHttpServiceFilter extends StreamingHttpServiceFilter { diff --git a/servicetalk-grpc-netty/src/main/java/io/servicetalk/grpc/netty/DefaultGrpcServerBuilder.java b/servicetalk-grpc-netty/src/main/java/io/servicetalk/grpc/netty/DefaultGrpcServerBuilder.java index 8d31755231..8e0bb8d0e8 100644 --- a/servicetalk-grpc-netty/src/main/java/io/servicetalk/grpc/netty/DefaultGrpcServerBuilder.java +++ b/servicetalk-grpc-netty/src/main/java/io/servicetalk/grpc/netty/DefaultGrpcServerBuilder.java @@ -52,6 +52,7 @@ import java.util.Map; import java.util.function.BooleanSupplier; import java.util.function.Predicate; +import java.util.function.Supplier; import javax.annotation.Nullable; import static io.servicetalk.grpc.api.GrpcExecutionStrategies.defaultStrategy; @@ -59,14 +60,19 @@ import static io.servicetalk.grpc.internal.DeadlineUtils.readTimeoutHeader; import static io.servicetalk.http.netty.HttpProtocolConfigs.h2Default; import static io.servicetalk.utils.internal.DurationUtils.ensurePositive; +import static java.util.Objects.requireNonNull; final class DefaultGrpcServerBuilder extends GrpcServerBuilder implements ServerBinder { private static final Logger LOGGER = LoggerFactory.getLogger(DefaultGrpcServerBuilder.class); - private final HttpServerBuilder httpServerBuilder; - @Nullable - private GrpcServerBuilder.HttpInitializer initializer; + private final Supplier httpServerBuilderSupplier; + private GrpcServerBuilder.HttpInitializer initializer = builder -> { + // no-op + }; + private GrpcServerBuilder.HttpInitializer directCallInitializer = builder -> { + // no-op + }; private final ExecutionContextBuilder contextBuilder = new ExecutionContextBuilder() // Make sure we always set a strategy so that ExecutionContextBuilder does not create a strategy which is // not compatible with gRPC. @@ -77,114 +83,113 @@ final class DefaultGrpcServerBuilder extends GrpcServerBuilder implements Server */ @Nullable private Duration defaultTimeout; - private boolean invokedBuild; - DefaultGrpcServerBuilder(final HttpServerBuilder httpServerBuilder) { - this.httpServerBuilder = httpServerBuilder.protocols(h2Default()).allowDropRequestTrailers(true); - appendCatchAllFilterIfRequired(); + DefaultGrpcServerBuilder(final Supplier httpServerBuilderSupplier) { + this.httpServerBuilderSupplier = () -> httpServerBuilderSupplier.get() + .protocols(h2Default()).allowDropRequestTrailers(true); } @Override public GrpcServerBuilder initializeHttp(final GrpcServerBuilder.HttpInitializer initializer) { - this.initializer = initializer; + this.initializer = requireNonNull(initializer); return this; } @Override public GrpcServerBuilder defaultTimeout(Duration defaultTimeout) { - if (invokedBuild) { - throw new IllegalStateException("default timeout cannot be modified after build, create a new builder"); - } this.defaultTimeout = ensurePositive(defaultTimeout, "defaultTimeout"); return this; } @Override public GrpcServerBuilder protocols(final HttpProtocolConfig... protocols) { - httpServerBuilder.protocols(protocols); + directCallInitializer = directCallInitializer.append(builder -> builder.protocols(protocols)); return this; } @Override public GrpcServerBuilder sslConfig(final ServerSslConfig config) { - httpServerBuilder.sslConfig(config); + directCallInitializer = directCallInitializer.append(builder -> builder.sslConfig(config)); return this; } @Override public GrpcServerBuilder sslConfig(final ServerSslConfig defaultConfig, final Map sniMap) { - httpServerBuilder.sslConfig(defaultConfig, sniMap); + directCallInitializer = directCallInitializer.append(builder -> builder.sslConfig(defaultConfig, sniMap)); return this; } @Override public GrpcServerBuilder socketOption(final SocketOption option, final T value) { - httpServerBuilder.socketOption(option, value); + directCallInitializer = directCallInitializer.append(builder -> builder.socketOption(option, value)); return this; } @Override public GrpcServerBuilder listenSocketOption(final SocketOption option, final T value) { - httpServerBuilder.listenSocketOption(option, value); + directCallInitializer = directCallInitializer.append(builder -> builder.listenSocketOption(option, value)); return this; } @Override public GrpcServerBuilder enableWireLogging(final String loggerName, final LogLevel logLevel, final BooleanSupplier logUserData) { - httpServerBuilder.enableWireLogging(loggerName, logLevel, logUserData); + directCallInitializer = directCallInitializer.append(builder -> + builder.enableWireLogging(loggerName, logLevel, logUserData)); return this; } @Override public GrpcServerBuilder transportObserver(final TransportObserver transportObserver) { - httpServerBuilder.transportObserver(transportObserver); + directCallInitializer = directCallInitializer.append(builder -> builder.transportObserver(transportObserver)); return this; } @Override public GrpcServerBuilder lifecycleObserver(final GrpcLifecycleObserver lifecycleObserver) { - httpServerBuilder.lifecycleObserver(new GrpcToHttpLifecycleObserverBridge(lifecycleObserver)); + directCallInitializer = directCallInitializer.append(builder -> builder + .lifecycleObserver(new GrpcToHttpLifecycleObserverBridge(lifecycleObserver))); return this; } @Override public GrpcServerBuilder drainRequestPayloadBody(boolean enable) { - httpServerBuilder.drainRequestPayloadBody(enable); + directCallInitializer = directCallInitializer.append(builder -> builder.drainRequestPayloadBody(enable)); return this; } @Override public GrpcServerBuilder appendConnectionAcceptorFilter(final ConnectionAcceptorFactory factory) { - httpServerBuilder.appendConnectionAcceptorFilter(factory); + directCallInitializer = directCallInitializer.append(builder -> + builder.appendConnectionAcceptorFilter(factory)); return this; } @Override public GrpcServerBuilder executor(final Executor executor) { contextBuilder.executor(executor); - httpServerBuilder.executor(executor); + directCallInitializer = directCallInitializer.append(builder -> builder.executor(executor)); return this; } @Override public GrpcServerBuilder ioExecutor(final IoExecutor ioExecutor) { contextBuilder.ioExecutor(ioExecutor); - httpServerBuilder.ioExecutor(ioExecutor); + directCallInitializer = directCallInitializer.append(builder -> builder.ioExecutor(ioExecutor)); return this; } @Override public GrpcServerBuilder bufferAllocator(final BufferAllocator allocator) { contextBuilder.bufferAllocator(allocator); - httpServerBuilder.bufferAllocator(allocator); + directCallInitializer = directCallInitializer.append(builder -> builder.bufferAllocator(allocator)); return this; } @Override public GrpcServerBuilder executionStrategy(final GrpcExecutionStrategy strategy) { contextBuilder.executionStrategy(strategy); - httpServerBuilder.executionStrategy(strategy); + directCallInitializer = directCallInitializer.append(builder -> builder.executionStrategy(strategy)); return this; } @@ -195,25 +200,26 @@ protected Single doListen(final GrpcServiceFactory servi @Override protected void doAppendHttpServiceFilter(final StreamingHttpServiceFilterFactory factory) { - httpServerBuilder.appendServiceFilter(factory); + directCallInitializer = directCallInitializer.append(builder -> builder.appendServiceFilter(factory)); } @Override protected void doAppendHttpServiceFilter(final Predicate predicate, final StreamingHttpServiceFilterFactory factory) { - httpServerBuilder.appendServiceFilter(predicate, factory); + directCallInitializer = directCallInitializer.append(builder -> + builder.appendServiceFilter(predicate, factory)); } private HttpServerBuilder preBuild() { - if (!invokedBuild) { - // TODO(dj): Consider logging a warning in every method that changes the state of the builder once it has - // been used to build a client. - if (initializer != null) { - initializer.initialize(httpServerBuilder); - } - doAppendHttpServiceFilter(new TimeoutHttpServiceFilter(grpcDetermineTimeout(defaultTimeout), true)); - } - invokedBuild = true; + final HttpServerBuilder httpServerBuilder = httpServerBuilderSupplier.get(); + + appendCatchAllFilter(httpServerBuilder); + + directCallInitializer.initialize(httpServerBuilder); + initializer.initialize(httpServerBuilder); + + httpServerBuilder.appendServiceFilter( + new TimeoutHttpServiceFilter(grpcDetermineTimeout(defaultTimeout), true)); return httpServerBuilder; } diff --git a/servicetalk-grpc-netty/src/main/java/io/servicetalk/grpc/netty/GrpcServers.java b/servicetalk-grpc-netty/src/main/java/io/servicetalk/grpc/netty/GrpcServers.java index 338b24be82..09e487483e 100644 --- a/servicetalk-grpc-netty/src/main/java/io/servicetalk/grpc/netty/GrpcServers.java +++ b/servicetalk-grpc-netty/src/main/java/io/servicetalk/grpc/netty/GrpcServers.java @@ -36,7 +36,7 @@ private GrpcServers() { * @return a new builder. */ public static GrpcServerBuilder forPort(int port) { - return new DefaultGrpcServerBuilder(HttpServers.forPort(port)); + return new DefaultGrpcServerBuilder(() -> HttpServers.forPort(port)); } /** @@ -46,6 +46,6 @@ public static GrpcServerBuilder forPort(int port) { * @return a new builder. */ public static GrpcServerBuilder forAddress(SocketAddress socketAddress) { - return new DefaultGrpcServerBuilder(HttpServers.forAddress(socketAddress)); + return new DefaultGrpcServerBuilder(() -> HttpServers.forAddress(socketAddress)); } } From 83b600d38fdc37f6ce00f3a9afb0884a52418fff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dariusz=20J=C4=99drzejczyk?= Date: Thu, 7 Oct 2021 12:25:24 +0200 Subject: [PATCH 7/8] Clarified javadoc for lifecycleObserver overriding by initializer --- .../java/io/servicetalk/grpc/api/GrpcServerBuilder.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcServerBuilder.java b/servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcServerBuilder.java index 7fb66100ec..11cf2e1af8 100644 --- a/servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcServerBuilder.java +++ b/servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcServerBuilder.java @@ -19,6 +19,7 @@ import io.servicetalk.concurrent.api.Executor; import io.servicetalk.concurrent.api.Single; import io.servicetalk.http.api.HttpExecutionStrategy; +import io.servicetalk.http.api.HttpLifecycleObserver; import io.servicetalk.http.api.HttpProtocolConfig; import io.servicetalk.http.api.HttpRequest; import io.servicetalk.http.api.HttpServerBuilder; @@ -223,7 +224,10 @@ public GrpcServerBuilder transportObserver(TransportObserver transportObserver) /** * Sets a {@link GrpcLifecycleObserver} that provides visibility into gRPC lifecycle events. - * + *

+ * Note, if {@link #initializeHttp(HttpInitializer)} is used to configure + * {@link HttpServerBuilder#lifecycleObserver(HttpLifecycleObserver)} – that will override the value specified + * using this method. Please choose only one approach. * @param lifecycleObserver A {@link GrpcLifecycleObserver} that provides visibility into gRPC lifecycle events. * @return {@code this}. */ From f18fed7982ce4227b612f94ec2b9bd8670ef4ace Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dariusz=20J=C4=99drzejczyk?= Date: Fri, 8 Oct 2021 15:03:33 +0200 Subject: [PATCH 8/8] Added javadoc for the append method --- .../java/io/servicetalk/grpc/api/GrpcServerBuilder.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcServerBuilder.java b/servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcServerBuilder.java index 11cf2e1af8..adabd3b612 100644 --- a/servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcServerBuilder.java +++ b/servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcServerBuilder.java @@ -69,6 +69,12 @@ public interface HttpInitializer { */ void initialize(HttpServerBuilder builder); + /** + * Appends the passed {@link HttpInitializer} to this {@link HttpInitializer} such that this instance is + * applied first and then the argument's {@link HttpInitializer}. + * @param toAppend {@link HttpInitializer} to append. + * @return A composite {@link HttpInitializer} after the append operation. + */ default HttpInitializer append(HttpInitializer toAppend) { return builder -> { initialize(builder);