core: Remove temporary AbstractManagedChannelImplBuilder

sergiitk · sergiitk · commit 0e03654add90 · 2023-10-03T10:35:16.000-07:00
This breaks the ABI of the classes listed below. Users that recompiled their code using grpc-java [`v1.36.0`] (https://github.com/grpc/grpc-java/releases/tag/v1.36.0) (released on Feb 23, 2021) and later, ARE NOT AFFECTED. Users that compiled their source using grpc-java earlier than [`v1.36.0`] (https://github.com/grpc/grpc-java/releases/tag/v1.36.0) need to recompile when upgrading to grpc-java `v1.59.0`. Otherwise the code will fail on runtime with `NoSuchMethodError`. For example, code: ```java NettyChannelBuilder.forTarget("localhost:100").maxRetryAttempts(2); ``` Will fail with > `java.lang.NoSuchMethodError: 'io.grpc.internal.AbstractManagedChannelImplBuilder io.grpc.netty.NettyChannelBuilder.maxRetryAttempts(int)'` **Affected classes** Class `AbstractManagedChannelImplBuilder` is deleted, and no longer in the class hierarchy of the channel builders: - `io.grpc.netty.NettyChannelBuilder` - `io.grpc.okhttp.OkhttpChannelBuilder` - `grpc.cronet.CronetChannelBuilder`
diff --git a/api/src/main/java/io/grpc/ForwardingChannelBuilder.java b/api/src/main/java/io/grpc/ForwardingChannelBuilder.java
@@ -16,8 +16,6 @@
 
 package io.grpc;
 
-import com.google.common.base.MoreObjects;
-import com.google.errorprone.annotations.DoNotCall;
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.Executor;
@@ -28,40 +26,27 @@
  * A {@link ManagedChannelBuilder} that delegates all its builder methods to another builder by
  * default.
  *
- * @param <T> The type of the subclass extending this abstract class.
+ * <p>Important! Use {@link ForwardingChannelBuilder2} instead!
+ *
+ * <p>This class mistakenly used {@code <T extends ForwardingChannelBuilder<T>>} which causes
+ * return types to be {@link ForwardingChannelBuilder} instead of {@link ManagedChannelBuilder}.
+ * This pollutes the ABI of its subclasses with undesired method signatures.
+ * {@link ForwardingChannelBuilder2} generates correct return types; use it instead.
  *
+ * @param <T> The type of the subclass extending this abstract class.
  * @since 1.7.0
  */
 @ExperimentalApi("https://github.com/grpc/grpc-java/issues/3363")
 public abstract class ForwardingChannelBuilder<T extends ForwardingChannelBuilder<T>>
-    extends ManagedChannelBuilder<T> {
+    extends ForwardingChannelBuilder2<T> {
+  // TODO(sergiitk): deprecate after stabilizing
 
   /**
    * The default constructor.
    */
-  protected ForwardingChannelBuilder() {}
-
-  /**
-   * This method serves to force sub classes to "hide" this static factory.
-   */
-  @DoNotCall("Unsupported")
-  public static ManagedChannelBuilder<?> forAddress(String name, int port) {
-    throw new UnsupportedOperationException("Subclass failed to hide static factory");
+  protected ForwardingChannelBuilder() {
   }
 
-  /**
-   * This method serves to force sub classes to "hide" this static factory.
-   */
-  @DoNotCall("Unsupported")
-  public static ManagedChannelBuilder<?> forTarget(String target) {
-    throw new UnsupportedOperationException("Subclass failed to hide static factory");
-  }
-
-  /**
-   * Returns the delegated {@code ManagedChannelBuilder}.
-   */
-  protected abstract ManagedChannelBuilder<?> delegate();
-
   @Override
   public T directExecutor() {
     delegate().directExecutor();
@@ -249,24 +234,11 @@ public T disableServiceConfigLookUp() {
     return thisT();
   }
 
-  /**
-   * Returns the {@link ManagedChannel} built by the delegate by default. Overriding method can
-   * return different value.
-   */
-  @Override
-  public ManagedChannel build() {
-    return delegate().build();
-  }
-
-  @Override
-  public String toString() {
-    return MoreObjects.toStringHelper(this).add("delegate", delegate()).toString();
-  }
-
   /**
    * Returns the correctly typed version of the builder.
    */
   protected final T thisT() {
+    // TODO(sergiitk): make private when stabilizing.
     @SuppressWarnings("unchecked")
     T thisT = (T) this;
     return thisT;
diff --git a/api/src/main/java/io/grpc/ForwardingChannelBuilder2.java b/api/src/main/java/io/grpc/ForwardingChannelBuilder2.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2020 The gRPC Authors
+ * Copyright 2023 The gRPC Authors
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -14,58 +14,46 @@
  * limitations under the License.
  */
 
-package io.grpc.internal;
+package io.grpc;
 
 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;
-import io.grpc.CompressorRegistry;
-import io.grpc.DecompressorRegistry;
-import io.grpc.ManagedChannel;
-import io.grpc.ManagedChannelBuilder;
-import io.grpc.NameResolver;
-import io.grpc.ProxyDetector;
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.Executor;
 import java.util.concurrent.TimeUnit;
 import javax.annotation.Nullable;
 
 /**
- * Temporarily duplicates {@link io.grpc.ForwardingChannelBuilder} to fix ABI backward
- * compatibility.
+ * A {@link ManagedChannelBuilder} that delegates all its builder methods to another builder by
+ * default.
  *
- * @param <T> The concrete type of this builder.
- * @see <a href="https://github.com/grpc/grpc-java/issues/7211">grpc/grpc-java#7211</a>
+ * <p>Always choose this over {@link ForwardingChannelBuilder}, because
+ * {@link ForwardingChannelBuilder2} is ABI-safe.
+ *
+ * @param <T> The type of the subclass extending this abstract class.
+ * @since 1.59.0
  */
-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;
+@ExperimentalApi("https://github.com/grpc/grpc-java/issues/10585")
+public abstract class ForwardingChannelBuilder2<T extends ManagedChannelBuilder<T>>
+    extends ManagedChannelBuilder<T> {
 
   /**
    * The default constructor.
    */
-  protected AbstractManagedChannelImplBuilder() {}
+  protected ForwardingChannelBuilder2() {
+  }
 
   /**
-   * This method serves to force sub classes to "hide" this static factory.
+   * This method serves to force subclasses to "hide" this static factory.
    */
   @DoNotCall("Unsupported")
   public static ManagedChannelBuilder<?> forAddress(String name, int port) {
     throw new UnsupportedOperationException("Subclass failed to hide static factory");
   }
 
   /**
-   * This method serves to force sub classes to "hide" this static factory.
+   * This method serves to force subclasses to "hide" this static factory.
    */
   @DoNotCall("Unsupported")
   public static ManagedChannelBuilder<?> forTarget(String target) {
@@ -170,31 +158,7 @@ public T idleTimeout(long value, TimeUnit unit) {
 
   @Override
   public T maxInboundMessageSize(int 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;
+    delegate().maxInboundMessageSize(max);
     return thisT();
   }
 
@@ -305,7 +269,7 @@ public String toString() {
   /**
    * Returns the correctly typed version of the builder.
    */
-  protected final T thisT() {
+  private T thisT() {
     @SuppressWarnings("unchecked")
     T thisT = (T) this;
     return thisT;
diff --git a/api/src/test/java/io/grpc/ForwardingChannelBuilder2Test.java b/api/src/test/java/io/grpc/ForwardingChannelBuilder2Test.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2020 The gRPC Authors
+ * Copyright 2023 The gRPC Authors
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -14,33 +14,30 @@
  * limitations under the License.
  */
 
-package io.grpc.internal;
+package io.grpc;
 
 import static com.google.common.truth.Truth.assertThat;
 import static org.mockito.Mockito.doReturn;
 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;
 
 /**
- * Unit tests for {@link AbstractManagedChannelImplBuilder}.
+ * Unit tests for {@link ForwardingChannelBuilder2}.
  */
 @RunWith(JUnit4.class)
-public class AbstractManagedChannelImplBuilderTest {
+public class ForwardingChannelBuilder2Test {
   private final ManagedChannelBuilder<?> mockDelegate = mock(ManagedChannelBuilder.class);
 
-  private final AbstractManagedChannelImplBuilder<?> testChannelBuilder = new TestBuilder();
+  private final ForwardingChannelBuilder2<?> testChannelBuilder = new TestBuilder();
 
-  private final class TestBuilder extends AbstractManagedChannelImplBuilder<TestBuilder> {
+  private final class TestBuilder extends ForwardingChannelBuilder2<TestBuilder> {
     @Override
     protected ManagedChannelBuilder<?> delegate() {
       return mockDelegate;
@@ -53,8 +50,7 @@ public void allMethodsForwarded() throws Exception {
         ManagedChannelBuilder.class,
         mockDelegate,
         testChannelBuilder,
-        // maxInboundMessageSize is the only method that shouldn't forward.
-        ImmutableSet.of(ManagedChannelBuilder.class.getMethod("maxInboundMessageSize", int.class)),
+        Collections.emptyList(),
         new ForwardingTestUtil.ArgumentProvider() {
           @Override
           public Object get(Method method, int argPos, Class<?> clazz) {
@@ -67,15 +63,6 @@ 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/core/build.gradle b/core/build.gradle
@@ -14,10 +14,6 @@ plugins {
     id "ru.vyarus.animalsniffer"
 }
 
-import static java.nio.charset.StandardCharsets.US_ASCII;
-
-import com.google.common.primitives.Bytes;
-
 description = 'gRPC: Core'
 
 dependencies {
@@ -73,50 +69,6 @@ animalsniffer {
 components.java.withVariantsFromConfiguration(configurations.testFixturesApiElements) { skip() }
 components.java.withVariantsFromConfiguration(configurations.testFixturesRuntimeElements) { skip() }
 
-import net.ltgt.gradle.errorprone.CheckSeverity
-
-def replaceBytes(byte[] haystack, byte[] needle, byte[] replacement) {
-    int i = Bytes.indexOf(haystack, needle);
-    assert i != -1;
-    byte[] result = new byte[haystack.length - needle.length + replacement.length];
-    System.arraycopy(haystack, 0, result, 0, i);
-    System.arraycopy(replacement, 0, result, i, replacement.length);
-    System.arraycopy(haystack, i + needle.length, result, i + replacement.length, haystack.length - i - needle.length);
-    return result;
-}
-
-def bigEndianShortBytes(int value) {
-    return [value >> 8, value & 0xFF] as byte[];
-}
-
-def replaceConstant(File file, String needle, String replacement) {
-    // CONSTANT_Utf8_info. https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-4.html#jvms-4.4.7
-    byte[] needleBytes = Bytes.concat(
-        [1] as byte[], bigEndianShortBytes(needle.length()), needle.getBytes(US_ASCII));
-    byte[] replacementBytes = Bytes.concat(
-        [1] as byte[], bigEndianShortBytes(replacement.length()), replacement.getBytes(US_ASCII));
-    file.setBytes(replaceBytes(file.getBytes(), needleBytes, replacementBytes));
-}
-
-plugins.withId("java") {
-    tasks.named("compileJava").configure {
-        doLast {
-            // Replace value of Signature Attribute.
-            // https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-4.html#jvms-4.7.9
-	    //
-	    // Have new compilations compile against a public class, without breaking the internal
-	    // ABI for the moment. After giving users some time to recompile, this should be removed
-	    // and #7211 can continue. When this is removed, at the very least the generics need to
-	    // be changed to avoid <? extends io.grpc.internal.*>.
-            project.replaceConstant(
-                destinationDirectory.file(
-                    "io/grpc/internal/AbstractManagedChannelImplBuilder.class").get().getAsFile(),
-                "<T:Lio/grpc/internal/AbstractManagedChannelImplBuilder<TT;>;>Lio/grpc/ManagedChannelBuilder<TT;>;",
-                "<T:Lio/grpc/ManagedChannelBuilder<TT;>;>Lio/grpc/ManagedChannelBuilder<TT;>;");
-        }
-    }
-}
-
 tasks.register("versionFile") {
     doLast {
         new File(buildDir, "version").write("${project.version}\n")
diff --git a/cronet/src/main/java/io/grpc/cronet/CronetChannelBuilder.java b/cronet/src/main/java/io/grpc/cronet/CronetChannelBuilder.java
@@ -28,9 +28,9 @@
 import io.grpc.ChannelCredentials;
 import io.grpc.ChannelLogger;
 import io.grpc.ExperimentalApi;
+import io.grpc.ForwardingChannelBuilder2;
 import io.grpc.Internal;
 import io.grpc.ManagedChannelBuilder;
-import io.grpc.internal.AbstractManagedChannelImplBuilder;
 import io.grpc.internal.ClientTransportFactory;
 import io.grpc.internal.ConnectionClientTransport;
 import io.grpc.internal.GrpcUtil;
@@ -52,8 +52,7 @@
 
 /** Convenience class for building channels with the cronet transport. */
 @ExperimentalApi("There is no plan to make this API stable, given transport API instability")
-public final class CronetChannelBuilder
-    extends AbstractManagedChannelImplBuilder<CronetChannelBuilder> {
+public final class CronetChannelBuilder extends ForwardingChannelBuilder2<CronetChannelBuilder> {
 
   private static final String LOG_TAG = "CronetChannelBuilder";
 
diff --git a/inprocess/src/main/java/io/grpc/inprocess/InProcessChannelBuilder.java b/inprocess/src/main/java/io/grpc/inprocess/InProcessChannelBuilder.java
@@ -23,9 +23,9 @@
 import io.grpc.ChannelCredentials;
 import io.grpc.ChannelLogger;
 import io.grpc.ExperimentalApi;
+import io.grpc.ForwardingChannelBuilder2;
 import io.grpc.Internal;
 import io.grpc.ManagedChannelBuilder;
-import io.grpc.internal.AbstractManagedChannelImplBuilder;
 import io.grpc.internal.ClientTransportFactory;
 import io.grpc.internal.ConnectionClientTransport;
 import io.grpc.internal.GrpcUtil;
@@ -47,7 +47,8 @@
  */
 @ExperimentalApi("https://github.com/grpc/grpc-java/issues/1783")
 public final class InProcessChannelBuilder extends
-    AbstractManagedChannelImplBuilder<InProcessChannelBuilder> {
+    ForwardingChannelBuilder2<InProcessChannelBuilder> {
+
   /**
    * Create a channel builder that will connect to the server with the given name.
    *
diff --git a/interop-testing/src/test/java/io/grpc/ChannelAndServerBuilderTest.java b/interop-testing/src/test/java/io/grpc/ChannelAndServerBuilderTest.java
@@ -66,8 +66,11 @@ public static Collection<Object[]> params() throws Exception {
       Class<?> clazz = Class.forName(className, false /*initialize*/, loader);
       if (ServerBuilder.class.isAssignableFrom(clazz) && clazz != ServerBuilder.class) {
         classes.add(new Object[]{clazz});
-      } else if (ManagedChannelBuilder.class.isAssignableFrom(clazz)
-          && clazz != ManagedChannelBuilder.class) {
+        continue;
+      }
+      // ForwardingChannelBuilder extends ForwardingChannelBuilder2, not need for extra checks.
+      if (ManagedChannelBuilder.class.isAssignableFrom(clazz)
+          && clazz != ManagedChannelBuilder.class && clazz != ForwardingChannelBuilder.class) {
         classes.add(new Object[]{clazz});
       }
     }
diff --git a/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java b/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java
diff --git a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java

Original file line number	Diff line number	Diff line change
`@@ -66,8 +66,11 @@ public static Collection<Object[]> params() throws Exception {`
`66`	`66`	`Class<?> clazz = Class.forName(className, false /initialize/, loader);`
`67`	`67`	`if (ServerBuilder.class.isAssignableFrom(clazz) && clazz != ServerBuilder.class) {`
`68`	`68`	`classes.add(new Object[]{clazz});`
`69`		`- } else if (ManagedChannelBuilder.class.isAssignableFrom(clazz)`
`70`		`- && clazz != ManagedChannelBuilder.class) {`
	`69`	`+ continue;`
	`70`	`+ }`
	`71`	`+ // ForwardingChannelBuilder extends ForwardingChannelBuilder2, not need for extra checks.`
	`72`	`+ if (ManagedChannelBuilder.class.isAssignableFrom(clazz)`
	`73`	`+ && clazz != ManagedChannelBuilder.class && clazz != ForwardingChannelBuilder.class) {`
`71`	`74`	`classes.add(new Object[]{clazz});`
`72`	`75`	`}`
`73`	`76`	`}`