gRFC L12: C# Client and Server Interceptor Support

[Mehrdad Afshari]

I've submitted a gRFC for adding C# client and server interceptors:

https://github.com/grpc/proposal/pull/38

Implementation: https://github.com/grpc/grpc/pull/12613

Feedback is appreciated. Per the gRFC process, please keep discussion in this thread.

Cheers,

Mehrdad

[jos...@jungleegames.com]

I'd love to help get this in - it's the best feature yet!

[Christopher Warrington - MSFT]

> Per the gRFC process, please keep discussion in this thread.

There seems to be significant discussion going on in the pull request. I have some comments. Would they be preferred here or at https://github.com/grpc/proposal/pull/38?

[Mehrdad Afshari]

General comments on the proposal are preferred here. Comments that are tied to on the implementation are probably easier addressed on the pull request of the implementation, i.e. https://github.com/grpc/grpc/pull/12613

[Christopher Warrington - MSFT]

General comments on the proposal are preferred here. Comments that are tied to on the implementation are probably easier addressed on the pull request of the implementation, i.e. https://github.com/grpc/grpc/pull/12613

1. The proposal talks about adding an Items dictionary for interceptors' satellite data. What types are thought for the key and value? A string -> object map? (I skimmed the implementation, and I think this is actually being dropped for the first version due to multi-thread access concerns.)

2. It seems implied that an interceptor can manipulate the request or response objects. Yet, the interfaces are all generic. Is an explicit is/as check/cast expected if an interceptor wants to look at the actual payloads? (Kinda "in other words": I don't see a way to register an interceptor for a specific "Foo" method with concrete types where I get an actual FooRequest object instead of TRequest.)

3. It looks like the server-side interceptors all are invoked after unmarshalling has been performed. If I wanted to write a RejectRequestsIfServiceOverloadedInterceptor that fails requests, it would be better if I didn't have to unmarshall the payloads just to reject them. Though, I'm not sure if this is enough motivation for having to deal with the extra complication of having interceptors injected at different places in the lifetime of a call...

4. I'm confused by the section starting with "the class provides a few static methods that lets the interceptor register additional hooks to execute code when certain events happen in asynchronous and streaming calls", particularly by the set of static protected methods that come after it. I'm not sure what type these live in or who would override them. Can you demonstrate/point to in the implementation PR an example of how these would be used?

[Jan Tattermusch]

Thanks for the feedback, Christopher! Mehrdad might have more answers. For now, I'll comment on some of your questions.

On Fri, Oct 13, 2017 at 8:34 AM, 'Christopher Warrington - MSFT' via grpc.io <grp...@googlegroups.com> wrote:

General comments on the proposal are preferred here. Comments that are tied to on the implementation are probably easier addressed on the pull request of the implementation, i.e. https://github.com/grpc/grpc/pull/12613

1. The proposal talks about adding an Items dictionary for interceptors' satellite data. What types are thought for the key and value? A string -> object map? (I skimmed the implementation, and I think this is actually being dropped for the first version due to multi-thread access concerns.)

We are currently discussing the type of "items" in https://github.com/grpc/grpc/pull/12613. So far it's undecided.

 

2. It seems implied that an interceptor can manipulate the request or response objects. Yet, the interfaces are all generic. Is an explicit is/as check/cast expected if an interceptor wants to look at the actual payloads? (Kinda "in other words": I don't see a way to register an interceptor for a specific "Foo" method with concrete types where I get an actual FooRequest object instead of TRequest.)

3. It looks like the server-side interceptors all are invoked after unmarshalling has been performed. If I wanted to write a RejectRequestsIfServiceOverloadedInterceptor that fails requests, it would be better if I didn't have to unmarshall the payloads just to reject them. Though, I'm not sure if this is enough motivation for having to deal with the extra complication of having interceptors injected at different places in the lifetime of a call...

Thanks for bringing this up! Mehrdad I believe we should mention this topic in the proposal doc (if this is possible/not-possible and why). If we allow pre-unmarshalling interceptors (and we should at least consider that as there are definitely use cases for them), we should include pre-unmarshalling interceptor as one of our examples.

 

4. I'm confused by the section starting with "the class provides a few static methods that lets the interceptor register additional hooks to execute code when certain events happen in asynchronous and streaming calls", particularly by the set of static protected methods that come after it. I'm not sure what type these live in or who would override them. Can you demonstrate/point to in the implementation PR an example of how these would be used? 

--
You received this message because you are subscribed to the Google Groups "grpc.io" group.
To unsubscribe from this group and stop receiving emails from it, send an email to grpc-io+unsubscribe@googlegroups.com.
To post to this group, send email to grp...@googlegroups.com.
Visit this group at https://groups.google.com/group/grpc-io.
To view this discussion on the web visit https://groups.google.com/d/msgid/grpc-io/852bd664-db54-4c61-9303-00ba886c6314%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

--

Jan

[Mehrdad Afshari]

Thanks Christopher for comments and Jan for some answers. I should update the proposal since there were some changes in the design, but to add to Jan's answer.

1. We will likely drop this feature in the initial pull request and add it later.

2. Yes, it will needed to be registered generically. Otherwise, at the current point the interceptor is hooked, the gRPC code invoking the interceptors will need to reflect on the type and wire it up to the correct method with reflection. I think what we do here is a good way to handle it, as the interceptor code, which knows exactly what request type it is dealing with, can simply check `typeof(TRequest)` and feed it to the correct interceptor method accordingly.

3. I see this is a good point. I think such pre-unmarshal interceptions would need to happen at a quite different point in the RPC stack, and will be orthogonal to these "application level" ones. I will think about it more, but I don't know yet if the use case is acute or there is a more general use case than just dropping the requests based on headers or some other characteristic. Of the top of my head, I feel using a custom Decompressor or custom deserializer function you may be able to get that effect of interception in the gRPC pipeline. Also, this seems to only affect Unary requests, since for streaming calls, you have to actively read from request stream, which you can choose not to. (Note that under the current design, the interceptors intercept on Service handlers, not a full Server, and you can pick and choose what interceptors you want to register for a specific service handler. On the API level, one way to accomplish this is giving a `Task<TRequest>` instead of `TRequest` to the unary request interceptors, but it is not clear to me how best to wire this up to the gRPC core.) Please let me know if you have ideas here.

4. This is old and needs to be removed. The problem was the interceptors for streaming calls, if they were interested in further interception during the call, needed to wrap delegates in `Async*Call` objects with their own functions. However, the constructor was not accessible to the user (it was marked `internal`). We have since opened up the constructor for those objects eliminating a backdoor `protected static` set of functions, eliminating the need altogether.

[Mehrdad Afshari]

I happened to change the server side design to alleviate the concerns re (3): https://github.com/grpc/grpc/pull/12613/commits/c1e9d1dd72f969d539274b0f7b31ae3bb4cc7a5b

The trade-off is one additional null check in the fast path where no interceptors are present, which is probably negligible and worth the benefits.