From 95973827f556685a511cdff411f7c53590deb2cf Mon Sep 17 00:00:00 2001 From: Kun Zhang Date: Tue, 3 May 2016 14:12:05 -0700 Subject: [PATCH] Refactor HandlerRegistry. See #933 - Create InternalHandlerRegistry, an immutable look-up table. Handlers passed to ServerBuilder.addService() go to this registry. This covers the most common use cases. By keeping the registry internal we could freely change the registry's interface to accommodate optimizations, e.g., for hpack. - The internal registry uses a flat fullMethodName -> handler look-up table instead of a hierarchical one used before. It faster because it saves one look-up and a substring. - Introduces the fallback registry, settable by ServerBuilder.fallbackHandlerRegistry(), for advanced users who want a dynamic registry. Moved the current MutableHandlerRegistryImpl to io.grpc.util.MutableHandlerRegistry as a stock implementation of the fallback registry. The io.grpc.MutableHandlerRegistry interface is now removed. --- .../netty/HandlerRegistryBenchmark.java | 10 +-- core/src/main/java/io/grpc/ServerBuilder.java | 15 ++-- .../java/io/grpc/ServerServiceDefinition.java | 3 +- .../inprocess/InProcessServerBuilder.java | 17 ---- .../internal/AbstractServerImplBuilder.java | 65 ++++++-------- .../InternalHandlerRegistry.java} | 59 ++++++++----- .../java/io/grpc/internal/ServerImpl.java | 17 ++-- .../MutableHandlerRegistry.java} | 12 ++- .../java/io/grpc/internal/ServerImplTest.java | 85 +++++++++++++------ .../MutableHandlerRegistryTest.java} | 12 ++- .../io/grpc/netty/NettyServerBuilder.java | 18 ---- 11 files changed, 167 insertions(+), 146 deletions(-) rename core/src/main/java/io/grpc/{MutableHandlerRegistry.java => internal/InternalHandlerRegistry.java} (52%) rename core/src/main/java/io/grpc/{MutableHandlerRegistryImpl.java => util/MutableHandlerRegistry.java} (91%) rename core/src/test/java/io/grpc/{MutableHandlerRegistryImplTest.java => util/MutableHandlerRegistryTest.java} (96%) diff --git a/benchmarks/src/jmh/java/io/grpc/benchmarks/netty/HandlerRegistryBenchmark.java b/benchmarks/src/jmh/java/io/grpc/benchmarks/netty/HandlerRegistryBenchmark.java index 68351c0d12..f16d19c595 100644 --- a/benchmarks/src/jmh/java/io/grpc/benchmarks/netty/HandlerRegistryBenchmark.java +++ b/benchmarks/src/jmh/java/io/grpc/benchmarks/netty/HandlerRegistryBenchmark.java @@ -32,9 +32,9 @@ package io.grpc.benchmarks.netty; import io.grpc.MethodDescriptor; -import io.grpc.MutableHandlerRegistryImpl; import io.grpc.ServerMethodDefinition; import io.grpc.ServerServiceDefinition; +import io.grpc.util.MutableHandlerRegistry; import org.openjdk.jmh.annotations.Benchmark; import org.openjdk.jmh.annotations.Fork; @@ -50,7 +50,7 @@ import java.util.List; import java.util.Random; /** - * Benchmark for {@link MutableHandlerRegistryImpl}. + * Benchmark for {@link MutableHandlerRegistry}. */ @State(Scope.Benchmark) @Fork(1) @@ -68,7 +68,7 @@ public class HandlerRegistryBenchmark { @Param({"100"}) public int methodCountPerService; - private MutableHandlerRegistryImpl registry; + private MutableHandlerRegistry registry; private List fullMethodNames; /** @@ -76,7 +76,7 @@ public class HandlerRegistryBenchmark { */ @Setup(Level.Trial) public void setup() throws Exception { - registry = new MutableHandlerRegistryImpl(); + registry = new MutableHandlerRegistry(); fullMethodNames = new ArrayList(serviceCount * methodCountPerService); for (int serviceIndex = 0; serviceIndex < serviceCount; ++serviceIndex) { String serviceName = randomString(); @@ -94,7 +94,7 @@ public class HandlerRegistryBenchmark { } /** - * Benchmark the {@link MutableHandlerRegistryImpl#lookupMethod(String)} throughput. + * Benchmark the {@link MutableHandlerRegistry#lookupMethod(String)} throughput. */ @Benchmark public void lookupMethod(Blackhole bh) { diff --git a/core/src/main/java/io/grpc/ServerBuilder.java b/core/src/main/java/io/grpc/ServerBuilder.java index 81b171f7ec..e514f95351 100644 --- a/core/src/main/java/io/grpc/ServerBuilder.java +++ b/core/src/main/java/io/grpc/ServerBuilder.java @@ -75,8 +75,6 @@ public abstract class ServerBuilder> { * Adds a service implementation to the handler registry. * * @param service ServerServiceDefinition object - * @throws UnsupportedOperationException if this builder does not support dynamically adding - * services. */ public abstract T addService(ServerServiceDefinition service); @@ -84,12 +82,17 @@ public abstract class ServerBuilder> { * Adds a service implementation to the handler registry. * * @param bindableService BindableService object - * @throws UnsupportedOperationException if this builder does not support dynamically adding - * services. */ @ExperimentalApi("https://github.com/grpc/grpc-java/issues/1701") public abstract T addService(BindableService bindableService); + /** + * Sets a fallback handler registry that will be looked up in if a method is not found in the + * primary registry. + */ + @ExperimentalApi("https://github.com/grpc/grpc-java/issues/933") + public abstract T fallbackHandlerRegistry(@Nullable HandlerRegistry fallbackRegistry); + /** * Makes the server use TLS. * @@ -104,7 +107,7 @@ public abstract class ServerBuilder> { * decompressors are in {@code DecompressorRegistry.getDefaultInstance}. */ @ExperimentalApi("https://github.com/grpc/grpc-java/issues/1704") - public abstract T decompressorRegistry(DecompressorRegistry registry); + public abstract T decompressorRegistry(@Nullable DecompressorRegistry registry); /** * Set the compression registry for use in the channel. This is an advanced API call and @@ -112,7 +115,7 @@ public abstract class ServerBuilder> { * compressors are in {@code CompressorRegistry.getDefaultInstance}. */ @ExperimentalApi("https://github.com/grpc/grpc-java/issues/1704") - public abstract T compressorRegistry(CompressorRegistry registry); + public abstract T compressorRegistry(@Nullable CompressorRegistry registry); /** * Builds a server using the given parameters. diff --git a/core/src/main/java/io/grpc/ServerServiceDefinition.java b/core/src/main/java/io/grpc/ServerServiceDefinition.java index 6cf656a062..def8329f2f 100644 --- a/core/src/main/java/io/grpc/ServerServiceDefinition.java +++ b/core/src/main/java/io/grpc/ServerServiceDefinition.java @@ -75,7 +75,8 @@ public final class ServerServiceDefinition { * * @param name the fully qualified name without leading slash. E.g., "com.foo.Foo/Bar" */ - ServerMethodDefinition getMethod(String name) { + @Internal + public ServerMethodDefinition getMethod(String name) { return methods.get(name); } diff --git a/core/src/main/java/io/grpc/inprocess/InProcessServerBuilder.java b/core/src/main/java/io/grpc/inprocess/InProcessServerBuilder.java index 5c42643e6e..58cad724fe 100644 --- a/core/src/main/java/io/grpc/inprocess/InProcessServerBuilder.java +++ b/core/src/main/java/io/grpc/inprocess/InProcessServerBuilder.java @@ -34,7 +34,6 @@ package io.grpc.inprocess; import com.google.common.base.Preconditions; import io.grpc.ExperimentalApi; -import io.grpc.HandlerRegistry; import io.grpc.internal.AbstractServerImplBuilder; import java.io.File; @@ -48,17 +47,6 @@ import java.io.File; @ExperimentalApi("https://github.com/grpc/grpc-java/issues/1783") public final class InProcessServerBuilder extends AbstractServerImplBuilder { - /** - * Create a server builder that will bind with the given name. - * - * @param name the identity of the server for clients to connect to - * @param registry the registry of handlers used for dispatching incoming calls - * @return a new builder - */ - public static InProcessServerBuilder forName(String name, HandlerRegistry registry) { - return new InProcessServerBuilder(name, registry); - } - /** * Create a server builder that will bind with the given name. * @@ -71,11 +59,6 @@ public final class InProcessServerBuilder private final String name; - private InProcessServerBuilder(String name, HandlerRegistry registry) { - super(registry); - this.name = Preconditions.checkNotNull(name, "name"); - } - private InProcessServerBuilder(String name) { this.name = Preconditions.checkNotNull(name, "name"); } diff --git a/core/src/main/java/io/grpc/internal/AbstractServerImplBuilder.java b/core/src/main/java/io/grpc/internal/AbstractServerImplBuilder.java index 0f528abd37..b82a917949 100644 --- a/core/src/main/java/io/grpc/internal/AbstractServerImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/AbstractServerImplBuilder.java @@ -33,7 +33,6 @@ package io.grpc.internal; import static com.google.common.base.MoreObjects.firstNonNull; -import com.google.common.base.Preconditions; import com.google.common.util.concurrent.MoreExecutors; import io.grpc.BindableService; @@ -42,9 +41,8 @@ import io.grpc.Context; import io.grpc.DecompressorRegistry; import io.grpc.HandlerRegistry; import io.grpc.Internal; -import io.grpc.MutableHandlerRegistry; -import io.grpc.MutableHandlerRegistryImpl; import io.grpc.ServerBuilder; +import io.grpc.ServerMethodDefinition; import io.grpc.ServerServiceDefinition; import java.util.concurrent.Executor; @@ -59,7 +57,18 @@ import javax.annotation.Nullable; public abstract class AbstractServerImplBuilder> extends ServerBuilder { - private final HandlerRegistry registry; + private static final HandlerRegistry EMPTY_FALLBACK_REGISTRY = new HandlerRegistry() { + @Override public ServerMethodDefinition lookupMethod(String method, String authority) { + return null; + } + }; + + private final InternalHandlerRegistry.Builder registryBuilder = + new InternalHandlerRegistry.Builder(); + + @Nullable + private HandlerRegistry fallbackRegistry; + @Nullable private Executor executor; @@ -69,20 +78,6 @@ public abstract class AbstractServerImplBuilderThis is supported only if the user didn't provide a handler registry, or the provided one is - * a {@link MutableHandlerRegistry}. Otherwise it throws an UnsupportedOperationException. - * - * @param service ServerServiceDefinition object. - */ @Override public final T addService(ServerServiceDefinition service) { - if (registry instanceof MutableHandlerRegistry) { - ((MutableHandlerRegistry) registry).addService(service); - return thisT(); - } - throw new UnsupportedOperationException("Underlying HandlerRegistry is not mutable"); + registryBuilder.addService(service); + return thisT(); } - /** - * Adds a service implementation to the handler registry. - * - *

This is supported only if the user didn't provide a handler registry, or the provided one is - * a {@link MutableHandlerRegistry}. Otherwise it throws an UnsupportedOperationException. - * - * @param bindableService BindableService object. - */ @Override public final T addService(BindableService bindableService) { return addService(bindableService.bindService()); } + @Override + public final T fallbackHandlerRegistry(HandlerRegistry registry) { + this.fallbackRegistry = registry; + return thisT(); + } + @Override public final T decompressorRegistry(DecompressorRegistry registry) { decompressorRegistry = registry; @@ -139,8 +121,9 @@ public abstract class AbstractServerImplBuilder> methods; + + private InternalHandlerRegistry(ImmutableMap> methods) { + this.methods = methods; + } + @Nullable - public abstract ServerServiceDefinition addService(ServerServiceDefinition service); + ServerMethodDefinition lookupMethod(String methodName) { + return methods.get(methodName); + } - /** - * Returns {@code false} if {@code service} was not registered. - */ - public abstract boolean removeService(ServerServiceDefinition service); + static class Builder { + // Store per-service first, to make sure services are added/replaced atomically. + private final HashMap services = + new HashMap(); + + Builder addService(ServerServiceDefinition service) { + services.put(service.getName(), service); + return this; + } + + InternalHandlerRegistry build() { + ImmutableMap.Builder> mapBuilder = + ImmutableMap.builder(); + for (ServerServiceDefinition service : services.values()) { + for (ServerMethodDefinition method : service.getMethods()) { + mapBuilder.put(method.getMethodDescriptor().getFullMethodName(), method); + } + } + return new InternalHandlerRegistry(mapBuilder.build()); + } + } } diff --git a/core/src/main/java/io/grpc/internal/ServerImpl.java b/core/src/main/java/io/grpc/internal/ServerImpl.java index afa7273864..79eb5b060a 100644 --- a/core/src/main/java/io/grpc/internal/ServerImpl.java +++ b/core/src/main/java/io/grpc/internal/ServerImpl.java @@ -80,7 +80,8 @@ public final class ServerImpl extends io.grpc.Server { /** Executor for application processing. */ private Executor executor; private boolean usingSharedExecutor; - private final HandlerRegistry registry; + private final InternalHandlerRegistry registry; + private final HandlerRegistry fallbackRegistry; private boolean started; private boolean shutdown; private boolean terminated; @@ -100,14 +101,17 @@ public final class ServerImpl extends io.grpc.Server { /** * Construct a server. * + * @param registry the primary method registry + * @param fallbackRegistry the secondary method registry, used only if the primary registry + * doesn't have the method * @param executor to call methods on behalf of remote clients - * @param registry of methods to expose to remote clients. */ - ServerImpl(Executor executor, HandlerRegistry registry, InternalServer transportServer, - Context rootContext, DecompressorRegistry decompressorRegistry, - CompressorRegistry compressorRegistry) { + ServerImpl(Executor executor, InternalHandlerRegistry registry, HandlerRegistry fallbackRegistry, + InternalServer transportServer, Context rootContext, + DecompressorRegistry decompressorRegistry, CompressorRegistry compressorRegistry) { this.executor = executor; this.registry = Preconditions.checkNotNull(registry, "registry"); + this.fallbackRegistry = Preconditions.checkNotNull(fallbackRegistry, "fallbackRegistry"); this.transportServer = Preconditions.checkNotNull(transportServer, "transportServer"); // Fork from the passed in context so that it does not propagate cancellation, it only // inherits values. @@ -312,6 +316,9 @@ public final class ServerImpl extends io.grpc.Server { ServerStreamListener listener = NOOP_LISTENER; try { ServerMethodDefinition method = registry.lookupMethod(methodName); + if (method == null) { + method = fallbackRegistry.lookupMethod(methodName); + } if (method == null) { stream.close( Status.UNIMPLEMENTED.withDescription("Method not found: " + methodName), diff --git a/core/src/main/java/io/grpc/MutableHandlerRegistryImpl.java b/core/src/main/java/io/grpc/util/MutableHandlerRegistry.java similarity index 91% rename from core/src/main/java/io/grpc/MutableHandlerRegistryImpl.java rename to core/src/main/java/io/grpc/util/MutableHandlerRegistry.java index f578e99a28..9a57f98117 100644 --- a/core/src/main/java/io/grpc/MutableHandlerRegistryImpl.java +++ b/core/src/main/java/io/grpc/util/MutableHandlerRegistry.java @@ -29,7 +29,13 @@ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. */ -package io.grpc; +package io.grpc.util; + +import io.grpc.ExperimentalApi; +import io.grpc.HandlerRegistry; +import io.grpc.MethodDescriptor; +import io.grpc.ServerMethodDefinition; +import io.grpc.ServerServiceDefinition; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; @@ -45,17 +51,15 @@ import javax.annotation.concurrent.ThreadSafe; */ @ThreadSafe @ExperimentalApi("https://github.com/grpc/grpc-java/issues/933") -public final class MutableHandlerRegistryImpl extends MutableHandlerRegistry { +public final class MutableHandlerRegistry extends HandlerRegistry { private final ConcurrentMap services = new ConcurrentHashMap(); - @Override @Nullable public ServerServiceDefinition addService(ServerServiceDefinition service) { return services.put(service.getName(), service); } - @Override public boolean removeService(ServerServiceDefinition service) { return services.remove(service.getName(), service); } diff --git a/core/src/test/java/io/grpc/internal/ServerImplTest.java b/core/src/test/java/io/grpc/internal/ServerImplTest.java index cd20f9ced2..51b238286d 100644 --- a/core/src/test/java/io/grpc/internal/ServerImplTest.java +++ b/core/src/test/java/io/grpc/internal/ServerImplTest.java @@ -42,6 +42,7 @@ import static org.mockito.Matchers.isA; import static org.mockito.Matchers.isNotNull; import static org.mockito.Matchers.notNull; import static org.mockito.Matchers.same; +import static org.mockito.Mockito.mock; import static org.mockito.Mockito.timeout; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; @@ -54,17 +55,17 @@ import io.grpc.Compressor; import io.grpc.CompressorRegistry; import io.grpc.Context; import io.grpc.DecompressorRegistry; +import io.grpc.HandlerRegistry; import io.grpc.IntegerMarshaller; import io.grpc.Metadata; import io.grpc.MethodDescriptor; import io.grpc.MethodDescriptor.MethodType; -import io.grpc.MutableHandlerRegistry; -import io.grpc.MutableHandlerRegistryImpl; import io.grpc.ServerCall; import io.grpc.ServerCallHandler; import io.grpc.ServerServiceDefinition; import io.grpc.Status; import io.grpc.StringMarshaller; +import io.grpc.util.MutableHandlerRegistry; import org.junit.After; import org.junit.Before; @@ -75,6 +76,7 @@ import org.junit.rules.ExpectedException; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; import org.mockito.ArgumentCaptor; +import org.mockito.Matchers; import org.mockito.Mock; import org.mockito.MockitoAnnotations; @@ -112,10 +114,11 @@ public class ServerImplTest { } private ExecutorService executor = Executors.newSingleThreadExecutor(); - private MutableHandlerRegistry registry = new MutableHandlerRegistryImpl(); + private InternalHandlerRegistry registry = new InternalHandlerRegistry.Builder().build(); + private MutableHandlerRegistry fallbackRegistry = new MutableHandlerRegistry(); private SimpleServer transportServer = new SimpleServer(); - private ServerImpl server = new ServerImpl(executor, registry, transportServer, SERVER_CONTEXT, - decompressorRegistry, compressorRegistry); + private ServerImpl server = new ServerImpl(executor, registry, fallbackRegistry, transportServer, + SERVER_CONTEXT, decompressorRegistry, compressorRegistry); @Mock private ServerStream stream; @@ -123,6 +126,9 @@ public class ServerImplTest { @Mock private ServerCall.Listener callListener; + @Mock + private ServerCallHandler callHandler; + /** Set up for test. */ @Before public void startUp() throws IOException { @@ -143,8 +149,8 @@ public class ServerImplTest { @Override public void shutdown() {} }; - ServerImpl server = new ServerImpl(executor, registry, transportServer, SERVER_CONTEXT, - decompressorRegistry, compressorRegistry); + ServerImpl server = new ServerImpl(executor, registry, fallbackRegistry, transportServer, + SERVER_CONTEXT, decompressorRegistry, compressorRegistry); server.start(); server.shutdown(); assertTrue(server.isShutdown()); @@ -161,8 +167,8 @@ public class ServerImplTest { throw new AssertionError("Should not be called, because wasn't started"); } }; - ServerImpl server = new ServerImpl(executor, registry, transportServer, SERVER_CONTEXT, - decompressorRegistry, compressorRegistry); + ServerImpl server = new ServerImpl(executor, registry, fallbackRegistry, transportServer, + SERVER_CONTEXT, decompressorRegistry, compressorRegistry); server.shutdown(); assertTrue(server.isShutdown()); assertTrue(server.isTerminated()); @@ -170,8 +176,8 @@ public class ServerImplTest { @Test public void startStopImmediateWithChildTransport() throws IOException { - ServerImpl server = new ServerImpl(executor, registry, transportServer, SERVER_CONTEXT, - decompressorRegistry, compressorRegistry); + ServerImpl server = new ServerImpl(executor, registry, fallbackRegistry, transportServer, + SERVER_CONTEXT, decompressorRegistry, compressorRegistry); server.start(); class DelayedShutdownServerTransport extends SimpleServerTransport { boolean shutdown; @@ -202,8 +208,8 @@ public class ServerImplTest { } } - ServerImpl server = new ServerImpl(executor, registry, new FailingStartupServer(), - SERVER_CONTEXT, decompressorRegistry, compressorRegistry); + ServerImpl server = new ServerImpl(executor, registry, fallbackRegistry, + new FailingStartupServer(), SERVER_CONTEXT, decompressorRegistry, compressorRegistry); try { server.start(); fail("expected exception"); @@ -218,7 +224,7 @@ public class ServerImplTest { = Metadata.Key.of("inception", Metadata.ASCII_STRING_MARSHALLER); final AtomicReference> callReference = new AtomicReference>(); - registry.addService(ServerServiceDefinition.builder("Waiter") + fallbackRegistry.addService(ServerServiceDefinition.builder("Waiter") .addMethod( MethodDescriptor.create( MethodType.UNKNOWN, "Waiter/serve", STRING_MARSHALLER, INTEGER_MARSHALLER), @@ -292,7 +298,7 @@ public class ServerImplTest { public void exceptionInStartCallPropagatesToStream() throws Exception { CyclicBarrier barrier = executeBarrier(executor); final Status status = Status.ABORTED.withDescription("Oh, no!"); - registry.addService(ServerServiceDefinition.builder("Waiter") + fallbackRegistry.addService(ServerServiceDefinition.builder("Waiter") .addMethod( MethodDescriptor.create(MethodType.UNKNOWN, "Waiter/serve", STRING_MARSHALLER, INTEGER_MARSHALLER), @@ -340,8 +346,8 @@ public class ServerImplTest { } transportServer = new MaybeDeadlockingServer(); - ServerImpl server = new ServerImpl(executor, registry, transportServer, SERVER_CONTEXT, - decompressorRegistry, compressorRegistry); + ServerImpl server = new ServerImpl(executor, registry, fallbackRegistry, transportServer, + SERVER_CONTEXT, decompressorRegistry, compressorRegistry); server.start(); new Thread() { @Override @@ -402,7 +408,7 @@ public class ServerImplTest { @Test public void testCallContextIsBoundInListenerCallbacks() throws Exception { - registry.addService(ServerServiceDefinition.builder("Waiter") + fallbackRegistry.addService(ServerServiceDefinition.builder("Waiter") .addMethod( MethodDescriptor.create( MethodType.UNKNOWN, "Waiter/serve", STRING_MARSHALLER, INTEGER_MARSHALLER), @@ -489,7 +495,7 @@ public class ServerImplTest { final AtomicReference> callReference = new AtomicReference>(); - registry.addService(ServerServiceDefinition.builder("Waiter") + fallbackRegistry.addService(ServerServiceDefinition.builder("Waiter") .addMethod( MethodDescriptor.create( MethodType.UNKNOWN, "Waiter/serve", STRING_MARSHALLER, INTEGER_MARSHALLER), @@ -524,8 +530,8 @@ public class ServerImplTest { return 65535; } }; - ServerImpl server = new ServerImpl(executor, registry, transportServer, SERVER_CONTEXT, - decompressorRegistry, compressorRegistry); + ServerImpl server = new ServerImpl(executor, registry, fallbackRegistry, transportServer, + SERVER_CONTEXT, decompressorRegistry, compressorRegistry); server.start(); Truth.assertThat(server.getPort()).isEqualTo(65535); @@ -534,8 +540,8 @@ public class ServerImplTest { @Test public void getPortBeforeStartedFails() { transportServer = new SimpleServer(); - ServerImpl server = new ServerImpl(executor, registry, transportServer, SERVER_CONTEXT, - decompressorRegistry, compressorRegistry); + ServerImpl server = new ServerImpl(executor, registry, fallbackRegistry, transportServer, + SERVER_CONTEXT, decompressorRegistry, compressorRegistry); thrown.expect(IllegalStateException.class); thrown.expectMessage("started"); server.getPort(); @@ -544,8 +550,8 @@ public class ServerImplTest { @Test public void getPortAfterTerminationFails() throws Exception { transportServer = new SimpleServer(); - ServerImpl server = new ServerImpl(executor, registry, transportServer, SERVER_CONTEXT, - decompressorRegistry, compressorRegistry); + ServerImpl server = new ServerImpl(executor, registry, fallbackRegistry, transportServer, + SERVER_CONTEXT, decompressorRegistry, compressorRegistry); server.start(); server.shutdown(); server.awaitTermination(); @@ -554,6 +560,35 @@ public class ServerImplTest { server.getPort(); } + @Test + public void handlerRegistryPriorities() throws Exception { + HandlerRegistry fallbackRegistry = mock(HandlerRegistry.class); + MethodDescriptor method1 = MethodDescriptor.create( + MethodType.UNKNOWN, "Service1/Method1", STRING_MARSHALLER, INTEGER_MARSHALLER); + registry = new InternalHandlerRegistry.Builder() + .addService(ServerServiceDefinition.builder("Service1") + .addMethod(method1, callHandler).build()) + .build(); + transportServer = new SimpleServer(); + ServerImpl server = new ServerImpl(executor, registry, fallbackRegistry, transportServer, + SERVER_CONTEXT, decompressorRegistry, compressorRegistry); + server.start(); + + ServerTransportListener transportListener + = transportServer.registerNewServerTransport(new SimpleServerTransport()); + // This call will be handled by callHandler from the internal registry + transportListener.streamCreated(stream, "Service1/Method1", new Metadata()); + // This call will be handled by the fallbackRegistry because it's not registred in the internal + // registry. + transportListener.streamCreated(stream, "Service1/Method2", new Metadata()); + + verify(callHandler, timeout(2000)).startCall(same(method1), + Matchers.>anyObject(), Matchers.anyObject()); + verify(fallbackRegistry, timeout(2000)).lookupMethod("Service1/Method2", null); + verifyNoMoreInteractions(callHandler); + verifyNoMoreInteractions(fallbackRegistry); + } + /** * Useful for plugging a single-threaded executor from processing tasks, or for waiting until a * single-threaded executor has processed queued tasks. diff --git a/core/src/test/java/io/grpc/MutableHandlerRegistryImplTest.java b/core/src/test/java/io/grpc/util/MutableHandlerRegistryTest.java similarity index 96% rename from core/src/test/java/io/grpc/MutableHandlerRegistryImplTest.java rename to core/src/test/java/io/grpc/util/MutableHandlerRegistryTest.java index 8eb2964164..d3e9b71a34 100644 --- a/core/src/test/java/io/grpc/MutableHandlerRegistryImplTest.java +++ b/core/src/test/java/io/grpc/util/MutableHandlerRegistryTest.java @@ -29,7 +29,7 @@ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. */ -package io.grpc; +package io.grpc.util; import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.collect.Iterables.getOnlyElement; @@ -42,6 +42,10 @@ import static org.mockito.Mockito.mock; import io.grpc.MethodDescriptor.Marshaller; import io.grpc.MethodDescriptor.MethodType; +import io.grpc.MethodDescriptor; +import io.grpc.ServerCallHandler; +import io.grpc.ServerMethodDefinition; +import io.grpc.ServerServiceDefinition; import org.junit.After; import org.junit.Test; @@ -49,10 +53,10 @@ import org.junit.runner.RunWith; import org.junit.runners.JUnit4; import org.mockito.Mockito; -/** Unit tests for {@link MutableHandlerRegistryImpl}. */ +/** Unit tests for {@link MutableHandlerRegistry}. */ @RunWith(JUnit4.class) -public class MutableHandlerRegistryImplTest { - private MutableHandlerRegistry registry = new MutableHandlerRegistryImpl(); +public class MutableHandlerRegistryTest { + private MutableHandlerRegistry registry = new MutableHandlerRegistry(); @SuppressWarnings("unchecked") private Marshaller requestMarshaller = mock(Marshaller.class); @SuppressWarnings("unchecked") diff --git a/netty/src/main/java/io/grpc/netty/NettyServerBuilder.java b/netty/src/main/java/io/grpc/netty/NettyServerBuilder.java index 7ac1b845f2..b4f25d7440 100644 --- a/netty/src/main/java/io/grpc/netty/NettyServerBuilder.java +++ b/netty/src/main/java/io/grpc/netty/NettyServerBuilder.java @@ -37,7 +37,6 @@ import static io.grpc.internal.GrpcUtil.DEFAULT_MAX_MESSAGE_SIZE; import com.google.common.base.Preconditions; import io.grpc.ExperimentalApi; -import io.grpc.HandlerRegistry; import io.grpc.Internal; import io.grpc.internal.AbstractServerImplBuilder; import io.grpc.internal.GrpcUtil; @@ -83,18 +82,6 @@ public final class NettyServerBuilder extends AbstractServerImplBuilder