Allow multiple silo interceptors to be set

[richorama]

The current Orleans interceptor API provides methods for getting and setting interceptors:

InvokeInterceptor GetInvokeInterceptor();
void SetInvokeInterceptor(InvokeInterceptor interceptor);

The problem is that a second call to SetInvokeInterceptor will overwrite the original interceptor. This means that two bootstrap providing could be competing for a single interceptor, unless the developer observes any previously set interceptor.

I propose adding a new method to add an interceptor to an internally maintained stack. The runtime code will then run each of the interceptors in the stack in turn.

This would allow easier registration of multiple interceptors.

[ReubenBond]

Each interceptor wraps the invocation of the next interceptor, similar to an OWIN middleware.

request => {
  // do stuff
  var result = next();
  // do stuff with result
  return result;
}

Does ASP.NET 5 use a similar model to OWIN?

[galvesribeiro]

Yes. Asp.Net Core has the composable middleware behavior and we could use that as example

[Eldar1205]

Why not simply use the existing interceptor in the execution of the new interceptor?

[richorama]

@Eldar1205 yes, this is a valid solution, however it's not intuitive. You can't tell from the API that you need to wrap any existing interceptor, which leads to interceptors overwriting each other.

[Eldar1205]

I think the API leads to realization there is one interceptor since there is a Get which returns a single interceptor בתאריך 9 בספט' 2016 11:40,‏ "Richard Astbury" notifications@github.com כתב:

@Eldar1205 https://github.com/Eldar1205 yes, this is a valid solution, however it's not intuitive. You can't tell from the API that you need to wrap any existing interceptor, which leads to interceptors overwriting each other.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dotnet/orleans/issues/2000#issuecomment-245854432, or mute the thread https://github.com/notifications/unsubscribe-auth/AFBYt-ORcpd1Q-5r0x1LmFwcXDaj8cTjks5qoRt2gaJpZM4JYTiR .

[ReubenBond]

From an API perspective, how does this look:

// Make this internal/private:
// InvokeInterceptor GetInvokeInterceptor();

void AddInvokeInterceptor(InvokeInterceptor interceptor);
void RemoveInvokeInterceptor(InvokeInterceptor interceptor);

The configured InvokeInterceptor will form a chain and be executed in sequence as described above. There'll be no guarantee on ordering of interceptors - they will be executed in insertion order.

Are there cases where we need to be able to guarantee an ordering?

If we have general consensus, then I'll go ahead and implement this

[veikkoeeva]

@ReubenBond I think that's fine when it's clearly documented in the API doc. What about adding a getter to retrieve a snapshot of interceptors in order in case one wants to check it programmatically (e.g. dump the contents or do programmatic reordering based on it, using the explicit functions)?

[richorama]

@ReubenBond I like the design.

• Do you plan to mark SetInvokeInterceptor() as obsolete?
• I think that executing the interceptors in the order they are added should be the guarantee.
• I agree with @veikkoeeva's suggestion, perhaps a InvokeInterceptor[] GetInvokeInterceptors(); method?

[Eldar1205]

I suggest IReadOnlyList instead of array for the interceoptors list. That way you can return a refernce to internal collection knowing the caller doesnt change it. If you want to protect caller from other changes in parallel can use ImmutableCollections to avoid copying into array on every Get call

On May 31, 2017 11:29, "Richard Astbury" notifications@github.com wrote:

@ReubenBond https://github.com/reubenbond I like the design.

• Do you plan to mark SetInvokeInterceptor() as obsolete?
• I think that executing the interceptors in the order they are added should be the guarantee.
• I agree with @veikkoeeva https://github.com/veikkoeeva's suggestion, perhaps a InvokeInterceptor[] GetInvokeInterceptors(); method?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dotnet/orleans/issues/2000#issuecomment-305122124, or mute the thread https://github.com/notifications/unsubscribe-auth/AFBYt8lBAdhiN-ZsOOUIJVBswbb9DYM_ks5r_STigaJpZM4JYTiR .

[ReubenBond]

IReadOnlyList<T> it is.

IReadOnlyList<InvokeInterceptor> GetInvokeInterceptors();
void AddInvokeInterceptor(InvokeInterceptor interceptor);
void RemoveInvokeInterceptor(InvokeInterceptor interceptor);

Look at how this simple feature is spread out over too many locations:

With SiloBuilder we could do much better, but it's worth thinking about this now. We could require that interceptors be registered in the DI container - which would mean they're immutable, but that does simplify things. Would that be acceptable? It means that the APIs are removed. We still have the ordering guarantee. Alternatively we can wrap the above APIs in a targeted interface and register that in the container, so that the container has to be built before those things can be configured.

I prefer the simple DI approach, where we register each InvokeInterceptor with the container. What do you all think?

[DixonDs]

@ReubenBond Once you mention DI, does Orleans actually need to provide interceptors functionality instead of leaving it to IoC containers like https://www.nuget.org/packages/Unity.Interception/ or https://www.nuget.org/packages/StructureMap.DynamicInterception ?

[veikkoeeva]

@ReubenBond I like DI, and one can use a factory method to make them mutable in the sense the factory method can server what is needed. The targeted interface you propose is the same time, I think, but with more predetermined form and hence it's clearer about the ordering aspect.

[rikbosch]

@ReubenBond registering them in DI could possibly give unwanted results, when the DI provider does not respect the order of registration.

Better would be IMO to construct the interceptors from DI serviceprovider, but registering them explicitly in order in the silo (builder). This is how aspnet core does it's middleware resolving too:

https://github.com/aspnet/HttpAbstractions/blob/dev/src/Microsoft.AspNetCore.Http/MiddlewareFactory.cs

[ReubenBond]

@rikbosch ASP.NET also relies on registration order in places and it's required by all conforming containers: https://github.com/aspnet/DependencyInjection/blob/dev/src/Microsoft.Extensions.DependencyInjection.Specification.Tests/DependencyInjectionSpecificationTests.cs#L160

[Eldar1205]

I suggest to take into consideration that providing Orleans with implementation types for everything via DI always have at least two flaws which is IMO are significant: the first is that it's hard to document in code because there's no dedicated registration API in code - where would you document that multiple interceptors can be registered, and that the order matters? The interceptor interface doesn't sound to me like the place to document such things, the interface shouldn't know how its lifestyle is handled by certain parts of Orleans, and it should only know about the contract it represents and what Orleans expects of implementations. With dedicated "AddInterceptor" and "GetInterceptors" API, it's clear there's more than one just from method signatures and docs can emphasize the order part. The second flaw is discoverability - how can one discover via Intellisense that interceptors exist if they should be registered into a DI container? With dedicated API one would see it popping up in Intellisense some day, that's how I discovered the feature exists today and started to get more interested. I know that release notes, online documentation and unit tests are all ways to document an API an exists that uses the DI container, but from my experience with frameworks/libraries most people discover most features by just stumbling over them in Intellisense or as part of active Intellisense discovery, they usually don't read everything in online docs nor all release notes nor all Orleans unit tests (which must require looking at source code).

2017-06-02 19:39 GMT+03:00 Reuben Bond notifications@github.com:

@rikbosch https://github.com/rikbosch ASP.NET also relies on registration order in places and it's required by all conforming containers: https://github.com/aspnet/DependencyInjection/blob/dev/ src/Microsoft.Extensions.DependencyInjection.Specification.Tests/ DependencyInjectionSpecificationTests.cs#L160

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dotnet/orleans/issues/2000#issuecomment-305842546, or mute the thread https://github.com/notifications/unsubscribe-auth/AFBYt_vv1myZ1erUVty1lKwS18taHhPJks5sADq1gaJpZM4JYTiR .

[ReubenBond]

Where would you document that multiple interceptors can be registered, and that the order matters?

We would have to document this in documentation. It's very likely, though, that we would have an extension method on the SiloBuilder like: builder.AddIncomingRequestInterceptor((a, b, c) => ...)

The second flaw is discoverability

Having an extension method on the container also solves that.

[ReubenBond]

I added a PR for this: #3083 If you feel that the approach I've taken is wrong, please let me know

[Eldar1205]

I like the approach but the fact that the interceptors invocation order is from first to last bothers me a bit. If you look at ASP.Net WebApi message handler, the Nth message handler decorates the (N-1)th message handler, whereas in your implementation if I understand correctly the Nth interceptor decorates the (N+1)th interceptor. I would suggest to comply with how WebApi handled invocation order, I find it more intuitive. At any case I suggest for this aspect to be documented explicitly in the Obsolete message on SetInvokeInterceptor which says to use DI registration instead, and in the extension methods that will be added in the future.

2017-06-03 3:03 GMT+03:00 Reuben Bond notifications@github.com:

I added a PR for this: #3083 https://github.com/dotnet/orleans/pull/3083 If you feel that the approach I've taken is wrong, please let me know

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dotnet/orleans/issues/2000#issuecomment-305934836, or mute the thread https://github.com/notifications/unsubscribe-auth/AFBYty5Nt3m5rIFqxW-6MHaDDOYwJYPiks5sAKLdgaJpZM4JYTiR .

[ReubenBond]

@Eldar1205 I think of this more like the OWIN pipeline, which has insertion-order decoration.

pipeline.Add((ctx, next) => {Console.WriteLine("First"); return next()});
pipeline.Add((ctx, next) => {Console.WriteLine("Second"); return next()});

It looks like ASP.NET message handlers are the same:

Am I perhaps misunderstanding?

[Eldar1205]

In the diagram you sent, message handler 2 is registered last but executed first, e.g. it decorates message handler 1. Owin indeed does it the other way around, so I guess there's no standard paradigm from ASP.Net team. I find WebApi approach more intuitive, perhaps out of habbit.

On Jun 7, 2017 6:35 PM, "Reuben Bond" notifications@github.com wrote:

@Eldar1205 https://github.com/eldar1205 I think of this more like the OWIN pipeline, which has insertion-order decoration.

pipeline.Add((ctx, next) => {Console.WriteLine("First"); return next()});pipeline.Add((ctx, next) => {Console.WriteLine("Second"); return next()});

It looks like ASP.NET message handlers are the same: [image: image] https://user-images.githubusercontent.com/203839/26887095-f8348afe-4b5b-11e7-96bc-c0ba7f9724ea.png

Am I perhaps misunderstanding?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dotnet/orleans/issues/2000#issuecomment-306833445, or mute the thread https://github.com/notifications/unsubscribe-auth/AFBYtywKkdUimm18_z38rFR77gkuScpgks5sBsNTgaJpZM4JYTiR .

[ReubenBond]

@Eldar1205 are you sure about that? The diagram shows the request arrow going through Message Handler 1 before Message Handler 2, and then the reverse on the response side. It's conceptually like this (ignore that request/response are in properties for ASP.NET - in Orleans they're parameter/return vals):

var handlerOne = (ctx, next) =>
{
  // ~~ request side ~~
  DoStuff(request); // do something with the request.

  var response = await next(request); // Calls into handlerTwo, etc

  // ~~ response side ~~
  DoMoreStuff(response); // Modify the response
  return response;
}

[Eldar1205]

We use WebApi message handlers in the way I described, we have an exception handler that should decorates all others which is registered last, and I was told by the engineer working on it he verified it indeed catches exceptions from the other handlers. I will verify myself and share what I've found.

2017-06-07 19:54 GMT+03:00 Reuben Bond notifications@github.com:

@Eldar1205 https://github.com/eldar1205 are you sure about that? The diagram shows the request arrow going through Message Handler 1 before Message Handler 2, and then the reverse on the response side. It's conceptually like this (ignore that request/response are in some class properties - in Orleans they're parameter/return vals):

var handlerOne = (ctx, next) => { // ~~ request side ~~ DoStuff(request); // do something with the request.

var response = await next(request); // Calls into handlerTwo, etc

// ~~ response side ~~ DoMoreStuff(response); // Modify the response return response; }

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dotnet/orleans/issues/2000#issuecomment-306856941, or mute the thread https://github.com/notifications/unsubscribe-auth/AFBYt8pXheltBKXQTyKuskwl4OkHpTbRks5sBtWzgaJpZM4JYTiR .

[ReubenBond]

Thanks, @Eldar1205 - I appreciate it.

[Eldar1205]

Hi, we've re-verified and we were wrong, the order of WebApi message handlers is as you thought and similar to Owin. We actually caught a bug thanks to that check, so thank you for the discussion!

2017-06-08 19:16 GMT+03:00 Reuben Bond notifications@github.com:

Thanks, @Eldar1205 https://github.com/eldar1205 - I appreciate it.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dotnet/orleans/issues/2000#issuecomment-307153225, or mute the thread https://github.com/notifications/unsubscribe-auth/AFBYtwhggyNA5Q5GtL40Kws_aU9EolzCks5sCB56gaJpZM4JYTiR .

[ReubenBond]

Resolved by #3083