Fix AbstractManagedChannelImplBuilder#maxInboundMessageSize(int) ABI (#8607)

sergiitk · web-flow · commit 0376de15b8ba · 2021-10-14T17:25:06.000-07:00
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.
diff --git 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
     <T extends AbstractManagedChannelImplBuilder<T>> extends ManagedChannelBuilder<T> {
 
+  /**
+   * Added for ABI compatibility.
+   *
+   * <p>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
@@ -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.<Method>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
@@ -99,7 +99,6 @@ public final class NettyChannelBuilder extends
   private ObjectPool<? extends EventLoopGroup> 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
@@ -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;
 
   /**
