Feature: Per-Grain-class Request Interceptors

[ReubenBond]

This feature allows users to override Grain.Invoke(InvokeMethodRequest request, IGrainMethodInvoker invoker) in order to perform request interception on a per-Grain-class level.

As requested by @yevhen in #749.

Example:

public class InterceptRequestGrain : Grain, ISomeGrain 
{
  protected override Task<object> Invoke(InvokeMethodRequest request, IGrainMethodInvoker invoker)
  {
    RequestContext.Set("InterceptedValue", 74);
    return base.Invoke(request, invoker);
  }

  // Other methods...
}

[ReubenBond]

We create local variables to hold request.InterfaceId, etc.

[jason-bragg]

Concerns:

• This approach works well for the simple case where one wants to do something simple before/after each grain call, but call specific actions would be difficult.
• Grain developers need to provide the invoker in the grain implementation. If they are doing this, why not just hook in the logic directly. Why even get the framework involved.

Alternative:
Since each grain adheres to a specific interface, a decorator pattern seems a natural approach to intercepting calls. Consider an interface:

public interface IInterceptor : IGrain where T : IGrain
{
Grain Intercepted { set; }
}

If instead of a Invoke call to override in the grain, the grain can have a protected virtual property containing the interceptor.

public abstract class Grain : IAddressable
{
...
protected virtual IGrain Interceptor { get; set; }
...
}

After constructing a grain of type G, if the interceptor property is empty (developer did not override it), the grain construction can use the ServiceProvider to create an implementation of IIntercepter. If one is found, the interceptors Intercepted property is set to the grain and the grain's Interceptor property is set to the interceptor.

When invoking grain calls, if the interceptor is set, it is passed to the invoker's invoke call, rather than the grain, if it is not, the grain is used, as usual.

The interceptor implementation would simply be a decorator that conforms to the grain's interface and the Interceptor interface. It would intercept each grain call, do what it wants, then passed the call through to the real grain (set in the Intercepted property).

This allows grain developers to provide their own interceptors, or for interceptors to be injected after the fact via the service provider (for monitoring, debugging, fault injection, testing, ...)

Thoughts?

jbragg

[ReubenBond]

@jason-bragg thanks for the input :) Grain developers don't need to provide the IGrainMethodInvoker, it's provided by the framework.

The Interceptor approach you outlined can be implemented like so:

public class InterceptRequestGrain : Grain, ISomeGrain 
{
  protected override Task<object> Invoke(InvokeMethodRequest request, IGrainMethodInvoker invoker)
  {
    if (this.Invoker != null)
    {
      return invoker.Invoke(Interceptor, request);
    }

    return base.Invoke(request, invoker);
  }

  protected virtual ISomeGrain Interceptor { get; set; }

  // Other methods...
}

InvokeMethodRequest is already exposed publicly through the GrainClient interceptor. I have been considering adding support to retrieve the relevant MethodInfo for a given InvokeMethodRequest (or the constituent InterfaceId + MethodId), which would help to support other scenarios (authorization filters, for example).

I am open to your suggestion @jason-bragg.

@yevhen: do you have anything to add?

[yevhen]

This approach works well for the simple case where one wants to do something simple before/after each grain call, but call specific actions would be difficult.

quite opposite. It covers all possible use-cases I can come up with.

Grain developers need to provide the invoker in the grain implementation. If they are doing this, why not just hook in the logic directly. Why even get the framework involved.

As @ReubenBond already said that's not the case.

Since each grain adheres to a specific interface, a decorator pattern seems a natural approach to intercepting calls.

That's exactly the situation were trying to avoid with proposed design. The proposed design is actually a better more lean implementation of decorator pattern. Consider the interface below:

public interface ISomeGrain : IGrain
{ 
    Task Foo();
    Task Bar(string x);
}

To implement simple logging, with the design that you' suggesting I will need to hand-write an implementation like the one below:

public class SomeGrainDecorator : Grain, ISomeGrain
{ 
    ISomeGrain next;

    public SomeGrainDecorator(ISomeGrain next)
    {
        this.next = next;
    }

     public Task Foo()
     {
          // log before
          return next.Foo();
          // log after
     }

     public Task Bar(string x)
     {
          // log before
          return next.Foo(x);
          // log after
     }
}

I will basically need to hand-write and duplicate every method for every grain interface in the system, if I want to log all calls to a grains. Imagine a system with tens of interfaces and hundreds of methods. That will be an incredible (wasted) effort.

On contrary, with the @ReubenBond proposed design, I will be able to simply create a base class from which all my grains will inherit from and put this logic in a single place.

public abstract class AppGrainBase : Grain 
{
    override Task<object> Invoke(InvokeMethodRequest request, IGrainMethodInvoker invoker)
    {
          // log before
          return invoker.Invoke(Interceptor, request);;
          // log after
    }
}

@ReubenBond I don't have anything else to add

[yevhen]

@Plasma will that be enough to fulfill you request about server-side interception?

[jthelin]

Could you explain about what this part of the change does please?

s/IdentifierNameSyntax arguments/ExpressionSyntax arguments/

[ReubenBond]

The change was introduced when I was (temporarily) using an expression (method.Arguments) instead of a variable (arguments). It is no longer needed, but I should have been using that type there anyhow - IdentifierNameSyntax is a sub-class of ExpressionSyntax and the code requires an expression, not an identifier.

[jthelin]

Documenting some design sanity check items that i looked at, mostly for the record:

• We already have the relevant InvokeMethodRequest object is already lying around in both InsideGrainClient and OutsideGrainClient, so there will be no extra object creation overhead from this change.
• The InvokeMethodRequest object is sealed and constructor has internal-to-runtime visibility and private setters. The Arguments property is object[] so an Interceptor could potentially alter specific argument entries before passing to downstream Invoke, but cannot add or remove extra arguments (array size is fixed) So for all practical purposes the object is effectively immutable.

Other comments:

The question about which specific "interceptor API" should be used is hard to make without a clear statement of the real world usage scenarios.
This was a HUGE and long-running discussion topic back in the day of the Apache Axis2 project (compare: Handlers and Modules), so is definitely not an easy topic to get right.
https://axis.apache.org/axis2/java/core/docs/Axis2ArchitectureGuide.html#extending

Issues #749 and #963 don't really give enough scenario background to fully clarify the design intent for this change IMHO.
The included test case gives one (very simple) usage example, and @yevhen code snippet for logging interceptor are good starters.
But it feels like we need to surface more of the usage scenarios to properly decide between competing Interceptor / callback API proposals that are being suggested.

Thoughts?

[jason-bragg]

@ReubenBond & @yevhen All good points.

"I will basically need to hand-write and duplicate every method for every grain interface in the system"
I agree that having the single point of entry for the invoker (the virtual invoke) call is less work than having to implement hooks for the entire interface, when some common behavior is needed. This is a nice capability in this design that we should preserve.

I'll take a step back from implementation details and explore the usage pattern for interceptors that I am working from.

I would expect someone to be able to write a grain that implements a grain interface without concern for intercepting calls. Then, once there is a need to intercept a grain's calls, write an interceptor for the grain that the system somehow magically uses.

For interceptors I was expecting for a way in which someone could add interceptors after the fact without changing the grain. It seems to me that intercepting grain call is about injecting some logic in-between the caller and called behavior. To require the interceptor to be part of the intercepted grain, to me, negates much of it's value.

I would expect someone to be able to write something like the below.

public interface ISomeGrain : IGrain
{
Task Foo();
Task Bar(string x);
}

Interceptor could hook all calls

public class SomeGrainLogInterceptor : IIntercepeter
{
public Task Invoke(InvokeMethodRequest request, IGrainMethodInvoker invoker)
}

or hook each call

public class SomeGrainCallMonitorInterceptor : Intercepeter, ISomeGrain
{
Task Foo();
Task Bar(string x);
}

Is the above a reasonable expectation for interceptors?

[jason-bragg]

@jthelin "But it feels like we need to surface more of the usage scenarios" +1

[ReubenBond]

I very much appreciate your input, @jason-bragg, @jthelin, & @yevhen :)

Here's a few usage scenarios from the top of my head:

• Logging
• Pre-call authorization
• Pre-call/post-call invariant checking
• Automatic storage persistence (if method is not marked [ReadOnly] & no exception thrown, perform WriteStateAsync - not saying this is necessarily a good idea, but users could could have a base class which implements this). This is similar to ServiceFabric's actor semantics, by the way.

It enables a degree of Aspect Oriented Programming.

Things like Event Sourcing may become easier to implement, too. We can ensure that all outstanding events are applied before invoking a method. We can also reset an activation if an exception is thrown (depends on desired semantics/coding practices).

Of course, these things may not be desirable for everyone, but this PR puts the choice in users' hands.

Some of those things become easier with additional information. For example, I would like to be able to get the corresponding MethodInfo for the InvokeMethodRequest. That might look something like invoker.GetMethodInfo(InvokeMethodRequest request, Type target) - type is there because implementations of a grain interface have different MethodInfo to interface itself, most importantly because of ability to add different attributes, such as [ReadOnly], or [Authorize]

EDIT: Many of the Cross-cutting Concerns listed in this Wikipedia article can be solved using grain interceptors

[Plasma]

@yevhen Thanks for the ping - I think for me #963 solves my use case more elegantly, but if this Invoke() method was added to IGrain then I would just add this logic to each grain base class as needed, so either way I am not as fussed (#963 seems a bit cleaner given how client interceptors work?). The logic I would be doing would roughly be extracting the User ID from RequestContext and setting up a Principal object.

I agree with the above points that the decorator pattern for intercepting ("I will basically need to hand-write and duplicate every method for every grain interface in the system") is not the way to go.

Great PRs thanks Reuben.

[gabikliot]

My main issue with this PR is the invoker that is passed to the interceptor code.
The way it is done now, we "route" the request to the interceptor and its interceptor's role later on route it to the actual call (or not).

First, code wise, I don't think IGrainMethodInvoker should be passed to the application and be part of the programming model (it maybe need to be public, due to code gen, unfortunatelly, but should not be passed to the app code). InvokeMethodRequest fine, but not the invoker itself.

More importantly, interceptor for me is a code that executes before the call but does no change where the call goes, can not substitute the call itself, and is not to be used for "routing" requests differently then the framework intended (even the behavior on thrown exception should be frameworks decision). The way interceptor is written now, developers will be able to do so. Maybe we don't intend them to, but they will be able to not invoke the actual call, or even implement their whole app logic inside the single interceptor method, thus basically throwing away the strong typeness of the arguments, thus going (backwards, in my opinion) to Akka/Erlang weak typed actors.
We may not want them to do so, but they will able to. People will abuse it and it could be become an alternative "weakly typed" programming model. And I don't think we should encourage them.
If we do want such a "routing" functionality, lets discuss it as separate feature and not under interceptor category.

For interceptor I would stick with the same method signature as we have for a global interceptor, returning void, not accepting invoker. Just add a virtual protected function on the grain base. Also, no need to change any code gen. Just call this base function, just like you do for a global interceptor directly. Can even optimize with a precomputed flag if the override exist or not.

Additionally, it is not even clear, after @Plasma's response, that anyone actually needs the per type interceptor. The global interceptor (#963) looks like enough.

[ReubenBond]

I'm happy either way.

We could hide the invoker or we could split this into PreInvoke/PostInvoke methods. If the latter, then I would want the PostInvoke method to be passed any exception which was thrown from the grain method or PreInvoke call.

I much prefer Task as the return type, but if you're of a strong opinion that it should be void, then I will use void.

[yevhen]

Meh

[ReubenBond]

Ultimately we cannot stop users from hurting themselves while still letting them execute code. The goal is only to discourage bad behavior.

How about something similar to this in Grain:

protected virtual Task<object> Invoke(InvokeMethodRequest request)
{
    return this.currentInvoker.Invoke(this, request);
}

internal Task<object> Invoke(InvokeMethodRequest request, IGrainMethodInvoker invoker)
{
    // BTW: this could go on IActivationData, but there is a comment telling me that we should not 
    this.currentInvoker = invoker; 
    return this.Invoke(request);
}

That way, users must call base.Invoke(request) instead of having access to the invoker.

BTW, it may make more sense to access the method invoker from the ActivationData instance, since that's where it currently lives. I saw this comment in Grain, though, and shied away:

// Do not use this directly because we currently don't provide a way to inject it;
// any interaction with it will result in non unit-testable code. Any behaviour that can be accessed 
// from within client code (including subclasses of this class), should be exposed through IGrainRuntime.
// The better solution is to refactor this interface and make it injectable through the constructor.
internal IActivationData Data;

Not 100% sure what "directly" means in that context... the line below it is this:

internal GrainReference GrainReference { get { return Data.GrainReference; } }

We can solve this without bike-shedding.

[veikkoeeva]

I'll add a few existing interceptor implementations to facililate discussion:

• NancyFx: The before and after module hooks
• Logging and Intercepting Database Operations

Both of them has capabilities listed and some rationale on them. Altering the flow of call is something people expect, I believe. For instance the case with @Plasma where a request could be aborted with an authorization failure.

I could think of a few more use cases such as caching and error handling. Has anyone ideas how to improve and utilize the first-pass, still rudimentary DI we have? Maybe it'd be beneficial to think this in context of #934 too, even if it broandens the scope. One of the initial ideas, which I'm fond of too, in bringing DI was to make it look like what other parts of .NET stack is likely to use but still taking account the Orleans model. That way we'd benefit from the broader system and make already learned knowledge applicable here too.

[ReubenBond]

This can already use DI - you have access to your grain, which is constructed using DI.
I am quite happy with the latest proposal (simple Invoke signature with a call to base.Invoke(request)). Got to run, but let me know what you think.

[yevhen]

base.Invoke(request) is exactly what I have in Orleankka 👍

[gabikliot]

Reuben, can you please describe your scenarios where:

1. you need a per grain type interceptor and a global interceptor, like done in Generic external serializer interface #953, is not enough.
2. you need to return a value from the interceptor and also control the call's flow (notice that Generic external serializer interface #953 allows to throw an exception which aborts the rest of the execution).

[ReubenBond]

For 1) the global interceptor runs before, not around, the invocation, so it cannot do things like ensure that invariants are met.
For 2) Caching is one example - I'm not sure it's a valuable one, though.
Controlling the call's flow is useful when you need to ensure a precondition is met before any call (eg in Event Sourcing: all outstanding events have been applied) and that a post condition is met after any call (eg in Event Sourcing: if the call fails then ensure no internal state has been changed / no events were emitted)

It is also useful for @Plasma's case where he wants to inject a lifetime-scoped IPrincipal object. In his case, the interceptor may look like this:

using (container.BeginScope())
{
  // Similar to Web API's ApiController.User...
  this.User = container.ResolveInstance<IPrincipal>();

  return await base.Invoke(request);
}

It's not so much that we need the return value, but rather that is the most obvious way to implement this and I don't see a compelling reason to hide the return value. The return value can be useful for logging/diagnostics, anyhow.

This is structurally similar to the model used in OWIN & ASP.NET 5, by the way

[yevhen]

What about making a copy of local state before every request, say for safety/purity reasons? It will be a 10 min exercise for anyone who want to implement this, if we have per-grain type interceptor. I can have SnapshottedGrain base class doing simple cloning of state before invoking actual handler.

With event-sourcing and some additional metadata, like knowing when something is a query or command, I can optimize this logic further, and make snapshots of the state only when request is a command (I can inspect it easily).

@ReubenBond

This is structurally similar to the model used in OWIN & ASP.NET 5, by the way

indeed. It's the same Pipeline model.

[ReubenBond]

On that note, I would prefer that our global interceptor also used a similar approach: rather than running before a request, it should run around a request like an OWIN/ASP.NET middleware. This allows for greater composability.

[gabikliot]

Can you please share a link to OWIN interceptors?

[gabikliot]

Ok, so Postsharp has both options:

1. method decorator - "it allows you to add instructions before and after method execution. You may want to use method decorators to perform logging, monitor performance, initialize database transactions or any one of many other infrastructure related tasks".

2. method interception - the hook gets invoked instead of the method. "It is often useful to be able to intercept the invocation of a method and invoke your own hook in its place. Common use cases for this capability include dispatching the method execution to a different thread, asynchronously executing the method at a later time, and retrying the method call when exception is thrown."

Both are clearly valuable. "Decorators are faster than interceptors, but interceptors are more powerful". Specifically, in his example he shows things that can be done with interceptor and not with decorator (custom retry policy).

Clearly (if we embrace this terminology) I was thinking about pre and post decorators and not about interceptors. Mostly since that is what we had already implemented for client pre-invoke "interceptor", but also since that is what I used in the past in other systems (CORBA) and there is was called pre and post "interceptors".

So I guess Reuben is asking for interceptors and not decorators.

The argument in favor of interceptors is that it is more powerful.
The argument against interceptors will be that it may provide too much power and encourage escaping/breaking from the Orleans model. I can see later on similar arguments of "why doesn't Orleans allows to inject my own thread scheduler with its own non-necessarily single threaded guarantees". While some may actually argue in favor of such a full customization, it comes at the price of increased complexity. Adding more and more different customizations increases the mental load on the future user, there are more concepts to learn, more public APIs, more choices. At the end the system inevitable becomes more complicated. There is nothing wrong in principle with such a complicated system (Akka is such), except that ... it is not Orleans. It will become a complicated beast like WCF. On the other hand, of course, we cannot lock the system and not allow new features and more customization. Where is the line?

The line goes via an open and willing to listen discussion, where sides can be convinced in the opinion which is different from their initial. We, the Orleans core team, are not vetoing anything. And after reading the examples in Post Sharp, I am more inclined to be convinced that interceptors are more valuable than decorators and that maybe we indeed should embrace them and not decorators. But there are still tradeoffs in my opinion, the choice is not a clear cut, and thus this discussion.

[gabikliot]

And thanks @veikkoeeva for sharing the links:
NancyFx: The before and after module hooks - they have pre and post decorators (which they called interceptors, like I initially did), not full interceptors.

And in Logging and Intercepting Database Operations there is yet a third option: pre and post decorators with optional returned result. If the pre decorator returns a result, the actual operation is not invoked and the decorator's result is returned instead. But still, the pre decorator does not invoke the original operation itself (so its not an interceptor in PostSharp terminology).

[galvesribeiro]

I saw some comparisons with Postsharp and OWIN/Nancy on what this PR is all about... So my question is... Is clear the intentions/design on allow only interception of a grain call per grain-type in order to REALL intercept and maybe modify the behavior of the request(maybe return a value and never call the real operation itself)? Or there are some intention on give the user the ability to just execute custom code before and/or after a grain call (such as logging) ?
isn't clear (for me) what was the original motivation for that feature (or I was so dumb that didn't understood yet)...

[sergeybykov]

This is obviously not needed any more.

[sergeybykov]

I suggest IInterceptor -> IGrainInvokeInterceptor.

LGTM. I like your solution with an explicit interface.

[ReubenBond]

Sounds good, @sergeybykov - I'll make the suggested changes soon. Thanks for the CR

[gabikliot]

LGTM on the approach.

Would not IInterceptable be a better interface name for the grain to implement? From grain standpoint, it is interceptable.
We already have IRemindable.
Will maybe have IStreamable in the future, on the same note.

[ReubenBond]

I think IInvokable works - the grain can receive Invoke calls, but that's already taken by the framework 😄

IInterceptable might imply that outsiders can intercept grain calls.

What do you think?

[sergeybykov]

@ReubenBond I had a similar reaction - IInterceptable seems to mean that somebody else can intercept calls to the grain. IWhaeverIntercetor OTOH conveys that it's the grain that intercepts.

[gabikliot]

OK, the "standard" in .NET appears to be IInterceptor http://docs.autofac.org/en/latest/advanced/interceptors.html
https://github.com/nhibernate/nhibernate-core/blob/master/src/NHibernate/IInterceptor.cs
http://simpleinjector.readthedocs.org/en/latest/InterceptionExtensions.html

so maybe IGrainInterceptor? Why do we need invoke in it?

[ReubenBond]

@gabikliot I'm happy with that. Do you think we should rename the method to Intercept, too, in order to be in-line with Autofac & SimpleInjector?

[gabikliot]

I wasn't implying that.
I don't know.
I just Googled (not Binged, Googled) for Interceptable and IInterceptor and found only one example in some library in Java for the former while all .NET examples from famous frameworks that I know used the latter. That convinced me thatIInterceptor is the way to go.

Yes, probably Intercept is better than Invoke. I am less opinionated about that.

[centur]

can I tap in with the name - ICanIntercept or ICanInterceptSelf to indicate that grain can intercept requests to self, but not to other grains

[sergeybykov]

@centur I see nothing wrong with ICanIntercept or ICanInterceptSelf other than that I think stylistically it would stand out from the rest of the names we used so far. I think IXYZIntercptor is more conservative and inline with the rest of the codebase naming style wise.

[centur]

As I said to Reuben - it's your call on naming, I just found all these ISomething-bla-bla-able, especially when they have only single word for description, so hard to comprehend when reading code, so my personal rule of thumb is that if adding Can\Has\Is\Be helps reader to read code - I'll add it and care less about style but more about understanding. In a long run projects end up with multiple interfaces implementation like IInterceptor, IInterceptable,IInvokable on the same class and then it's even harder to sort them out

[galvesribeiro]

Great work @ReubenBond, thanks!

[sergeybykov]

👍 Now we can start bringing Orleankka pieces in. :-)

[wanderq]

Thank you, @ReubenBond. Officially top commented PR to the Orleans of all time.

[Plasma]

Amazing; thank you @ReubenBond and team. 👍

[gabikliot]

Good milestone indeed @ReubenBond !

[yevhen]

Congrats! 🎉

[veikkoeeva]

I'll make the sensation of achievement one more 👍 longer. :)

[gabikliot]

Now that this was merged we also need to update documentation:
http://dotnet.github.io/orleans/Advanced-Concepts/Interceptors

[edikep2000]

Hello and Good Morning Everyone.
Please I did like to disable code generation altogether, My CI Pipeline does not integrate well with this procedure, I have read in some places that it is possible to disable Code Generation Altogether. I did like to know how this could be achieved

[sergeybykov]

@edikep2000 You need to add https://www.nuget.org/packages/Microsoft.Orleans.OrleansCodeGenerator/ to your silo and client processes. Though I think you better open a new issue, as this one seems unrelated.