diff --git a/api/src/main/java/io/grpc/ForwardingChannelBuilder.java b/api/src/main/java/io/grpc/ForwardingChannelBuilder.java index b584e7132e99..dea6c79c1842 100644 --- 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 The type of the subclass extending this abstract class. + *

Important! Use {@link ForwardingChannelBuilder2} instead! + * + *

This class mistakenly used {@code >} 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 return correct types; use it instead. * + * @param 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> - extends ManagedChannelBuilder { + extends ForwardingChannelBuilder2 { + // 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/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java b/api/src/main/java/io/grpc/ForwardingChannelBuilder2.java similarity index 67% rename from core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java rename to api/src/main/java/io/grpc/ForwardingChannelBuilder2.java index dd8328ee09cd..10d7155ca144 100644 --- a/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.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,19 +14,10 @@ * 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; @@ -34,30 +25,27 @@ 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 The concrete type of this builder. - * @see grpc/grpc-java#7211 + *

Always choose this over {@link ForwardingChannelBuilder}, because + * {@link ForwardingChannelBuilder2} is ABI-safe. + * + * @param The type of the subclass extending this abstract class. + * @since 1.59.0 */ -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; +@ExperimentalApi("https://github.com/grpc/grpc-java/issues/TODO") +public abstract class ForwardingChannelBuilder2> + extends ManagedChannelBuilder { /** * 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) { @@ -65,7 +53,7 @@ public static ManagedChannelBuilder forAddress(String name, int port) { } /** - * 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/core/src/test/java/io/grpc/internal/AbstractManagedChannelImplBuilderTest.java b/api/src/test/java/io/grpc/ForwardingChannelBuilder2Test.java similarity index 74% rename from core/src/test/java/io/grpc/internal/AbstractManagedChannelImplBuilderTest.java rename to api/src/test/java/io/grpc/ForwardingChannelBuilder2Test.java index 81c50b19fbf9..491dd076c79b 100644 --- a/core/src/test/java/io/grpc/internal/AbstractManagedChannelImplBuilderTest.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 { + private final class TestBuilder extends ForwardingChannelBuilder2 { @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 index 14ad2e748f6e..9c81e33f372f 100644 --- 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 . - project.replaceConstant( - destinationDirectory.file( - "io/grpc/internal/AbstractManagedChannelImplBuilder.class").get().getAsFile(), - ";>Lio/grpc/ManagedChannelBuilder;", - ";>Lio/grpc/ManagedChannelBuilder;"); - } - } -} - 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 index 22778691a101..066992018ed3 100644 --- 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 { +public final class CronetChannelBuilder extends ForwardingChannelBuilder2 { 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 index 35998a535e2d..aa53dbc87190 100644 --- 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 { + ForwardingChannelBuilder2 { + /** * 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 index 6d082d404215..ba21e78c9573 100644 --- 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 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 index da7fe84d9cb8..138e11f6dfe3 100644 --- a/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java +++ b/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java @@ -33,10 +33,10 @@ import io.grpc.ChannelLogger; import io.grpc.EquivalentAddressGroup; import io.grpc.ExperimentalApi; +import io.grpc.ForwardingChannelBuilder2; import io.grpc.HttpConnectProxiedSocketAddress; import io.grpc.Internal; import io.grpc.ManagedChannelBuilder; -import io.grpc.internal.AbstractManagedChannelImplBuilder; import io.grpc.internal.AtomicBackoff; import io.grpc.internal.ClientTransportFactory; import io.grpc.internal.ConnectionClientTransport; @@ -72,8 +72,7 @@ */ @ExperimentalApi("https://github.com/grpc/grpc-java/issues/1784") @CheckReturnValue -public final class NettyChannelBuilder extends - AbstractManagedChannelImplBuilder { +public final class NettyChannelBuilder extends ForwardingChannelBuilder2 { // 1MiB. public static final int DEFAULT_FLOW_CONTROL_WINDOW = 1024 * 1024; @@ -102,6 +101,7 @@ public final class NettyChannelBuilder extends private boolean autoFlowControl = DEFAULT_AUTO_FLOW_CONTROL; private int flowControlWindow = DEFAULT_FLOW_CONTROL_WINDOW; private int maxHeaderListSize = GrpcUtil.DEFAULT_MAX_HEADER_LIST_SIZE; + private int maxInboundMessageSize = GrpcUtil.DEFAULT_MAX_MESSAGE_SIZE; private long keepAliveTimeNanos = KEEPALIVE_TIME_NANOS_DISABLED; private long keepAliveTimeoutNanos = DEFAULT_KEEPALIVE_TIMEOUT_NANOS; private boolean keepAliveWithoutCalls; diff --git a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java index a3f99b67cfa3..8e9ed75bf76a 100644 --- a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java +++ b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java @@ -29,11 +29,11 @@ import io.grpc.CompositeCallCredentials; import io.grpc.CompositeChannelCredentials; import io.grpc.ExperimentalApi; +import io.grpc.ForwardingChannelBuilder2; import io.grpc.InsecureChannelCredentials; import io.grpc.Internal; import io.grpc.ManagedChannelBuilder; import io.grpc.TlsChannelCredentials; -import io.grpc.internal.AbstractManagedChannelImplBuilder; import io.grpc.internal.AtomicBackoff; import io.grpc.internal.ClientTransportFactory; import io.grpc.internal.ConnectionClientTransport; @@ -83,8 +83,7 @@ /** Convenience class for building channels with the OkHttp transport. */ @ExperimentalApi("https://github.com/grpc/grpc-java/issues/1785") -public final class OkHttpChannelBuilder extends - AbstractManagedChannelImplBuilder { +public final class OkHttpChannelBuilder extends ForwardingChannelBuilder2 { private static final Logger log = Logger.getLogger(OkHttpChannelBuilder.class.getName()); public static final int DEFAULT_FLOW_CONTROL_WINDOW = 65535; @@ -188,6 +187,7 @@ 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; /**