Skip to content

Commit

Permalink
core: Remove temporary AbstractManagedChannelImplBuilder
Browse files Browse the repository at this point in the history
  • Loading branch information
@sergiitk
sergiitk committed Oct 2, 2023
1 parent 269b8f0 commit 07688f5
Show file tree
Hide file tree
Showing 9 changed files with 53 additions and 175 deletions.
50 changes: 11 additions & 39 deletions api/src/main/java/io/grpc/ForwardingChannelBuilder.java
View file
Expand Up @@ -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;
Expand All @@ -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 return correct 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();
Expand Down Expand Up @@ -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;
Expand Down
72 changes: 18 additions & 54 deletions ...al/AbstractManagedChannelImplBuilder.java → ...va/io/grpc/ForwardingChannelBuilder2.java
View file
@@ -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.
Expand All @@ -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) {
Expand Down Expand Up @@ -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();
}

Expand Down Expand Up @@ -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;
Expand Down
29 changes: 8 additions & 21 deletions ...bstractManagedChannelImplBuilderTest.java → ...o/grpc/ForwardingChannelBuilder2Test.java
View file
@@ -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.
Expand All @@ -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;
Expand All @@ -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) {
Expand All @@ -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()) {
Expand Down
48 changes: 0 additions & 48 deletions core/build.gradle
View file
Expand Up @@ -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 {
Expand Down Expand Up @@ -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")
Expand Down
5 changes: 2 additions & 3 deletions cronet/src/main/java/io/grpc/cronet/CronetChannelBuilder.java
View file
Expand Up @@ -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;
Expand All @@ -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";

Expand Down
5 changes: 3 additions & 2 deletions inprocess/src/main/java/io/grpc/inprocess/InProcessChannelBuilder.java
View file
Expand Up @@ -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;
Expand All @@ -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.
*
Expand Down
7 changes: 5 additions & 2 deletions interop-testing/src/test/java/io/grpc/ChannelAndServerBuilderTest.java
View file
Expand Up @@ -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});
}
}
Expand Down

0 comments on commit 07688f5

Please sign in to comment.