L12: C# Interceptors Proposal #38
Conversation
|
@jtattermusch What do you think about merging the |
L12-csharp-interceptors.md
Outdated
| underlying `CallInvoker` handler for the final interceptor in the chain: | ||
|
|
||
| ```csharp | ||
| TResponse BlockingUnaryCall<TRequest, TResponse>(Method<TRequest, TResponse> method, string host, CallOptions options, TRequest request, Func<Method<TRequest, TResponse>, string, CallOptions, TRequest, TResponse> next); |
There was a problem hiding this comment.
nit: add line wraps for better readability.
L12-csharp-interceptors.md
Outdated
| proposal suggests adding an `Items` dictionary to `ServerCallContext` to hold | ||
| arbitrary state. | ||
|
|
||
| ## Rationale |
There was a problem hiding this comment.
I think we would benefit from one of:
- extra section with main use cases and examples.
- examples directly in the implementation PR (as unit tests or in Grpc.Examples project).
In my experience example snippets is where potential flaws of a new API are the easiest to spot and without it reasoning about the API pros and cons becomes very abstract.
L12-csharp-interceptors.md
Outdated
|
|
||
| In the general case, the interceptors are not obligated to invoke `next` once. | ||
| They might choose to not continue with the call chain and terminate it | ||
| immediately by returning their own desired return value, or potentially call |
There was a problem hiding this comment.
I think allowing shortcircuiting the call and retries are an interesting use case, but I'd like to see an example of those to make sure such scenarios really work.
L12-csharp-interceptors.md
Outdated
| code when certain events happen in asynchronous and streaming calls. These | ||
| helper static functions are the only reason this facility could not have been | ||
| implemented by an external library: they require access to the internal | ||
| constructors of `AsyncUnaryCall<TResponse>`, |
There was a problem hiding this comment.
FYI we've actually been thinking of making those constructors public (they're useful for testing as well).
L12-csharp-interceptors.md
Outdated
|
|
||
| Since support for this handler substitution was needed from the | ||
| `ServerServiceDefinition` class itself, a general internal method | ||
| `SubstituteHandlers` is added to actually apply the mapping given the wrapping |
There was a problem hiding this comment.
nit: if it's an internal method, it's probably irrelevant for the public API discussion here - it's purely a question of implementation.
L12-csharp-interceptors.md
Outdated
|
|
||
| ## Rationale | ||
|
|
||
| The design goals where flexibility, decoupling and non-disruption to existing |
There was a problem hiding this comment.
one design goal worth explicitly calling out is to not break the existing API surface.
- Unify `ClientInterceptor` and `ServerInterceptor` classes into one `Interceptor` base class, so that implementing a single class to serve on both sides becomes possible. - Update the signature of the client interceptor hooks, taking a context object instead of the contextual data in-line. Also, rename the `next` delegate to `continuation` for clarity. - Update the server-side interceptor machinery to reflect the new design, which is able to intercept the call before reading the first request and express interest in further interception if necessary. - Formatting and clarifications.
|
@chwarr please take a look at the updated proposal (and the PR) |
L12-csharp-interceptors.md
Outdated
| AsyncClientStreamingCall<TRequest, TResponse> AsyncClientStreamingCall<TRequest, TResponse>( | ||
| ClientInterceptorContext<TRequest, TResponse> context, | ||
| AsyncClientStreamingCallContinuation<TRequest, TResponse> continuation) | ||
| ``` |
There was a problem hiding this comment.
Minor: looks like premature termination of the code block here.
|
Looking at my schedule, I'm unlikely to have time to look at the implementation PR until next week at the earliest, and perhaps not even then. Please do not block on me. |
|
@mehrdada @jtattermusch looks like this one is ready to be merged. Could you please do so? |
|
Please do not merge this. There are some learnings from Python that I would like to apply on this. It is at the top of my priority list after P0 resolution. |
|
@mehrdada any progress? |
|
Friendly ping: @mehrdada Any progress on this? :) |
|
@mehrdada the interceptors have been available in c# for a while, we should get the proposal finalized and merged. |
|
@jtattermusch Done. PTAL and add a commit to change the status to Accepted if satisfactory. |
|
@mr-at0s just uploaded the polished prose and @jtattermusch should be able to merge |
L12-csharp-interceptors.md
Outdated
| invocations are separate and both need to be implemented and overriding one | ||
| does not automatically apply to the other. | ||
|
|
||
| Of the aformentioned methods, the ones that intercpet request-unary RPCs take |
Addresses grpc/grpc#11646
Implementation: grpc/grpc#12613
Discussion: https://groups.google.com/d/topic/grpc-io/GKcnFC3oxhk/discussion