From f397a293e2580061a7472854db90580ff088fb20 Mon Sep 17 00:00:00 2001 From: Sanjay Pujare Date: Wed, 22 Jun 2022 23:36:42 -0700 Subject: [PATCH 1/2] api,core: change ManagedChannel and Server Builders to use GlobalInterceptors also added a getter in GlobalInterceptors to expose the set flag --- .../main/java/io/grpc/GlobalInterceptors.java | 4 + .../io/grpc/InternalGlobalInterceptors.java | 4 + .../internal/ManagedChannelImplBuilder.java | 14 ++- .../io/grpc/internal/ServerImplBuilder.java | 15 ++- .../ManagedChannelImplBuilderTest.java | 99 ++++++++++++++++++- .../grpc/internal/ServerImplBuilderTest.java | 97 ++++++++++++++++++ 6 files changed, 226 insertions(+), 7 deletions(-) diff --git a/api/src/main/java/io/grpc/GlobalInterceptors.java b/api/src/main/java/io/grpc/GlobalInterceptors.java index c2378437313..2430bb79b3f 100644 --- a/api/src/main/java/io/grpc/GlobalInterceptors.java +++ b/api/src/main/java/io/grpc/GlobalInterceptors.java @@ -33,6 +33,10 @@ final class GlobalInterceptors { // Prevent instantiation private GlobalInterceptors() {} + static boolean isIsGlobalInterceptorsTracersSet() { + return isGlobalInterceptorsTracersSet; + } + /** * Sets the list of global interceptors and global server stream tracers. * diff --git a/api/src/main/java/io/grpc/InternalGlobalInterceptors.java b/api/src/main/java/io/grpc/InternalGlobalInterceptors.java index db0ff6e2ce9..dac37797782 100644 --- a/api/src/main/java/io/grpc/InternalGlobalInterceptors.java +++ b/api/src/main/java/io/grpc/InternalGlobalInterceptors.java @@ -42,5 +42,9 @@ public static List getServerStreamTracerFactories() return GlobalInterceptors.getServerStreamTracerFactories(); } + public static boolean isGlobalInterceptorsTracersSet() { + return GlobalInterceptors.isIsGlobalInterceptorsTracersSet(); + } + private InternalGlobalInterceptors() {} } diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java index 243a555ad1a..6210f4f039d 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java @@ -31,6 +31,7 @@ import io.grpc.DecompressorRegistry; import io.grpc.EquivalentAddressGroup; import io.grpc.InternalChannelz; +import io.grpc.InternalGlobalInterceptors; import io.grpc.ManagedChannel; import io.grpc.ManagedChannelBuilder; import io.grpc.NameResolver; @@ -636,9 +637,14 @@ public ManagedChannel build() { // TODO(zdapeng): FIX IT @VisibleForTesting List getEffectiveInterceptors() { - List effectiveInterceptors = - new ArrayList<>(this.interceptors); - if (statsEnabled) { + List effectiveInterceptors = new ArrayList<>(this.interceptors); + boolean isGlobalInterceptorsSet = InternalGlobalInterceptors.isGlobalInterceptorsTracersSet(); + List globalClientInterceptors = + InternalGlobalInterceptors.getClientInterceptors(); + if (isGlobalInterceptorsSet) { + effectiveInterceptors.addAll(globalClientInterceptors); + } + if (!isGlobalInterceptorsSet && statsEnabled) { ClientInterceptor statsInterceptor = null; try { Class censusStatsAccessor = @@ -674,7 +680,7 @@ List getEffectiveInterceptors() { effectiveInterceptors.add(0, statsInterceptor); } } - if (tracingEnabled) { + if (!isGlobalInterceptorsSet && tracingEnabled) { ClientInterceptor tracingInterceptor = null; try { Class censusTracingAccessor = diff --git a/core/src/main/java/io/grpc/internal/ServerImplBuilder.java b/core/src/main/java/io/grpc/internal/ServerImplBuilder.java index 277e476143d..dc3147bc2f1 100644 --- a/core/src/main/java/io/grpc/internal/ServerImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/ServerImplBuilder.java @@ -30,6 +30,7 @@ import io.grpc.DecompressorRegistry; import io.grpc.HandlerRegistry; import io.grpc.InternalChannelz; +import io.grpc.InternalGlobalInterceptors; import io.grpc.Server; import io.grpc.ServerBuilder; import io.grpc.ServerCallExecutorSupplier; @@ -246,7 +247,17 @@ public Server build() { @VisibleForTesting List getTracerFactories() { ArrayList tracerFactories = new ArrayList<>(); - if (statsEnabled) { + boolean isGlobalInterceptorsTracersSet + = InternalGlobalInterceptors.isGlobalInterceptorsTracersSet(); + List globalServerInterceptors + = InternalGlobalInterceptors.getServerInterceptors(); + List globalServerStreamTracerFactories + = InternalGlobalInterceptors.getServerStreamTracerFactories(); + if (isGlobalInterceptorsTracersSet) { + tracerFactories.addAll(globalServerStreamTracerFactories); + interceptors.addAll(globalServerInterceptors); + } + if (!isGlobalInterceptorsTracersSet && statsEnabled) { ServerStreamTracer.Factory censusStatsTracerFactory = null; try { Class censusStatsAccessor = @@ -278,7 +289,7 @@ List getTracerFactories() { tracerFactories.add(censusStatsTracerFactory); } } - if (tracingEnabled) { + if (!isGlobalInterceptorsTracersSet && tracingEnabled) { ServerStreamTracer.Factory tracingStreamTracerFactory = null; try { Class censusTracingAccessor = diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java index bc5b7cde651..286c48ebd62 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java +++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java @@ -35,9 +35,11 @@ import io.grpc.ClientInterceptor; import io.grpc.CompressorRegistry; import io.grpc.DecompressorRegistry; +import io.grpc.InternalGlobalInterceptors; import io.grpc.ManagedChannel; import io.grpc.MethodDescriptor; import io.grpc.NameResolver; +import io.grpc.StaticTestingClassLoader; import io.grpc.internal.ManagedChannelImplBuilder.ChannelBuilderDefaultPortProvider; import io.grpc.internal.ManagedChannelImplBuilder.ClientTransportFactoryBuilder; import io.grpc.internal.ManagedChannelImplBuilder.FixedPortProvider; @@ -47,12 +49,14 @@ import java.net.SocketAddress; import java.net.URI; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.concurrent.Executor; import java.util.concurrent.TimeUnit; +import java.util.regex.Pattern; import org.junit.Before; import org.junit.Rule; import org.junit.Test; @@ -78,6 +82,14 @@ public ClientCall interceptCall( return next.newCall(method, callOptions); } }; + private static final ClientInterceptor DUMMY_USER_INTERCEPTOR1 = + new ClientInterceptor() { + @Override + public ClientCall interceptCall( + MethodDescriptor method, CallOptions callOptions, Channel next) { + return next.newCall(method, callOptions); + } + }; @Rule public final MockitoRule mocks = MockitoJUnit.rule(); @SuppressWarnings("deprecation") // https://github.com/grpc/grpc-java/issues/7467 @@ -90,7 +102,12 @@ public ClientCall interceptCall( private ManagedChannelImplBuilder builder; private ManagedChannelImplBuilder directAddressBuilder; private final FakeClock clock = new FakeClock(); - + private final StaticTestingClassLoader classLoader = + new StaticTestingClassLoader( + getClass().getClassLoader(), + Pattern.compile( + "io\\.grpc\\.InternalGlobalInterceptors|io\\.grpc\\.GlobalInterceptors|" + + "io\\.grpc\\.internal\\.[^.]+")); @Before public void setUp() throws Exception { @@ -447,6 +464,86 @@ public void getEffectiveInterceptors_disableBoth() { assertThat(effectiveInterceptors).containsExactly(DUMMY_USER_INTERCEPTOR); } + @Test + public void getEffectiveInterceptors_callsGetGlobalInterceptors() throws Exception { + Class runnable = classLoader.loadClass(StaticTestingClassLoaderCallsGet.class.getName()); + ((Runnable) runnable.getDeclaredConstructor().newInstance()).run(); + } + + // UsedReflectively + public static final class StaticTestingClassLoaderCallsGet implements Runnable { + + @Override + public void run() { + ManagedChannelImplBuilder builder = + new ManagedChannelImplBuilder( + DUMMY_TARGET, + new UnsupportedClientTransportFactoryBuilder(), + new FixedPortProvider(DUMMY_PORT)); + List effectiveInterceptors = builder.getEffectiveInterceptors(); + assertThat(effectiveInterceptors).hasSize(2); + try { + InternalGlobalInterceptors.setInterceptorsTracers( + Arrays.asList(DUMMY_USER_INTERCEPTOR), + Collections.emptyList(), + Collections.emptyList()); + fail("exception expected"); + } catch (IllegalStateException e) { + assertThat(e).hasMessageThat().contains("Set cannot be called after any get call"); + } + } + } + + @Test + public void getEffectiveInterceptors_callsSetGlobalInterceptors() throws Exception { + Class runnable = classLoader.loadClass(StaticTestingClassLoaderCallsSet.class.getName()); + ((Runnable) runnable.getDeclaredConstructor().newInstance()).run(); + } + + // UsedReflectively + public static final class StaticTestingClassLoaderCallsSet implements Runnable { + + @Override + public void run() { + InternalGlobalInterceptors.setInterceptorsTracers( + Arrays.asList(DUMMY_USER_INTERCEPTOR, DUMMY_USER_INTERCEPTOR1), + Collections.emptyList(), + Collections.emptyList()); + ManagedChannelImplBuilder builder = + new ManagedChannelImplBuilder( + DUMMY_TARGET, + new UnsupportedClientTransportFactoryBuilder(), + new FixedPortProvider(DUMMY_PORT)); + List effectiveInterceptors = builder.getEffectiveInterceptors(); + assertThat(effectiveInterceptors) + .containsExactly(DUMMY_USER_INTERCEPTOR, DUMMY_USER_INTERCEPTOR1); + } + } + + @Test + public void getEffectiveInterceptors_setEmptyGlobalInterceptors() throws Exception { + Class runnable = + classLoader.loadClass(StaticTestingClassLoaderCallsSetEmpty.class.getName()); + ((Runnable) runnable.getDeclaredConstructor().newInstance()).run(); + } + + // UsedReflectively + public static final class StaticTestingClassLoaderCallsSetEmpty implements Runnable { + + @Override + public void run() { + InternalGlobalInterceptors.setInterceptorsTracers( + Collections.emptyList(), Collections.emptyList(), Collections.emptyList()); + ManagedChannelImplBuilder builder = + new ManagedChannelImplBuilder( + DUMMY_TARGET, + new UnsupportedClientTransportFactoryBuilder(), + new FixedPortProvider(DUMMY_PORT)); + List effectiveInterceptors = builder.getEffectiveInterceptors(); + assertThat(effectiveInterceptors).isEmpty(); + } + } + @Test public void idleTimeout() { assertEquals(ManagedChannelImplBuilder.IDLE_MODE_DEFAULT_TIMEOUT_MILLIS, diff --git a/core/src/test/java/io/grpc/internal/ServerImplBuilderTest.java b/core/src/test/java/io/grpc/internal/ServerImplBuilderTest.java index ad8cf41598a..ce601c5f837 100644 --- a/core/src/test/java/io/grpc/internal/ServerImplBuilderTest.java +++ b/core/src/test/java/io/grpc/internal/ServerImplBuilderTest.java @@ -18,11 +18,20 @@ import static com.google.common.truth.Truth.assertThat; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.fail; +import io.grpc.InternalGlobalInterceptors; import io.grpc.Metadata; +import io.grpc.ServerCall; +import io.grpc.ServerCallHandler; +import io.grpc.ServerInterceptor; import io.grpc.ServerStreamTracer; +import io.grpc.StaticTestingClassLoader; import io.grpc.internal.ServerImplBuilder.ClientTransportServersBuilder; +import java.util.Arrays; +import java.util.Collections; import java.util.List; +import java.util.regex.Pattern; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -38,6 +47,21 @@ public ServerStreamTracer newServerStreamTracer(String fullMethodName, Metadata throw new UnsupportedOperationException(); } }; + private static final ServerInterceptor DUMMY_TEST_INTERCEPTOR = + new ServerInterceptor() { + + @Override + public ServerCall.Listener interceptCall( + ServerCall call, Metadata headers, ServerCallHandler next) { + throw new UnsupportedOperationException(); + } + }; + private final StaticTestingClassLoader classLoader = + new StaticTestingClassLoader( + getClass().getClassLoader(), + Pattern.compile( + "io\\.grpc\\.InternalGlobalInterceptors|io\\.grpc\\.GlobalInterceptors|" + + "io\\.grpc\\.internal\\.[^.]+")); private ServerImplBuilder builder; @@ -101,4 +125,77 @@ public void getTracerFactories_disableBoth() { List factories = builder.getTracerFactories(); assertThat(factories).containsExactly(DUMMY_USER_TRACER); } + + @Test + public void getTracerFactories_callsGet() throws Exception { + Class runnable = classLoader.loadClass(StaticTestingClassLoaderCallsGet.class.getName()); + ((Runnable) runnable.getDeclaredConstructor().newInstance()).run(); + } + + public static final class StaticTestingClassLoaderCallsGet implements Runnable { + @Override + public void run() { + ServerImplBuilder builder = + new ServerImplBuilder( + streamTracerFactories -> { + throw new UnsupportedOperationException(); + }); + assertThat(builder.getTracerFactories()).hasSize(2); + assertThat(builder.interceptors).hasSize(0); + try { + InternalGlobalInterceptors.setInterceptorsTracers( + Collections.emptyList(), Collections.emptyList(), Collections.emptyList()); + fail("exception expected"); + } catch (IllegalStateException e) { + assertThat(e).hasMessageThat().contains("Set cannot be called after any get call"); + } + } + } + + @Test + public void getTracerFactories_callsSet() throws Exception { + Class runnable = classLoader.loadClass(StaticTestingClassLoaderCallsSet.class.getName()); + ((Runnable) runnable.getDeclaredConstructor().newInstance()).run(); + } + + public static final class StaticTestingClassLoaderCallsSet implements Runnable { + @Override + public void run() { + InternalGlobalInterceptors.setInterceptorsTracers( + Collections.emptyList(), + Arrays.asList(DUMMY_TEST_INTERCEPTOR), + Arrays.asList(DUMMY_USER_TRACER)); + ServerImplBuilder builder = + new ServerImplBuilder( + streamTracerFactories -> { + throw new UnsupportedOperationException(); + }); + assertThat(builder.getTracerFactories()).containsExactly(DUMMY_USER_TRACER); + assertThat(builder.interceptors).containsExactly(DUMMY_TEST_INTERCEPTOR); + } + } + + @Test + public void getEffectiveInterceptors_setEmpty() throws Exception { + Class runnable = + classLoader.loadClass(StaticTestingClassLoaderCallsSetEmpty.class.getName()); + ((Runnable) runnable.getDeclaredConstructor().newInstance()).run(); + } + + // UsedReflectively + public static final class StaticTestingClassLoaderCallsSetEmpty implements Runnable { + + @Override + public void run() { + InternalGlobalInterceptors.setInterceptorsTracers( + Collections.emptyList(), Collections.emptyList(), Collections.emptyList()); + ServerImplBuilder builder = + new ServerImplBuilder( + streamTracerFactories -> { + throw new UnsupportedOperationException(); + }); + assertThat(builder.getTracerFactories()).isEmpty(); + assertThat(builder.interceptors).isEmpty(); + } + } } From 71dccfa8baf6be95b4720269ebc28e4ac5b0243c Mon Sep 17 00:00:00 2001 From: Sanjay Pujare Date: Wed, 29 Jun 2022 23:44:37 -0700 Subject: [PATCH 2/2] address review comments --- .../main/java/io/grpc/GlobalInterceptors.java | 30 +++++++------------ .../io/grpc/InternalGlobalInterceptors.java | 4 --- .../java/io/grpc/GlobalInterceptorsTest.java | 9 +++--- .../internal/ManagedChannelImplBuilder.java | 5 ++-- .../io/grpc/internal/ServerImplBuilder.java | 6 ++-- 5 files changed, 22 insertions(+), 32 deletions(-) diff --git a/api/src/main/java/io/grpc/GlobalInterceptors.java b/api/src/main/java/io/grpc/GlobalInterceptors.java index 2430bb79b3f..44d1592b61a 100644 --- a/api/src/main/java/io/grpc/GlobalInterceptors.java +++ b/api/src/main/java/io/grpc/GlobalInterceptors.java @@ -16,6 +16,8 @@ package io.grpc; +import static com.google.common.base.Preconditions.checkNotNull; + import java.util.ArrayList; import java.util.Collections; import java.util.List; @@ -23,20 +25,16 @@ /** The collection of global interceptors and global server stream tracers. */ @Internal final class GlobalInterceptors { - private static List clientInterceptors = Collections.emptyList(); - private static List serverInterceptors = Collections.emptyList(); + private static List clientInterceptors = null; + private static List serverInterceptors = null; private static List serverStreamTracerFactories = - Collections.emptyList(); + null; private static boolean isGlobalInterceptorsTracersSet; private static boolean isGlobalInterceptorsTracersGet; // Prevent instantiation private GlobalInterceptors() {} - static boolean isIsGlobalInterceptorsTracersSet() { - return isGlobalInterceptorsTracersSet; - } - /** * Sets the list of global interceptors and global server stream tracers. * @@ -65,19 +63,13 @@ static synchronized void setInterceptorsTracers( if (isGlobalInterceptorsTracersSet) { throw new IllegalStateException("Global interceptors and tracers are already set"); } - - if (clientInterceptorList != null) { - clientInterceptors = Collections.unmodifiableList(new ArrayList<>(clientInterceptorList)); - } - - if (serverInterceptorList != null) { - serverInterceptors = Collections.unmodifiableList(new ArrayList<>(serverInterceptorList)); - } - - if (serverStreamTracerFactoryList != null) { - serverStreamTracerFactories = + checkNotNull(clientInterceptorList); + checkNotNull(serverInterceptorList); + checkNotNull(serverStreamTracerFactoryList); + clientInterceptors = Collections.unmodifiableList(new ArrayList<>(clientInterceptorList)); + serverInterceptors = Collections.unmodifiableList(new ArrayList<>(serverInterceptorList)); + serverStreamTracerFactories = Collections.unmodifiableList(new ArrayList<>(serverStreamTracerFactoryList)); - } isGlobalInterceptorsTracersSet = true; } diff --git a/api/src/main/java/io/grpc/InternalGlobalInterceptors.java b/api/src/main/java/io/grpc/InternalGlobalInterceptors.java index dac37797782..db0ff6e2ce9 100644 --- a/api/src/main/java/io/grpc/InternalGlobalInterceptors.java +++ b/api/src/main/java/io/grpc/InternalGlobalInterceptors.java @@ -42,9 +42,5 @@ public static List getServerStreamTracerFactories() return GlobalInterceptors.getServerStreamTracerFactories(); } - public static boolean isGlobalInterceptorsTracersSet() { - return GlobalInterceptors.isIsGlobalInterceptorsTracersSet(); - } - private InternalGlobalInterceptors() {} } diff --git a/api/src/test/java/io/grpc/GlobalInterceptorsTest.java b/api/src/test/java/io/grpc/GlobalInterceptorsTest.java index bfa42b180da..7315186f1ee 100644 --- a/api/src/test/java/io/grpc/GlobalInterceptorsTest.java +++ b/api/src/test/java/io/grpc/GlobalInterceptorsTest.java @@ -21,6 +21,7 @@ import java.util.ArrayList; import java.util.Arrays; +import java.util.Collections; import java.util.List; import java.util.regex.Pattern; import org.junit.Test; @@ -98,7 +99,7 @@ public static final class StaticTestingClassLoaderSetTwice implements Runnable { public void run() { GlobalInterceptors.setInterceptorsTracers( new ArrayList<>(Arrays.asList(new NoopClientInterceptor())), - null, + Collections.emptyList(), new ArrayList<>(Arrays.asList(new NoopServerStreamTracerFactory()))); try { GlobalInterceptors.setInterceptorsTracers( @@ -115,7 +116,7 @@ public static final class StaticTestingClassLoaderGetBeforeSetClientInterceptor @Override public void run() { List clientInterceptors = GlobalInterceptors.getClientInterceptors(); - assertThat(clientInterceptors).isEmpty(); + assertThat(clientInterceptors).isNull(); try { GlobalInterceptors.setInterceptorsTracers( @@ -132,7 +133,7 @@ public static final class StaticTestingClassLoaderGetBeforeSetServerInterceptor @Override public void run() { List serverInterceptors = GlobalInterceptors.getServerInterceptors(); - assertThat(serverInterceptors).isEmpty(); + assertThat(serverInterceptors).isNull(); try { GlobalInterceptors.setInterceptorsTracers( @@ -150,7 +151,7 @@ public static final class StaticTestingClassLoaderGetBeforeSetServerStreamTracer public void run() { List serverStreamTracerFactories = GlobalInterceptors.getServerStreamTracerFactories(); - assertThat(serverStreamTracerFactories).isEmpty(); + assertThat(serverStreamTracerFactories).isNull(); try { GlobalInterceptors.setInterceptorsTracers( diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java index 6210f4f039d..53d5302e92d 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java @@ -638,11 +638,12 @@ public ManagedChannel build() { @VisibleForTesting List getEffectiveInterceptors() { List effectiveInterceptors = new ArrayList<>(this.interceptors); - boolean isGlobalInterceptorsSet = InternalGlobalInterceptors.isGlobalInterceptorsTracersSet(); + boolean isGlobalInterceptorsSet = false; List globalClientInterceptors = InternalGlobalInterceptors.getClientInterceptors(); - if (isGlobalInterceptorsSet) { + if (globalClientInterceptors != null) { effectiveInterceptors.addAll(globalClientInterceptors); + isGlobalInterceptorsSet = true; } if (!isGlobalInterceptorsSet && statsEnabled) { ClientInterceptor statsInterceptor = null; diff --git a/core/src/main/java/io/grpc/internal/ServerImplBuilder.java b/core/src/main/java/io/grpc/internal/ServerImplBuilder.java index dc3147bc2f1..cd18457d51b 100644 --- a/core/src/main/java/io/grpc/internal/ServerImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/ServerImplBuilder.java @@ -247,15 +247,15 @@ public Server build() { @VisibleForTesting List getTracerFactories() { ArrayList tracerFactories = new ArrayList<>(); - boolean isGlobalInterceptorsTracersSet - = InternalGlobalInterceptors.isGlobalInterceptorsTracersSet(); + boolean isGlobalInterceptorsTracersSet = false; List globalServerInterceptors = InternalGlobalInterceptors.getServerInterceptors(); List globalServerStreamTracerFactories = InternalGlobalInterceptors.getServerStreamTracerFactories(); - if (isGlobalInterceptorsTracersSet) { + if (globalServerInterceptors != null) { tracerFactories.addAll(globalServerStreamTracerFactories); interceptors.addAll(globalServerInterceptors); + isGlobalInterceptorsTracersSet = true; } if (!isGlobalInterceptorsTracersSet && statsEnabled) { ServerStreamTracer.Factory censusStatsTracerFactory = null;