grpc / grpc-java

The Java gRPC implementation. HTTP/2 based RPC
https://grpc.io/docs/languages/java/
Apache License 2.0
11.44k stars 3.85k forks source link

Consider configuring `ProtoReflectionService` to use it without internal APIs #7138

Open ikhoon opened 4 years ago

ikhoon commented 4 years ago

Due to the popularity of gRPC, some frameworks or libraries have an integration layer for gRPC.

Especially, Armeria has its own server/client implementations to serve/call a gRPC stub. ProtoReflectionService is using the internal reference of a gRPC-Java server instance since 1.30.0 #6967. That means a stub running without gRPC-Java server could not use ProtoReflectionService. https://github.com/line/armeria/issues/2806

Armeria injects ServerServiceDefinitions to ProtoReflectionService using notifyOnBuild() hook. https://github.com/line/armeria/blob/armeria-0.99.6/grpc/src/main/java/com/linecorp/armeria/server/grpc/FramedGrpcService.java#L271-L275 If ProtoReflectionService supports better ways to build ProtoReflectionService, it could be a good extension point for gRPC-Java ecosystem.

voidzcy commented 4 years ago

The server reflection protocol in gRPC was designed

as an optional extension for servers to assist clients in runtime construction of requests without having stub information precompiled into the client.

So its primary usage is to expose services running on a gRPC server.

Armeria's usage of ProtoReflectionService is a pretty hacky way that takes advantage of Server's interface to expose a list of custom service definitions at runtime via the the reflection service. It is not what we intended.

You could design/define your own gRPC service to expose other services at runtime that does not depend on Server interface. A.k.a., a simple version of ProtoReflectionService (btw ProtoReflectionService itself is just a normal gRPC service and it's an optional layer installed on top of the a gRPC server).

ikhoon commented 4 years ago

You could design/define your own gRPC service to expose other services at runtime that does not depend on Server interface. A.k.a., a simple version of ProtoReflectionService

We don't really need ProtoReflectionService itself. A simple version sounds good to me. 👍

ejona86 commented 4 years ago

I'd actually expect Armeria to have a reasonably good time just returning the existing Server shim from ServerCall instead of passing it to notifyOnBuild(). That's not great, but the lifecycle methods are of dubious use from the ServerCall API. Note that it is also possible to do that just for the reflection service via an interceptor.

It might be fair to split the non-lifecycle methods out of Server, to have a "managed" vs "unmanaged" split like we have for Channel. But it seems that may be an overkill.

ikhoon commented 4 years ago

Note that it is also possible to do that just for the reflection service via an interceptor.

As you said, I implemented an intercepter that injects a dummy server to the current gRPC context. It is a workaround but a hacky way. It would be nice to support an API not to depend on gRPC internal implementations. :-)

ejona86 commented 4 years ago

Oh, I forgot we swapped to the internal Context key. Earlier in the iteration we had a ServerCall.getServer() API. We were having trouble settling on the replacement API and it seemed better to do the Context hack as an internal API to replace the previous broken internal API instead of delaying any longer.