From 06e81fa96ca897461f2ba7b5640cf43c4bd1a91d 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 In refactoring described in #7211, the implementation of #maxInboundMessageSize(int) (and its corresponding field) were pulled down from internal AbstractManagedChannelImplBuilder to concrete classes that actually enforce this setting. For the same reason, it wasn't ported to ManagedChannelImplBuilder (the #delegate()). Then AbstractManagedChannelImplBuilder was brought back to fix ABI backward compatibility, and temporarily turned into a ForwardingChannelBuilder, ref PR #7564. Eventually it will be deleted, after a period with "bridge" ABI solution introduced in #7834. However, restoring AbstractManagedChannelImplBuilder unintentionally made ABI of pre-refactoring builds expect it to be a method of AbstractManagedChannelImplBuilder, and not concrete classes, ref #8313. The end goal is to keep #maxInboundMessageSize(int) only in concrete classes that enforce it. To fix method's ABI, we temporary reintroduce it to the original layer it was removed from: AbstractManagedChannelImplBuilder. This class' only intention is to provide short-term ABI compatibility. Once we move forward with dropping the ABI, both fixes are no longer necessary, and both will perish with removing AbstractManagedChannelImplBuilder. --- .../AbstractManagedChannelImplBuilder.java | 35 ++++++++++++++++++- ...AbstractManagedChannelImplBuilderTest.java | 14 ++++++-- .../io/grpc/netty/NettyChannelBuilder.java | 1 - .../io/grpc/okhttp/OkHttpChannelBuilder.java | 1 - 4 files changed, 46 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 98bbfcc7b1e..dd8328ee09c 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,31 @@ 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? + + In refactoring described in #7211, the implementation of #maxInboundMessageSize(int) + (and its corresponding field) was pulled down from internal AbstractManagedChannelImplBuilder + to concrete classes that actually enforce this setting. For the same reason, it wasn't ported + to ManagedChannelImplBuilder (the #delegate()). + + Then AbstractManagedChannelImplBuilder was brought back to fix ABI backward compatibility, + and temporarily turned into a ForwardingChannelBuilder, ref PR #7564. Eventually it will + be deleted, after a period with "bridge" ABI solution introduced in #7834. + + However, restoring AbstractManagedChannelImplBuilder unintentionally made ABI of + #maxInboundMessageSize(int) implemented by the concrete classes backward incompatible: + pre-refactoring builds expect it to be a method of AbstractManagedChannelImplBuilder, + and not concrete classes, ref #8313. + + The end goal is to keep #maxInboundMessageSize(int) only in concrete classes that enforce it. + To fix method's ABI, we temporary reintroduce it to the original layer it was removed from: + AbstractManagedChannelImplBuilder. This class' only intention is to provide short-term + ABI compatibility. Once we move forward with dropping the ABI, both fixes are no longer + necessary, and both will perish with removing 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 8643165eec6..81c50b19fbf 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 809c94f12ce..17801afa382 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; diff --git a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java index f7d0d973802..af5ebe2886c 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; /**