Support multiple request interceptors

[ReubenBond]

Implements #2000 and marks the existing interface methods as obsolete.

Interceptors are configured by registering InvokeInterceptor instances with the DI container in the ConfigureServices startup method, like so:

services.AddSingleton<InvokeInterceptor>((method, request, target, invoker) =>
{
    RequestContext.Set(InterceptedPropertyKey, 1);
    return invoker.Invoke(target, request);
});

The interceptor set via IProviderRuntime.SetInvokeInterceptor(...) is executed last.

Future work: add an extension method to the future SiloBuilder to make configuration more discoverable/obvious.

[jdom]

Did you consider using a delegate composition approach? something like ASP.NET/Owin middleware pipeline, or, more closely related to interception, Filters?
I guess I'm fine if we decide this approach is better, but I'd like to understand the reasoning for going this route

[jdom]

it's a little odd to check for Count > 1 as well as checking if this.CurrentStreamProviderRuntime?.GetInvokeInterceptor() != null. I understand why you are doing it, but shouldn't you just not add the deprecated interceptor to the list and always treat it separately here?

[ReubenBond]

It's quite unfortunate. How would it look if I treated it separately? I'm not sure

[ReubenBond]

@jdom I considered that. This is the main reason why I didn't take that route:

Task<object> InvokeInterceptor(
  MethodInfo targetMethod,
  InvokeMethodRequest request,
  IGrain target,
  IGrainMethodInvoker invoker);

The last argument is IGrainMethodInvoker:

public interface IGrainMethodInvoker
{
    int InterfaceId { get; }
    ushort InterfaceVersion { get; }
    Task<object> Invoke(IAddressable grain, InvokeMethodRequest request);
}

Note no invoker is passed and no MethodInfo is passed.

So these APIs do not compose well. Maybe it would be better if InvokeInterceptor was recursively defined like:

Task<object> InvokeInterceptor(
  MethodInfo targetMethod,
  InvokeMethodRequest request,
  IGrain target,
  InvokeInterceptor invoker);

We could rethink these APIs to remove the impedance mismatch. The mismatch causes more allocations than I would like, too :P

[ReubenBond]

To be clear: I would be happy for these APIs to be fixed so that we can implement this feature more cleanly. The mismatch in APIs is why I put this implementation off for so long - the logic in InvokeWithInterceptors makes me uncomfortable because I tried to make these features pay-for-what-you-use and that resulted in complexity.

[ReubenBond]

Looking more at ASP.NET Core for inspiration/alignment. As pointed out in your Filters link, @jdom, they have this ActionExecutingContext type. That type looks like our InvokeMethodRequest, except it also has a Result property.
This is then plumbed through to IAsyncActionFilter.OnActionExecutionAsync: Task OnActionExecutionAsync(ActionExecutingContext context, ActionExecutionDelegate next);.
Where ActionExecutionDelegate is defined as:

public delegate Task<ActionExecutedContext> ActionExecutionDelegate();

The reason for the return value there is that MVC supports both async and sync action filters. The context is passed from the async delegate into the (old) sync filter. It's not needed for us.

So mapping this to our world, we might have:

public delegate Task MethodInvocationDelegate();

public delegate Task MethodInvocationInterceptor(
    MethodInvocationContext context,
    MethodInvocationDelegate next);

public class MethodInvocationContext
{
    public IAddressable Grain { get; } // Maybe this class would wrap `InvokeMethodRequest`
    public MethodInfo Method { get; }
    public object[] Arguments { get; }
    public object Result { get; set; } 
}

After awaiting next() in an MethodInvocationInterceptor, context.Result will be set to whatever the method returns. context.Result is mutable, so it can be modified after awaiting next() anyway.

How does this API look?

[jdom]

Can you clarify something of the proposed API: would people just register their MethodInvocationInterceptor delegates in DI or would we still have a base interface and class similar to IAsyncActionFilter?

[ReubenBond]

I'm not sure, @jdom I feel that delegates are clear, but we could always allow interfaces too and convert between the two - method signature would be the same.

[ReubenBond]

@jdom, I pushed an update - let me know what you think of the latest revision. It needs to be squashed.

Also need some more comments - eg on the IServiceCollection extensions

[jdom]

I'll continue reviewing tomorrow. I like what I saw until now :)

[jdom]

Is the order correct? I would have imagined that the deprecated silo level interceptor came before the grain level interceptor

[ReubenBond]

you're right - good point

[jdom]

If any of the interceptor sets the Result property, it should short-circuit the state machine, right?

[ReubenBond]

I'm not sure. Is that more or less surprising to the user? I think more. If the user wants to short-circuit evaluation, they can by not calling await context.Invoke()

[jdom]

Just a thought, but should we call them Filters (or GrainCallFilters or something like that) now that it fully aligns with action filters?

[ReubenBond]

I was wondering the same thing yesterday. I'm amenable to the name.

[ReubenBond]

In latest, I've called them GrainCallFilters.

services.AddGrainCallFilter(context =>
{
    // .. do stuff with the context...
    return context.Invoke();
});

The type of context is still IMethodInvocationContext. I could call it IGrainCallContext

[ReubenBond]

@Eldar1205 I'm interested in your feedback (and others) about the proposed interface. If you or your team would like to take a look at this PR (eg, the tests) and tell me what you think, I would appreciate it.

[jdom]

in the future we may consider ValueTask for this, right?

[ReubenBond]

Yes, that's a good idea.

[jdom]

This string missed the rename to IGrainCallFilter

[ReubenBond]

ahhh thanks

[jdom]

either move to a different file (preferable) or at least rename the file, since this is now the important interface for interception

[ReubenBond]

will do, also documented it.

[jdom]

I understand you are probably showing how to break this, but this is really not how filters should be used, right? And since it's the only example of usage in a grain, it might be misleading. Do you want to create a separate test/example grain that doesn't do all these edgy stuff and just plays along with the happy path?

[ReubenBond]

I'll refactor the tests for the other grains which use the deprecated method and put a note here.

[jdom]

grain method named called Execute is hard to immediately know if this is related to interception or not, since Invoke and Execute both sound like infrastructure methods. Maybe rename to DoStuff or something that's clear it's a domain-level method

[ReubenBond]

Will rename to CallWithBadInterceptors

[jdom]

lol, this is kind of clever, although it's probably much more readable if this would end up concatenating "123456"....

[ReubenBond]

ok, sure - makes sense

[jdom]

when filters throw (either synchronously or return a faulted Task), will everything work fine? I couldn't find any coverage for that scenario (but tests are a little bit confusing, so there is a chance there is coverage and I'm missing it)

[ReubenBond]

I'll add coverage.

[jdom]

As we talked in person, I meant some more coverage with the filters throwing themselves too, and whether that's captured and inserted in the context, or it just bubbles up the async delegate chain. Looks like the exceptions bubble up through the chain, and there is no context.Exception like in MVC filters. I'm fine to not imitate this as long as we have a small note in our documentation mentioning this difference.
I think I'm with you that this extra property in MVC instead of just bubbling the exception might have been a side effect of the original synchronous filters that did not work or were composed with async constructs. Even if there is a different valid reason, then we could evolve this approach to catch the exception and add the extra property, but I'm fine with what we have for now, as that would be an additive change.

[ReubenBond]

How's this?

// Grain code
        public Task FilterThrows() => Task.CompletedTask;

// Filter code
            if (methodInfo.Name == nameof(FilterThrows))
            {
                throw new MyDomainSpecificException("Filter THROW!");
            }

// Test
        public async Task GrainCallFilter_FilterThrows_Test()
        {
            var grain = this.fixture.GrainFactory.GetGrain<IMethodInterceptionGrain>(random.Next());
            
            var exception = await Assert.ThrowsAsync<MethodInterceptionGrain.MyDomainSpecificException>(() => grain.FilterThrows());
            Assert.NotNull(exception);
            Assert.Equal("Filter THROW!", exception.Message);
        }

[jdom]

It is fine. It is not asserting the difference with MVC itself (that we propagate the exception as is from filter to filter, and from grain to filter), to know if we ever align with them, but I don't really think it's much worth it either.
This test does assert the most important aspect, which is that we eventually bubble up the exception to the grain client/caller, regardless of how it was handled by the filtering logic.

[jdom]

@richorama might be a good candidate to check whether it would still support the Dashboard in a good way (especially WRT to configuration)

[ReubenBond]

Added more test coverage & improved test execution time. You may notice I also added a new test class, that's because I had previously hijacked interception tests to check that [MethodId(x)] overrides were working. I've separated those into their own class/tests + added coverage for [TypeCodeOverride(x)].

[Eldar1205]

Hi Reuben, I've looked at the tests to see the new API and I really like the introduction of IGrainCallContext with Invoke() method to simplify invoking the next interceptor and the properties to access the current method call context. I do find some names misleading: - IGrainCallContext reminds of the .Net call context, e.g. LogicalCallContext. I would prefer the name IGrainMethodCallContext or even better IGrainMethodInvocationContext, but IGrainCallContext is also fine. - IGrainCallFilter reminds of the WebApi filters, where I assume the inspiration is from since the API doesn't necessarily filter anything, however WebApi filters looked very differently, and the API reminds more of the WebApi message handlers and Owin middleware mentioned previously, so I would prefer the name IGrainCallMiddleware or IGrainCallHandler or stick with well-known interceptor term and call it IGrainCallInterceptor. At any case, I find the term 'filter' misleading, though I understand if you don't like to change it due to the amount of code docs written with the term 'filter'. I'll also note that I'm one of those that dislike the usage of IServiceCollection as a way to hook behavior into Orleans, but it seems like the current trend so I'll accept it hoping the online documentation would list of all the hook points injectable via IServiceCollection, since it's not the most discoverable. The extension methods to IServiceCollection make it much better, but one still needs to have the correct 'using' to find them without using tools like ReSharper. 2017-06-13 20:57 GMT+03:00 Reuben Bond <notifications@github.com>:

…

Added more test coverage & improved test execution time. You may notice I also added a new test class, that's because I had previously hijacked interception tests to check that [MethodId(x)] overrides were working. I've separated those into separate tests. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3083 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFBYt49HUSIvY04j8qynkiPzb_qDdlc7ks5sDs17gaJpZM4Nu4jU> .

[ReubenBond]

@Eldar1205 thank you, much appreciated :)
The 'filter' name is based on IAsyncActionFilter from MVC. The motivation behind aligning with ASP.NET / MVC is to reduce the number of concepts people need to learn when moving between the two worlds.
As for naming, I'm open to suggestion - I'll discuss with @jdom & others, too.

The extension methods are in the Orleans namespace, so they should be easily discoverable. I'll make sure the docs are clear.

[richorama]

@jdom good idea, I'll take a look shortly.

[jdom]

can you implement the interfaces explicitly? It's still a little confusing without going back and forth to know which are these methods for, especially since these test grains implement both the new and the deprecated way of intercepting.
Also, it probably makes sense logically too. These are cross-cutting concerns not related to the domain, so having them implemented explicitly instead of publicly makes sense as a good example. Not that the behavior will be any different though

[jdom]

LGTM. Also regarding the naming comments, also welcome to suggestions, although I personally prefer the ones @ReubenBond used instead of the proposed ones.
Also regarding the similarity to Filters or OWIN Middleware, we also discussed that in person before the rename, but I stand with @ReubenBond in that the new asynchronous filters in MVC Core look very similar to ours (and to middleware too), and they also relate to action interception (grain call interception in our case which is very similar). If we were talking about pre-MvcCore filters, then I would agree.

[ReubenBond]

Fixed my silly throw expressions which broke CI

[jdom]

I'll wait a couple of hours in case there is more feedback, otherwise I'll merge it with the current approach and naming

[richorama]

I like the approach. 👍

[richorama]

I assume I can register an interceptor from a bootstrap provider?

[ReubenBond]

@richorama no, once the container is baked you can no longer register interceptors. This is for performance/simplicity reasons.

However, you could register some custom interceptor which has the ability to be modified at runtime in the container and then later manipulate it. Does that make sense?

weird half-example:

// during startup
var filter = new CustomFilter();
services.RegisterSingleton<IGrainCallFilter>(filter);
services.RegisterSingleton<CustomFilter>(filter);

// in bootstrap
public MyBootstrapProvider(CustomFilter filter)
{
  this.filter = filter;
}

public override Task Init(...)
{
  // manipulate it
  filter.Delegate = ctx => { Console.WriteLine(ctx); return ctx.Invoke() };
}

[richorama]

@ReubenBond yeah, I can work around that. In my case I need to grab the Task Scheduler in the bootstrap provider, and pass it over to the interceptor.