BindService should not require a concrete instance of the GRPC service implementation

[shirhatti]

Currently we need an instance of the service implementation to call BindService and we are creating a dummy instance to bind the service right now: https://github.com/JunTaoLuo/GrpcSandbox/blob/105f2bc65ad1b6b642a62bec31789315936c78b2/Server/Dotnet/GrpcRouteBuilderExtensions.cs#L34-L36. Since we are exploring the use of Scoped services, we may not have an instance until the request is in flight.

[JamesNK]

I looked into this and I see two potential solutions:

1. We use the Descriptor property that is generated on the service definition. That has all the information that we need: method name, method type, input message, output message.

2. The tools are updated to generate a BindService overload that doesn't take an implementation. ServiceBinderBase would have a new virtual method that takes a Method<TRequest, TResponse> and it is called in the new BindService overload.

The nice thing about (2) is it provides a strongly typed hook for handling the method, i.e. you have TRequest and TResponse, and those generic types can then be used to call the marshaller.

On the other hand, getting into a generic context with (1) isn't that difficult. Combine some Type.MakeGenericType reflection with the input and output messages types and you're done. I lean towards doing (1) for now.

Thoughts @JunTaoLuo?

[JunTaoLuo]

I didn't see option (1) before but that sounds intriguing. I'll prototype it out and see what that looks like.

[JunTaoLuo]

Implemented option (1)

[JunTaoLuo]

@jtattermusch Now that we've changed the implementation, I'd like to propose that we add an option to the grpc code generation to skip generating the BindService methods. This would remove ServiceBinderBase and ServerServiceDefinition from Grpc.Common/Api and keep those types in Grpc.Core.

[JunTaoLuo]

@JamesNK I realized that option (1) actually adds a dependency on the Google.Protobuf package since we need access to the ServiceDescriptor. Grpc.Core goes around this issue by relying on the generated code for service binding, which isolates the GRPC implementation from the underlying message protocol (ProtoBuf). Though we can influence how to design Grpc.AspNetCore, I'm wondering if the isolation is something we need to keep in mind.

[JamesNK]

Good catch. I don't know. As long as we don't expose protobuf types externally we have the flexibility to remove it easily. We can discuss next meeting about whether this is an issue or not.

[mkosieradzki]

Isn't some kind of isolation required to retain FlatBuffers support as an alternative serialization protocol?

[csteegz]

FlatBuffers doesn't support C# right now, only C++. Bond over GRPC would be the other open source serialization to think about. That being said, option 2 would require an update in the tooling for any other serialization protocol that wanted to support it.

[JamesNK]

Isn't some kind of isolation required to retain FlatBuffers support as an alternative serialization protocol?

Requiring a dependency on Google.Protobuf for metadata about a service doesn't necessarily mean that another library couldn't be configured to be used for serialization. It wouldn't be ideal though.

I've created an issue to investigate supporting multiple serialization libraries - #30

[jtattermusch]

@JunTaoLuo Good catch with the protobuf dependency (Now I remember this was one of the reasons why things we used the ServerServiceDefinition, it's long time ago).

[jtattermusch]

Isn't some kind of isolation required to retain FlatBuffers support as an alternative serialization protocol?

Requiring a dependency on Google.Protobuf for metadata about a service doesn't necessarily mean that another library couldn't be configured to be used for serialization. It wouldn't be ideal though.

Requiring to read the Google.Protobuf descriptor for metadata wouldn't work for other serialization formats - in order to get a Google.Protobuf descriptor you need a service definition in .proto format (which in turn requires all the .proto files with message definitions to exist), and you wouldn't have that if you were using some other serialization format (bond, flatbuffers, thrift etc.).

I've created an issue to investigate supporting multiple serialization libraries - #30

[jtattermusch]

FlatBuffers doesn't support C# right now, only C++.

It does seem that Flatbuffers C# exist.
https://github.com/google/flatbuffers/tree/master/net/FlatBuffers

Bond over GRPC would be the other open source serialization to think about. That being said, option 2 would require an update in the tooling for any other serialization protocol that wanted to support it.

Right now, Grpc.Tools works for gRPC + Protobuf. If one wanted to use other serialization format (which is absolutely doable with gRPC C# today), one would need to write their own codegen and create their own tooling package that would invoke the codegen (for code integration support). Supporting more serialization formats in Grpc.Tools doesn't seem like a good idea as every codegen is different and likely has different options.

[jtattermusch]

I'd be leaning towards option 2) as it seems cleaner and doesn't involve dependency on Google.Protobuf

Alternatively, there's a serialization-format agnostic version of option 1) - instead of relying protobuf ServiceDescriptor, the generated code could expose a read-only list of Method<> instances (their private instances are there already, but one currently cannot list them https://github.com/grpc/grpc/blob/b6d8957b0d78f768b858698ba6a79296db448bb2/src/csharp/Grpc.Examples/MathGrpc.cs#L35). But this is already very similar to option 2), it just differs in the way one accesses the list (explicitly or implicitly by getting AddMethod calls). Option 2) has the advantage of giving access to the generic context.

Btw, the BindService(grpc::ServiceBinderBase serviceBinder, MathBase serviceImpl) method has been added very recently and is marked as experimental API, so we can afford to modify it if needed.