From 6a7c14225c9f9c6554dfb09fdc92bc2ecd55968b Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Thu, 14 Oct 2021 13:18:16 -0700 Subject: [PATCH] Fix AbstractManagedChannelImplBuilder#maxInboundMessageSize(int) ABI As part of refactoring described in issue #7211, the implementation of this method, and its corresponding field was pulled down from internal AbstractManagedChannelImplBuilder to concrete classes that actually enforce this setting. That's the right place for method's implementation, so it wasn't ported to ManagedChannelImplBuilder too. Then AbstractManagedChannelImplBuilder was brought to fix ABI backward compatibility, and temporarily turned into a ForwardingChannelBuilder, see PR #7564. Eventually it will be deleted, after a period with "bridge" solution added in #7834. However, this bringing AbstractManagedChannelImplBuilder back fix unintentionally made this method's ABI backward incompatible: pre-refactoring builds expect maxInboundMessageSize() to be a method of AbstractManagedChannelImplBuilder, and not concrete classes. This problem was caught in #8313. Since the end goal is to keep only this method in concrete classes the need it, to fix its ABI issue, we temporary reintroduce it to the original layer it was removed from, AbstractManagedChannelImplBuilder. This class is also temporary, and its only intention is a ABI compatibility. Once we move forward with dropping ABI compatibility (with #7834), this fix is also no longer necessary, and will go away with AbstractManagedChannelImplBuilder. --- .../AbstractManagedChannelImplBuilder.java | 36 ++++++++++++++++++- ...AbstractManagedChannelImplBuilderTest.java | 14 ++++++-- .../io/grpc/netty/NettyChannelBuilder.java | 5 ++- .../io/grpc/okhttp/OkHttpChannelBuilder.java | 1 - 4 files changed, 51 insertions(+), 5 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java index 98bbfcc7b1ec..f74762f2bd13 100644 --- a/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java @@ -17,6 +17,7 @@ package io.grpc.internal; import com.google.common.base.MoreObjects; +import com.google.common.base.Preconditions; import com.google.errorprone.annotations.DoNotCall; import io.grpc.BinaryLog; import io.grpc.ClientInterceptor; @@ -42,6 +43,14 @@ public abstract class AbstractManagedChannelImplBuilder > extends ManagedChannelBuilder { + /** + * Added for ABI compatibility. + * + *

See details in {@link #maxInboundMessageSize(int)}. + * TODO(sergiitk): move back to concrete classes as a private field, when this class is removed. + */ + protected int maxInboundMessageSize = GrpcUtil.DEFAULT_MAX_MESSAGE_SIZE; + /** * The default constructor. */ @@ -161,7 +170,32 @@ public T idleTimeout(long value, TimeUnit unit) { @Override public T maxInboundMessageSize(int max) { - delegate().maxInboundMessageSize(max); + /* + Why this method is not delegating, as the rest of the methods? + + As part of refactoring described in issue #7211, the implementation of this method, + and its corresponding field was pulled down from internal AbstractManagedChannelImplBuilder + to concrete classes that actually enforce this setting. That's the right place for + method's implementation, so it wasn't ported to ManagedChannelImplBuilder too. + + Then AbstractManagedChannelImplBuilder was brought to fix ABI backward compatibility, + and temporarily turned into a ForwardingChannelBuilder, see PR #7564. Eventually it will + be deleted, after a period with "bridge" solution added in #7834. + + However, this bringing AbstractManagedChannelImplBuilder back fix unintentionally + made this method's ABI backward incompatible: pre-refactoring builds expect + maxInboundMessageSize() to be a method of AbstractManagedChannelImplBuilder, and not + concrete classes, see https://gist.github.com/sergiitk/39583f20906df1813f5e170317a35dc4#v1322. + This problem was caught in #8313. + + Since the end goal is to keep only this method in concrete classes the need it, + to fix its ABI issue, we temporary reintroduce it to the original layer it was removed from, + AbstractManagedChannelImplBuilder. This class is also temporary, and its only intention + is a ABI compatibility. Once we move forward with dropping ABI compatibility (with #7834), + this fix is also no longer necessary, and will go away with AbstractManagedChannelImplBuilder. + */ + Preconditions.checkArgument(max >= 0, "negative max"); + maxInboundMessageSize = max; return thisT(); } diff --git a/core/src/test/java/io/grpc/internal/AbstractManagedChannelImplBuilderTest.java b/core/src/test/java/io/grpc/internal/AbstractManagedChannelImplBuilderTest.java index 8643165eec69..81c50b19fbf9 100644 --- a/core/src/test/java/io/grpc/internal/AbstractManagedChannelImplBuilderTest.java +++ b/core/src/test/java/io/grpc/internal/AbstractManagedChannelImplBuilderTest.java @@ -21,12 +21,12 @@ import static org.mockito.Mockito.mock; import com.google.common.base.Defaults; +import com.google.common.collect.ImmutableSet; import io.grpc.ForwardingTestUtil; import io.grpc.ManagedChannel; import io.grpc.ManagedChannelBuilder; import java.lang.reflect.Method; import java.lang.reflect.Modifier; -import java.util.Collections; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -53,7 +53,8 @@ public void allMethodsForwarded() throws Exception { ManagedChannelBuilder.class, mockDelegate, testChannelBuilder, - Collections.emptyList(), + // maxInboundMessageSize is the only method that shouldn't forward. + ImmutableSet.of(ManagedChannelBuilder.class.getMethod("maxInboundMessageSize", int.class)), new ForwardingTestUtil.ArgumentProvider() { @Override public Object get(Method method, int argPos, Class clazz) { @@ -66,6 +67,15 @@ public Object get(Method method, int argPos, Class clazz) { }); } + @Test + public void testMaxInboundMessageSize() { + assertThat(testChannelBuilder.maxInboundMessageSize) + .isEqualTo(GrpcUtil.DEFAULT_MAX_MESSAGE_SIZE); + + testChannelBuilder.maxInboundMessageSize(42); + assertThat(testChannelBuilder.maxInboundMessageSize).isEqualTo(42); + } + @Test public void allBuilderMethodsReturnThis() throws Exception { for (Method method : ManagedChannelBuilder.class.getDeclaredMethods()) { diff --git a/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java b/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java index 809c94f12ce8..d25e7fa467ce 100644 --- a/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java +++ b/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java @@ -99,7 +99,6 @@ public final class NettyChannelBuilder extends private ObjectPool eventLoopGroupPool = DEFAULT_EVENT_LOOP_GROUP_POOL; 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; @@ -522,6 +521,10 @@ public NettyChannelBuilder maxInboundMessageSize(int max) { return this; } + public int maxInboundMessageSize() { + return maxInboundMessageSize; + } + @CheckReturnValue ClientTransportFactory buildTransportFactory() { assertEventLoopAndChannelType(); diff --git a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java index f7d0d973802b..af5ebe2886cc 100644 --- a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java +++ b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java @@ -174,7 +174,6 @@ public static OkHttpChannelBuilder forTarget(String target, ChannelCredentials c private long keepAliveTimeoutNanos = DEFAULT_KEEPALIVE_TIMEOUT_NANOS; private int flowControlWindow = DEFAULT_FLOW_CONTROL_WINDOW; private boolean keepAliveWithoutCalls; - private int maxInboundMessageSize = GrpcUtil.DEFAULT_MAX_MESSAGE_SIZE; private int maxInboundMetadataSize = Integer.MAX_VALUE; /**