Feature: Server-side request interceptor (#749)

[ReubenBond]

Based on the discussion in #749.

[gabikliot]

You don't need XML file for the test. We have programmable API for providers and there is a virtual base method in the unit test class that can be overwritten to tweak the config object .

[gabikliot]

Thumbs up.

[ReubenBond]

Many thanks, @gabikliot. I'll push an update in a minute.
EDIT: pushed

[Plasma]

Awesome! Very simple implementation and pretty much exactly what I'd like to use for pre-method call setup (in my case, extracting the User ID from the RequestContext, then setting up the Thread.CurrentPrincipal, then invoking the method).

One suggestion for my use case (only me?) would be for this interceptor to return an IDisposable, which is disposed of when the call completes, so we can dismantle any context (eg, set Thread.CurrentPrincipal back to NULL, by returning a class that will do that upon disposal):

// From
public Action<InvokeMethodRequest, IGrain> PreInvokeCallback

// To
public Func<InvokeMethodRequest, IGrain, IDisposable> PreInvokeCallback

Then in the calling code:

// From
SiloProviderRuntime.Instance.CallPreInvokeCallback(request, target);
resultObject = await invoker.Invoke(target, request.InterfaceId, request.MethodId, request.Arguments);

// To
using (SiloProviderRuntime.Instance.CallPreInvokeCallback(request, target))
    resultObject = await invoker.Invoke(target, request.InterfaceId, request.MethodId, request.Arguments);

Thoughts?

[yevhen]

@Plasma I don't understand why would you need a Disposable here?

If interception will be implemented in a way it is implemented in every other project out there, specifically as decorator pattern, where you control invocation - you will have an easy way to understand when the call is finished.

But I suspect it want happen due to blocking from a core team, as they're scared that we'll abuse it (for the greater go0d). Poor puppy's are we ... 😄

[sergeybykov]

@Plasma Wouldn't a symmetric post-invoke callback be a cleaner solution for what you need than IDosposable?

[Plasma]

@yevhen @sergeybykov Yep either of those two options sound great to me.

[yevhen]

@sergeybykov forget about IDisposable. That solution was a obvious no-go for anyone.

We are not even discussing per-type interceptor. What we are really discussing is 2 different interception approaches:

1. Pipelining - like in ASP.NET MVC, OWIN, PostSharp, Akka and many many other frameworks and libraries
2. Pre/post calbacks - like in old ASP.NET WebForms model and ... I have no idea who else use this model, seriously

The first model has a number of clear advantages such as:

1. Well-known. It's a de-facto standard approach for doing request interception.
2. It's composable.
3. It allows to cancel request if needed.
4. It has clear correlation between pre and post execution of request.

The second model:

1. As far as I'm aware, used only in a old farty framework that need to die.
2. Non-composable. Complete lack of composability.
3. Don't allow to cancel requests. Even if it is, a proposed api will look much uglier, ie PreInvoke(PreInvokeArgs args); args.Cancel = true;
4. Has no clear correlation between pre and post execution of request. Even if it is, a proposed api will be ugly as hell, ie PostInvoke(PostInvokeArgs args); if (args.RequestId = ...;

Regarding second model: been there, done that. It was incredibly frustrating experience. Anyone who tried to do anything meaningful with ASP.NET WebForms Pre/Post model know what I'm talking about.

If you're against that virtual method - fine. I do care only about approach. If it will be pipelining implemented only as global callback - it's fine. I'll be able to add per-type interception in my own sandbox then. But if it will be a pre/post callback - it is the same as having no feature for me.

And let's remove personalities from this discussion. I don't really care what @yevhen or @Plasma need or think is enough for his particular situation. But I do care about other people looking at that and thinking: Holy crap what they were smoking when designed that feature?

The end.

[gabikliot]

#965 (comment)

[gabikliot]

The second model:

1. does allow to cancel requests by throwing exception from the pre decorator.
2. Has a clear correlation between pre and post execution of request, if we go with the per type per instance approach suggested in README: Documentation heading link #945. When processing is individual per grain instance, can use per grain interceptor. When it is the same for all grain types and all grain instances can use global.
3. I don't understand the composability argument. Can you explain? What do you want to compose that is not possible in the 2nd model?
4. "used only in a old farty framework that need to die." Maybe. I will try to find more links. Definitely Post Sharp supports both, and this is a clear evidence that both are valuable.

[yevhen]

does allow to cancel requests by throwing exception from the pre decorator.

But what if there are valid reason not to throw but still cancel?

Has a clear correlation between pre and post execution of request

no, it's not. Due to interleaving (reentrant) grains, PostInvoke will not correlate with PreInvoke. Users will need to correlate themselves.

I don't understand the composability argument. Can you explain?

Sure. The correlation problem described above, is exactly due to lack of composition. Try to do this with Pre/Post model:

PerformLogging(
    CollectStatistics(MessageCategory.Command,
        PublishEvents(Log, publisher,
            RetryOnConcurrencyConflicts(retryOnConcurrency, Log,
                RetryOnDuplicates(retryOnDuplicates, Log,
                    HandleIdempotency(Log,
                        WithinDbTransaction(store, tx =>

                            request => handler(request)
)))))))(x));

[gabikliot]

Good point about reentrant grains.

OK, I remove my objection to pipelining interceptors. The need to manually correlate the pre and post for reentrant calls is a problem.

[yevhen]

Thumbs up!

1 нояб. 2015 г., в 17:55, Gabriel Kliot notifications@github.com написал(а):

Good point about reentrant grains.

OK, I remove my objection to pipelining interceptors. The need to manually correlate the pre and post for reentrant calls is a problem.

—
Reply to this email directly or view it on GitHub.

[gabikliot]

Went to a run and thought more about it.
Pre and post decorators can easily be correlated, for any type of grain, reentrant and not, via RequestContext. RequestContext is already isolated per request.
That will work also for global interceptor, not just for per type interceptor. So @Plasma 's scenario is easily done this way.

Still did not see a single example where a value need to be returned from the pre decorator without invoking the actual logic ("cancelation and not throwing" scenario). Caching I think is too application specific, and is also different per method and not per grain type, so better done inside the grain logic itself.

[ReubenBond]

Using RequestContext is hackish and leads to non-obvious code. We should work to avoid it.

Having request interceptors as described allows for a much cleaner design: it gives consumers the option of accessing the grain directly. They can access/modify private members or use the request scoping of their IoC container

EDIT: I rather we do not merge this until the design in #965 is settled.

[jthelin]

Note to self:
The corresponding docs change for this PR are in #964, so we will only want to merge that PR when this is merged.

[gabikliot]

So after all the discussion here and in #965 we decided not to merge it? I would personally suggest to merge this one (global pre and post decorators) and then keep iterating and enhancing the per-instance interceptors that were suggested in #965.

[sergeybykov]

I think we are ready to merge #965. Do we also need this one?

Fine with me, if you think this is useful for certain scenarios regardless of #965.

[ReubenBond]

Given #965 is merged and we settled on semantics in that thread, I will update this PR and try to achieve parity. I believe it's best to not change the semantics of the GrainClient hook (pre-invoke) and to implement server-side hook in the same fashion as #965

[sergeybykov]

@ReubenBond Can you rebase this one? Will you have time to complete it? If not, we can try to do that.

[Plasma]

@sergeybykov when comparing this PR to #965 (comment) - does #965 adequately address server side interception?

Maybe I'm misreading it but does the grain intercept code in #965 happen on the server side (thus it can access memory / logical state on the silo and do whatever it wants before the code actually executes)? If so, I personally don't need this PR anymore.

[ReubenBond]

@Plasma yes, #965 is for server-side grain interception, too. It does not allow for a single global interceptor, though.

@sergeybykov I've rebased privately, but this PR is not ready to prime-time yet.

[ReubenBond]

This class was shifted from another file & should not be functionally different to how it was with #965

[ReubenBond]

Rebased & squashed 😄 Please critique & consider merging.

[sergeybykov]

These params don't match the callback definition.

[ReubenBond]

Thanks, will fix.

[sergeybykov]

Since InvokeInterceptor is a delegate, if I add more than one interceptor, they will be called in parallel, not as a pipeline? And each interceptor will invoke the grain via invoker?

[sergeybykov]

Thank you, @ReubenBond for completing it!