are these .Intercept() methods used anywhere?

They are provided for interceptor implementors to be able to intercept additional events in async or streaming calls. The initial interceptor hook is only invoked at the beginning of the call.

These functions are only needed because the constructor for AsyncXYZCall are internal. If we are going to make them public, now would be a good time. I could just remove all of this mess.

I'd quite like a way of creating an interceptor to just modify/inspect one aspect (e.g. metadata) of all calls - Interceptor.ForMetadata(metadata => { ...})

I agree, and have a few other helper extension methods in mind. I wanted to keep this PR minimal to the core hooks to make it easier to review... now I am thinking it might have been best to add such things for additional context?

I'm actually unsure what this is supposed to do.

I have thought about this a bit more and I think there is no good reason for this set of functions to exist, whose purpose is to circumvent the internal-ness of the constructors of Async***Call objects. If we open up the constructors, this whole thing becomes unnecessary.

What are the downsides to doing that?

Downsides:
we'd need to polish the Async*Call constructor signature and be certain that it's not going to change (that would be an API breakage and we are trying hard to avoid that). At the same we would lose some ability to add new members to the Async**Call (what if you need to add more arguments to the constructor later?).
Also, more generally, there is always some risk associated with exposing lower-level APIs to end users, because non-expert users tend to shoot themselves in the foot if you expose too much of the internals - and gRPC users shouldn't be required to become experts before they can use gRPC efficiently.

That said, I think it makes sense to revisit making Async**Call constructors public - now that we have an official support for interceptors coming.

FTR, here's an external PR for publishing the Async**Call constructor - but I think a bit more refactoring would be useful https://github.com/grpc/grpc/pull/9330/files.

we'd need to polish the Async*Call constructor signature and be certain that it's not going to change

The members are already public, so we cannot remove them and the constructor at the moment is basically initializing the existing members. We will be able to add additional constructors should we add new members (as long as we can find sensible defaults for values). In effect, by exposing this functionality within this PR under the interceptor API, we are transitively doing most of this API retention cost (admittedly it is a bit contained, but still would be breaking). I think we should seriously consider opening them up, since the class itself and its members are already not internal and the InterceptAsync* methods are really ugly in this PR.

I do agree that polishing the constructor now makes sense, but I don't see anything wrong with it. What would be the ideal in your opinion?

Yes, let's look into publishing the Async**Call constructors in a separate PR (and count on them being already public in this PR).

What do you think about not opening the constructors but adding a static factory method that returns a new instance? The advantage would be that we will retain the freedom to unseal the class and return a derived instance without an API breakage, but it is debatable that it would be worth much.

The PR for publicizing constructors is here: #12877 though I am still open to the static factory route idea.

would using a named delegate type be more readable than Func<Method<TRequest, TResponse>, string, CallOptions, TRequest, AsyncUnaryCall<TResponse>>?

Also, I think the name "next" can easily be confused as having to do with streaming, so we could come up with a better name (I think naming the delegate would also help here).

Impartial about the delegate type making much of a difference, since this is the only place they are used, but if you think that'd be better, I'll change to that. How about continuation for the name?

continuation sounds much better.

I have changed my mind a bit on continuation and have reservations about it since in the retry case, you can invoke it more than once and you can choose not to call it at all. How about invoker, though the downside is it is not an instance of CallInvoker.

What's the order of interceptors being applied if I do .Interept(interceptor1) .Intercept(interceptor2)? I assume it's the "decorator" style, so the interceptor applied as last will be invoked first (which is the same order as if I stacked CallInvokers on top of each other).

Yes, that is how it is going to work.

I am considering adding another overload that takes params Interceptor[] and adds the list in-order (not the opposite order implied by decorator style).

I'd definitely add this order of operations to be clear. I pref FIFO, but I would say you want both functions to behave the same versus supporting one the opposite way.

I think FIFO does not make sense when chaining calls like .Intercept(a).Intercept(b), because you can imagine a scenario in which you are given a CallInvoker by another part of your program that is created with channel.Intercept and you then decide to intercept it again and see the whole thing as a black box.

However, I do agree that an overload that takes multiple interceptors in a single call: .Intercept(a, b, c) should pass control to a, b, and c in that order.

I think the Intercept(a, b, c) overload would be useful.

I think it'd be nice to also have an overload like:

.Intercept(p => {
        p.AddLogging();
        p.AddAuth();
    });

where p is a pipeline object of some sort.

This would make it very easy to develop packagable pipeline plugins by just creating an extension method for the pipeline object. Additionally in cases where you don't care about the type of TRequest / TResponse (eg. logging) you could create a generic extension method so that your plugin will work for all types of requests / responses.

@plaisted Can you clarify what is the advantage of that over

.Intercept(new Logger(), new Authenticator());

?

@mehrdada

Functionally they are the same but using the extension method allows you to nicely encapsulate what "API" you want the user of the package to use vs. what the actual constructor that the class needs.

As a contrived example the Authenticator interceptor may require a TokenValidator service so it's constructor is actually Authenticator(ITokenValidator validator). Adding an extensions method in your package of AddJwtAuth() would simply construct the Authenticator interceptor with a JwtTokenValidator that implements ITokenValidator.

Now the extension method would generally do a lot more than that, maybe even add multiple interceptors which is trivial with this sort of setup:

//extension method
public static void AddJwtAuth(this IInterceptorPipeline pipeline, Action<JwtOptions> optionsBuilder) {
    var options = optionsBuilder(new JwtOptions());
    pipeline.Add(new Authenticator(new JwtTokenValidator(options)));
    pipeline.AddInterceptor2(); // add a dependent interceptor
    pipeline.AddInterceptor3(); // add another
}

//using it
.Intercept(p => {
        p.AddJwtAuth(o=>{ o.ValidationCertificate = cert; });
    });

I've written a few addons (aspcore mainly) that use this sort of approach and it works very nicely as you don't have to expose any implementation details to the users of the package. Admittedly all of this other than adding multiple interceptors could all be implemented relatively easily using a static "factory" method so a lot of this is preference and wanting to stay similar to how I see this done across different frameworks in c#.

Isn't erasing the type to <object,object> unfriendly to users? What are some example situations where I'd want do use the "items" object?

Have we considered using a .NET context to carry the extra state? Also I think some .NET context objects use string as a key (not sure what's better here actually).

I'm not sure what you mean by ".NET context" object? Synchronization context?
Yes, my initial version actually had this as <string, object> like HttpContext does, but I found myself wanting to use typeof(MyInterceptor) as a key, and it would not have worked with string keys, and I found no reason to restrict the dictionary to string keys (keys are generally used as input, not output, so they are rarely read to need explicit casting)—and for the value object it did not make sense to have anything less general than object.

What I have in mind is a higher level interceptor base class that inherits from ClientInterceptor that uses this dictionary under the hood, but relies on a convention to store data in that dictionary and expose it in a type-safe way.

I'm a huge fan of object versus string, sometime I like to store things via by type.

Get < T > ( ) for example.

I'm concerned that this already-large struct is becoming huge. The threading is also a concern... Dictionary<,> isn't thread-safe - what are the expected uses here, and are we satisfied they won't cause problems?

@jskeet The use-case is to share some call-specific data between the interceptors and potentially the call invoker. (The closest .NET analogy I have in mind is HttpContext.Context.) I initially thought Metadata headers should be abused for this purpose, but it can be too inconvenient. For example, one use case is associating a User object (or any other state information) via an interceptor that can then be pulled and injected into the RPC. Another design I contemplated with is replacing the dictionary with a single object Tag, however, from a struct size point of view, it has the same effect. It will alleviate the threading concerns though, perhaps at a cost to usability.

I understand the use case for "items", but I share @jskeet 's concerns.

Idea - could we postpone adding the "items" field and later add it without breaking the API? Adding the basic functionality for interceptors in this PR and having one or two "improvements" PR might actually be productive approach.

Happy to move items to a different PR, but this is an important requirement it seems, and we need to tackle it soon anyway. Metadata strings are a really ugly approach.

Another idea is to introduce an InterceptorContext which holds interceptor state and pass it along in the continuation chain. However, the downside is there is no clear way to share that information with the final CallInvoker (which might or might not be necessary). If we are going to do this, we can't do it in another PR, because that is a core API change in the interceptor itself (adding another argument to interception callbacks. and the continuation delegate. I'd really like to know what @jskeet thinks about this alternative solution.

Just turn this into an automatically implemented property? (Ditto for Host potentially, but that's not being changed in this PR, so it may want to be done separately.)

I considered this, but all (or almost all?) properties in gRPC C# code base seem to be not automatically generated. I wonder if it is intentional for compatibility purposes with older compilers. That's why I kept consistent with the approach in the codebase.

ad auto-properties: there's no particular reason why we are not using them, but you're right the most times I didn't use them so for now it might makes sense to stay consistent.

I think autoproperties have been in C# since early version, so compiler compatibility is not a concern here. But there are other language constructs (from C# 6) that we intentionally didn't use because until about a year ago, we needed the code to compile on older mono and Visual Studio. Now when we switched to the new dotnet projects, we could start using C# 6 in our code (the compiled code will still be compatible with older .NET and the generated code needs to still compile with older .NET) - so feel free to use C# 6 if you want to. Sometime in the future, we might refactor portions of our existing code and introduce C# 6 constructs where appropriate.

I'm concerned that this already-large struct is becoming huge. The threading is also a concern... Dictionary<,> isn't thread-safe - what are the expected uses here, and are we satisfied they won't cause problems?

Possibly clarify that any previous items are effectively lost rather than merged?

I'd quite like a way of creating an interceptor to just modify/inspect one aspect (e.g. metadata) of all calls - Interceptor.ForMetadata(metadata => { ...})

All this reflection looks inefficient to me. I haven't quite got my head round how it's all used at the moment, but it would be great to avoid it...

This process only happens at the startup time of the service once, so I don't think the potential performance impact matters. Unfortunately, I don't know of any other way to avoid this dance, since dynamic or using C# binder to find the appropriate overload require depending on additional libraries. We need this to map a non-generic object back to the correct parameterized-type overload. If you have a better way to accomplish this in mind, definitely let me know please.

This method is async and return await next(), while the client test is non-async-but-still-Task and just returns next(). Is the difference intentional?

Which client method? Line 69?

I was referring to difference between the client interceprot test
(in ClientInterceptorTest.AddHeaderClientInterceptor.BlockingUnaryCall) and the server interceptor test.

The relevant client test code is:

public override TResponse BlockingUnaryCall<TRequest, TResponse>(Method<TRequest, TResponse> method, string host, CallOptions options, TRequest request, Func<Method<TRequest, TResponse>, string, CallOptions, TRequest, TResponse> next)
{
    options.Headers.Add(this.header);
    return next(method, host, options, request);
}

It just returns a Task, while in the similar ServerInterceptorTest.AddRequestHeaderServerInterceptor.UnaryServerHandler, the method is marked async and has the pattern return await next(...). Since there's no other async action in this method, I would have expected a non-async method that just return next(...);

Is there something special about the server-side or is this an unintentional difference?

Thanks for pointing it out. Fixed.

nit: you can't actually use Intercept(a,b,c) when you're passing Func<Metadata, Metadata> as an interceptor, so the comment is a bit misleading.

I think it's fine to leave as is.

nit: since C# 6 you can use nameof(invoker) instead of "invoker".

nit: code style: I tend to put the private classes after all the instance methods as they are basically "helpers" (and the tests is what you should see first when reading the file).

nit: you don't need to set the UnaryHandler here IMHO.