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. On Fri, Oct 13, 2017 at 9:23 AM, Mehrdad Afshari <[email protected]> wrote: > 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/CABdARdB_b3-cpOTrUEFo2asH-wQbD1WEMPdE9K4PCWN%3DrGH_JQ%40mail.gmail.com. For more options, visit https://groups.google.com/d/optout.
