Add IInvocation.CaptureProceedInfo() method

[stakx]

TL;DR: This resolves #145.

This is another attempt (after #438) at enabling asynchronous interceptors (#145) via an API addition that's as non-breaking as possible, in the sense of both "not breaking any abstractions" and "not introducing any breaking changes".

I abandoned #438 because the abstraction introduced there didn't seem quite right: We don't really need the ability to snapshot an IInvocation object's state (specifically, currentInterceptorIndex) and restore it later; what we do need is a way to make invocation.Proceed() calls work from an async continuation that executes when interception of invocation has already completed.

The new API introduced here is an invocation.CaptureProceedInfo() method that captures, in the form of an IInvocationProceedInfo descriptor object (think Reflection!), whatever invocation.Proceed() would do at that specific moment. That action can then be Invoked at a later time via that returned descriptor object:

 void IInterceptor.Intercept(IInvocation invocation)
 {
     invocation.ReturnValue = InterceptAsync(invocation);
 }

 async Task InterceptAsync(IInvocation invocation)
 {
+    var proceed = invocation.CaptureProceedInfo();

     await ...              // early return, i.e. interception completes before the next line
-    invocation.Proceed();  // this would "proceed" back to the very first interceptor

+    proceed.Invoke();      // this proceeds to the interceptor following the current one
 }

This should be sufficient to let people implement some kind of AsyncInterceptor base class.

What this does not solve is #238, i.e. the fact that AbstractInvocation.Proceed is inherently thread-unsafe due to the non-atomic currentInterceptorIndex increment/decrement dance. #428 may possibly do a better job in that regard, FWIW.

/cc @JSkimming, @brunoblank (sorry for needlessly CCing you in #438; I hope this PR will fare better)

[JSkimming]

@stakx I'm gonna clone/build your PR locally and look to get a PR raised with https://github.com/JSkimming/Castle.Core.AsyncInterceptor to see how things fare. I'll feed back ASAP.

[JSkimming]

@stakx, just some early feedback (I'm still hacking)

IInvocationProceedInfo.Invocation property

A property on IInvocationProceedInfo to return the underlying IInvocation would be handy. It will make the implementation of AsyncInterceptorBase simpler as I'm thinking of changing the two abstract methods to take just the IInvocationProceedInfo. This will allow implementations of AsyncInterceptorBase to still access the IInvocation if needed.

Heap allocation due to calling GetProceedInfo()

Your implementation instantiates the heap object for every interception that uses GetProceedInfo() which was something you wanted to avoid (IIFK from a comment on a different thread). I agree, and I'm wondering if a way to avoid this is possible.

Instead of GetProceedInfo() returning an interface, can it return a struct instead? The implementation is merely a reference and an int which isn't much more to copy than a reference.

[stakx]

@JSkimming - thanks a lot for trying this out right away! Hopefully that way, we can get this async riddle solved soon!

I'm thinking of changing the two abstract methods to take just the IInvocationProceedInfo. This will allow implementations of AsyncInterceptorBase to still access the IInvocation if needed.

Hmm, I would say it is very likely that an interceptor will want to look at the IInvocation. Because that's what interceptors usually do in some way or another. Generally speaking, IInvocation is probably more important to an interceptor than being able to Proceed. So, if I designed an interceptor API, I'd provide the Intercept methods with an IInvocation right away, instead of forcing them to retrieve it through an intermediate object. Just my 2 cents.

A property on IInvocationProceedInfo to return the underlying IInvocation would be handy.

While I'd prefer to keep this new type as tight as possible in the beginning, I have no real objection to an additional IInvocation Invocation { get; }, if you still think that's necessary (even after the above caveat).

Your implementation instantiates the heap object for every interception that uses GetProceedInfo() which was something you wanted to avoid [...].

Instead of GetProceedInfo() returning an interface, can it return a struct instead? The implementation is merely a reference and an int which isn't much more to copy than a reference.

I've considered making it a readonly struct, but decided against it because it is problematic from an API design point of view: Any IInvocation implementation should be free to decide how to model Proceed state. By making InvocationProceedInfo a value type with an int interceptorIndex, we'd force any and all implementations to be able to map their own state to that way of modelling Proceed state. That would work for present-day AbstractInvocation, but not in the general case.

This could be mitigated by simply having InvocationProceedInfo have a Action proceed field instead of a combination of IInvocation + int. But then someone would have to allocate that delegate, which also creates an object on the heap. Nothing gained!

I think I can live with this very small performance dip, as long as these objects only get built on demand ("you only pay for what you use"), vs. building a whole batch of them in advance.

Finally, this facility will likely only be useful when dealing with async interception peculiarities, and there's a lot more overhead involved in juggling with async / Task objects than allocating one or two objects on the heap, so in the big scheme of things IInvocationProceedInfo being short lived heap objects shouldn't matter all that much.

[brunoblank]

Good discussion and thoughts.

I like the fact that it makes a fairly straight forward workaround possible for the cases where await before proceed problem occur. This adds the extra complexity to how to implement the Invoke method.

This can be abstracted away in as @JSkimming is pointing out in AsyncInterceptorBase.

I also agree with @stakx regarding the cost of allocating the extra object on the heap shouldn't matter that much.

I am happy an fix for the long lived await problem is near :)

I do think we should take the opportunity to do the breaking change in AsyncInterceptor

• removing the Proceed method

I also think after looking at the changes AsyncInterceptor - that the splitting up the "return" value to different methods should be looked at again. Just make the "Proceed" method return a Task and have the return-value fetched from IInvokation (same as for "synchronous" DP)

Side-note:
If InvokeMethodOnTarget was added to IInvocation then AsyncInterceptor could solve this problem entirely internally.

[JSkimming]

@stakx, thanks so much for these fixes. It's a top effort to navigate the challenges of maintaining backwards compatibility of a very popular library, but also to help drive it forward, so first and foremost, hats off to you my friend.

I've picked up an old branch I was hacking on in a failed attempt to fix this in Castle.Core.AsyncInterceptor, and applied the changes against a local copy of Castle.Core. The changes are in this commit JSkimming/Castle.Core.AsyncInterceptor@96d9a0f.

⭐️⭐️⭐️⭐️ I can confirm it has resolved the issues. ⭐️⭐️⭐️⭐️

This is great. There is a number of outstanding issues against the library which were dependent on this fix.

The fix I applied above was with the IInvocation Invocation { get; } proprty. I added a second commit with the changes without it JSkimming/Castle.Core.AsyncInterceptor@d7b90f8.

So concerning the IInvocation property, I don't have strong feelings, if you prefer the cleaner IInvocationProceedInfo I think that's perfectly reasonable.

I'm also considering a bit of a refactor of AsyncInterceptorBase as I feel it's imposing too many assumptions. It's trying to make life simple for implementations by providing just two asyncronious methods to implement, but in doing so, it forces asynchrony on synchronous method invocations (with some hackery to get around it), and now it forces IInvocationProceedInfo on implementations even if they don't need it.

So given this will be a breaking change, I may choose a bit of a larger refactor at the same time.

Anyway, thanks again for this. It's very much appreciated.

[brunoblank]

@JSkimming glad to hear the fix is resolving the issues!

I think it is perfectly fine to force asynchrony on AsyncInterceptions :)

When thinking on the use case for AsyncInterceptor is when you are in an "async call flow". So either the method should be async (and everything is fine) or sync then the method "should be cpu bound" (this is fine also) and not I/O (this would lead to the sync over async threadpool thread starvation).

The point being - if that code was written without using Core it would face the same problem.

Given this assumption, the Invoke method could run the non Task based methods synchronously on the same thread and return Task.Completed

[stakx]

@JSkimming - That's some awesome news! Very happy to hear that you got things working! 🍾🎆

Thank you also for the kind words. ❤️ I must confess that I've stayed away from those async issues longer than I should have, simply because they seemed so daunting and complex. Now I'm more than a little surprised at how quickly we've been able to make progress; I didn't expect it to go so well. Thanks to you for giving feedback so quickly.

I'm probably not going to merge this just yet. Ideally, we'd get @jonorossi to look at this new API and give it his blessings. Also, do you think we should wait until after you've refactored your AsyncInterceptor library, or do you think we'll be good to go either way?

[brunoblank]

Also, do you think we should wait until after you've refactored your AsyncInterceptor library, or do you think we'll be good to go either way?

I vote for: good to go directly

[JSkimming]

[JSkimming]

It could be worth releasing a beta version of Castle.Core to nuget, we could then iterate on that and solicit feedback with a beta AsyncInterceptor.

[stakx]

@jonorossi - Could we do a pre-release that includes this PR anytime soon to unblock the AsyncInterceptor project?

[jonorossi]

Could we do a pre-release that includes this PR anytime soon to unblock the AsyncInterceptor project?

Right now you can get 0.0.516 from AppVeyor:
https://ci.appveyor.com/nuget/core-0mhe40ifodk8

We do have "Do not publish NuGet package artifacts to account/project feeds on Pull Requests" set but it is publishing them anyway. I can't find anyway to filter the feed so you'll just have to look at the last build and pick that exact version.

[ndrwrbgs]

Could you add a test that calls proceed multiple times? E.g. an async retry interceptor.

[stakx]

@ndrwrbgs - Can do. Alternatively, feel free to suggest a concrete test case, I'd be happy to cherry-pick it from you. You might be more efficient at writing that particular test, or even already have one ready for adaptation somewhere, given your previous involvement with async interception. If not I'll have a go at it.

[jonorossi]

The docs looks great (just one more comment), heaps more guidance than I expected which is good.

[jonorossi]

Seems like a good time to merge to master. Go ahead if you are ready.

[stakx]

@jonorossi: Thanks for reviewing this so carefully! 👍

[stakx]

@ndrwrbgs: While I did add two tests that proceed multiple times, I opted not to add too many test about async scenarios in order to keep this PR focused.

Feel free to submit a PR that e.g. adds more tests to AsyncInterceptorTestCase.cs. Like I said, you're perhaps quicker than me at writing the exact test you had in mind, anyway.

[ndrwrbgs]

@stakx I don't have the attention span right now to fully review what changes were involved, but I blindly took the change and the feature branch in https://github.com/JSkimming/Castle.Core.AsyncInterceptor and it seems that whatever you did + @JSkimming's changes in his project at https://github.com/JSkimming/Castle.Core.AsyncInterceptor/tree/hacking-for-async-fix have made it so an asynchronous retry interceptor is now possible, cool!

(not to mention other solutions, like asynchronous logging interceptor)