api, core, services: make ProtoReflectionService interceptor compatible #6967

voidzcy · 2020-04-23T22:03:44Z

Resolves #3469.

Major changes:

Put ServerImpl instance to its Context so that all server calls have access to the Server instance from which they are dispatched.
InternalNotifyOnServerBuild is eliminated as the server reference can be captured with a server interceptor.
Enhancement: allow using the same ProtoReflectionService instance across multiple servers without recomputing the whole service index for each server.

api/src/main/java/io/grpc/ServerCall.java

services/src/main/java/io/grpc/protobuf/services/ProtoReflectionService.java

…per server.

…can be shared for multiple servers.

ejona86 · 2020-04-30T17:04:01Z

services/src/main/java/io/grpc/protobuf/services/ProtoReflectionService.java

@@ -77,8 +77,7 @@
  private ProtoReflectionService() {}

  /**
-   * Creates a instance of {@link ProtoReflectionService}. Intended usage is one instance per


Doh. I missed this before. Of course it was already limited to one server. Sorry for asking for the WeakHashMap, although it is nicer that way. I guess previously we'd have just returned wrong data? Yeah, using the WeakHashMap is much nicer for users.

services/src/main/java/io/grpc/protobuf/services/ProtoReflectionService.java

voidzcy · 2020-04-30T20:24:11Z

Per offline discussion, we decided to expose Server through Context (with an internal key). So no new API needs to be added.

ejona86 · 2020-04-30T21:07:07Z

core/src/main/java/io/grpc/internal/InternalServerAccessor.java

+ */
+@Internal
+public class InternalServerAccessor {
+  public static final Context.Key<Server> SERVER_KEY = ServerImpl.SERVER_CONTEXT_KEY;


Since this instance is being used as API and not as convenience, the instance needs to be created in io.grpc (not just copied, like here). If we shaded io.grpc.internal this reference would no longer match between implementations.

Moved the creation to io.grpc.Server and accessor to io.grpc.

ejona86 · 2020-04-30T21:07:58Z

core/src/main/java/io/grpc/internal/InternalServerAccessor.java

+/**
+ * Internal accessor for getting the {@link Server} instance inside server RPC {@link Context}.
+ * This is intended for usage internal to the gRPC team, as it's unclear to us what users would
+ * need. If you *really* think you need to use this, please file an issue for us to discuss a


Let's drop the "really". I don't see the need for the emphasis, and no real need to scare off users from filing an issue.

Reworded. Users should refer to the documentation at where the Context key is created (io.grpc.Server). The accessor is just for our own usage.

ejona86

After this one change, it will probably be finished.

ejona86 · 2020-04-30T21:59:43Z

api/src/main/java/io/grpc/Server.java

+   * unclear to us what users would need. If you think you need to use this, please file an
+   * issue for us to discuss a public API.
+   */
+  protected static final Context.Key<Server> SERVER_CONTEXT_KEY =


protected leaks it. Using package-private is probably better.

ejona86

Thank you!

Motivation: gRPC-Java [1.30.0](https://github.com/grpc/grpc-java/releases/tag/v1.30.0) has been released We need a workaround for `ProtoReflectionService`. Because it does not implement `InternalNotifyOnServerBuild` anymore. See grpc/grpc-java#6967 line#2806 It is a temporary workaround until upstream handles grpc/grpc-java#7138. Motivations: - Upgrade gRPC to 1.30.0 from 1.29.0 - Add ProtoReflectionServiceInterceptor that injects dummy server to gRPC context. Result: You can run gRPC 1.30.0 with Armeria server.

Motivation: gRPC-Java [1.30.0](https://github.com/grpc/grpc-java/releases/tag/v1.30.0) has been released We need a workaround for `ProtoReflectionService`. Because it does not implement `InternalNotifyOnServerBuild` anymore. See grpc/grpc-java#6967 line#2806 It is a temporary workaround until upstream handles grpc/grpc-java#7138. Motivations: - Upgrade gRPC to 1.30.0 from 1.29.0 - Add ProtoReflectionServiceInterceptor that injects dummy server to gRPC context. Result: You can run gRPC 1.30.0 with Armeria server. Fixes line#2806

Motivation: gRPC-Java [1.30.0](https://github.com/grpc/grpc-java/releases/tag/v1.30.0) has been released We need a workaround for `ProtoReflectionService`. Because it does not implement `InternalNotifyOnServerBuild` anymore. See grpc/grpc-java#6967 #2806 It is a temporary workaround until upstream handles grpc/grpc-java#7138. Modifications: - Upgrade gRPC to 1.30.0 from 1.29.0 - Add ProtoReflectionServiceInterceptor that injects dummy server to gRPC context. Result: You can run gRPC 1.30.0 with Armeria server. Fixes #2806

Motivation: gRPC-Java [1.30.0](https://github.com/grpc/grpc-java/releases/tag/v1.30.0) has been released We need a workaround for `ProtoReflectionService`. Because it does not implement `InternalNotifyOnServerBuild` anymore. See grpc/grpc-java#6967 line#2806 It is a temporary workaround until upstream handles grpc/grpc-java#7138. Modifications: - Upgrade gRPC to 1.30.0 from 1.29.0 - Add ProtoReflectionServiceInterceptor that injects dummy server to gRPC context. Result: You can run gRPC 1.30.0 with Armeria server. Fixes line#2806

…le (grpc#6967) Eliminate the hack of InternalNotifyOnBuild mechanism for letting ProtoReflectionService get access to the Sever instance, which makes ProtoReflectionService incompatible with server interceptors. This change put the Server instance into the Context and let the ProtoReflectionService RPC obtain it in its RPC Context. Also enhanced ProtoReflectionService so that one service instance can be used across multiple servers.

voidzcy added 5 commits April 23, 2020 14:58

Eliminate InternalNotifyOnServerBuilder interface.

4f7462d

Expose server reference on ServerCall interface.

7d8547a

Add getServer() API implementation to subclasses.

1952927

Obtain server via a server interceptor.

5b51f87

Eliminate callback for server build.

9273cdd

voidzcy marked this pull request as ready for review April 24, 2020 18:44

voidzcy requested review from ejona86 and ericgribkoff April 24, 2020 18:49

ejona86 reviewed Apr 28, 2020

View reviewed changes

api/src/main/java/io/grpc/ServerCall.java Outdated Show resolved Hide resolved

api/src/main/java/io/grpc/ServerCall.java Outdated Show resolved Hide resolved

services/src/main/java/io/grpc/protobuf/services/ProtoReflectionService.java Outdated Show resolved Hide resolved

voidzcy added 4 commits April 29, 2020 14:29

Add default implementation for ServerCall.getServer().

d268012

Add ExperimentalApi annotation.

933d099

Remove comment requirement for using one reflection service instance …

130adde

…per server.

Add a simple test case covering a single reflection service instance …

f50789b

…can be shared for multiple servers.

ejona86 reviewed Apr 30, 2020

View reviewed changes

voidzcy added 5 commits April 30, 2020 10:25

Use explicit lock

acb2a44

Put server into Context.

19fd730

Add an internal accessor for accessing the Context key.

b0d15a6

Change ProtoReflectionService to obtain Server from Context.

d3935fa

Revert changes for exposing Server via ServerCall.

e4a6b9b

voidzcy changed the title ~~api, core, services: make ProtoReflectionService interceptor compatible~~ core, services: make ProtoReflectionService interceptor compatible Apr 30, 2020

voidzcy requested a review from ejona86 April 30, 2020 20:44

ejona86 reviewed Apr 30, 2020

View reviewed changes

Move the Context key definition and accessor to io.grpc.

9a979c5

ejona86 reviewed Apr 30, 2020

View reviewed changes

Makee context key package private.

6277cac

ejona86 approved these changes Apr 30, 2020

View reviewed changes

voidzcy changed the title ~~core, services: make ProtoReflectionService interceptor compatible~~ api, core, services: make ProtoReflectionService interceptor compatible May 1, 2020

voidzcy merged commit a423900 into grpc:master May 1, 2020

okue mentioned this pull request Jun 18, 2020

Cannot build with grpc-java:1.30.0 due to ProtoReflectionService change line/armeria#2806

Closed

ikhoon mentioned this pull request Jun 18, 2020

Consider configuring ProtoReflectionService to use it without internal APIs #7138

Open

ikhoon mentioned this pull request Jun 18, 2020

Update gRPC-Java to 1.30.0 line/armeria#2814

Merged

github-actions bot locked as resolved and limited conversation to collaborators Jun 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

api, core, services: make ProtoReflectionService interceptor compatible #6967

api, core, services: make ProtoReflectionService interceptor compatible #6967

voidzcy commented Apr 23, 2020 •

edited

ejona86 Apr 30, 2020

voidzcy commented Apr 30, 2020

ejona86 Apr 30, 2020

voidzcy Apr 30, 2020

ejona86 Apr 30, 2020

voidzcy Apr 30, 2020

ejona86 left a comment

ejona86 Apr 30, 2020

ejona86 left a comment

api, core, services: make ProtoReflectionService interceptor compatible #6967

api, core, services: make ProtoReflectionService interceptor compatible #6967

Conversation

voidzcy commented Apr 23, 2020 • edited

ejona86 Apr 30, 2020

Choose a reason for hiding this comment

voidzcy commented Apr 30, 2020

ejona86 Apr 30, 2020

Choose a reason for hiding this comment

voidzcy Apr 30, 2020

Choose a reason for hiding this comment

ejona86 Apr 30, 2020

Choose a reason for hiding this comment

voidzcy Apr 30, 2020

Choose a reason for hiding this comment

ejona86 left a comment

Choose a reason for hiding this comment

ejona86 Apr 30, 2020

Choose a reason for hiding this comment

ejona86 left a comment

Choose a reason for hiding this comment

voidzcy commented Apr 23, 2020 •

edited