Fix AbstractManagedChannelImplBuilder#maxInboundMessageSize(int) ABI (#…

…8607)

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.

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();
   }
 

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()) {

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;

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;
 
   /**