grpc-java icon indicating copy to clipboard operation
grpc-java copied to clipboard

Consider configuring `ProtoReflectionService` to use it without internal APIs

Open ikhoon opened this issue 4 years ago • 5 comments

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

  • Armeria: https://line.github.io/armeria/docs/server-grpc
  • Heldion: https://helidon.io/docs/v2/#/se/grpc/01_introduction
  • Micronaut: https://micronaut-projects.github.io/micronaut-grpc/latest/guide/index.html

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.

ikhoon avatar Jun 18 '20 05:06 ikhoon

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).

voidzcy avatar Jun 18 '20 09:06 voidzcy

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. 👍

ikhoon avatar Jun 18 '20 10:06 ikhoon

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.

ejona86 avatar Jun 18 '20 15:06 ejona86

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. :-)

ikhoon avatar Jun 18 '20 15:06 ikhoon

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.

ejona86 avatar Jun 18 '20 17:06 ejona86