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. On Fri, Oct 13, 2017 at 12:43 AM, Jan Tattermusch <[email protected]> wrote: > 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 <[email protected]> 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/g >>> rpc/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 [email protected]. >> To post to this group, send email to [email protected]. >> Visit this group at https://groups.google.com/group/grpc-io. >> To view this discussion on the web visit https://groups.google.com/d/ms >> gid/grpc-io/852bd664-db54-4c61-9303-00ba886c6314%40googlegroups.com >> <https://groups.google.com/d/msgid/grpc-io/852bd664-db54-4c61-9303-00ba886c6314%40googlegroups.com?utm_medium=email&utm_source=footer> >> . >> >> For more options, visit https://groups.google.com/d/optout. >> > > > > -- > > Jan > -- 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 [email protected]. To post to this group, send email to [email protected]. 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/CABdARdDsswjNA16JJJ0LjS7WkkHyDoHK%2B%2BiqpO2v_eq%3DdSfCSA%40mail.gmail.com. For more options, visit https://groups.google.com/d/optout.
