Fix AbstractManagedChannelImplBuilder#maxInboundMessageSize(int) ABI

As part of refactoring described in issue grpc#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 grpc#7564. Eventually it will
be deleted, after a period with "bridge" solution added in grpc#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 grpc#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 grpc#7834),
this fix is also no longer necessary, and will go away with AbstractManagedChannelImplBuilder.

Author: sergiitk

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

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