-
Notifications
You must be signed in to change notification settings - Fork 467
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Asynchronous Interceptors #145
Comments
Not sure what you mean? |
Even if I try to hook up a continuation like suggested on StackOverflow it will still not work properly if I want to call in the continuation |
I dont think calling proceed in the continuation is a good idea anyways, you wont get a Task instance until you run the actual method. Looking fwd to your proposal. |
We've had this discussion some time ago. It breaks the abstraction On Thu, 3 Mar 2016, 10:34 hamilton verissimo notifications@github.com
sent from my phone |
@hammett You are correct, but you can make it work if you return My scenario is I have an I was thinking something like this might enable my scenario.
@kkozmic probably refers to #107 and that's probably more what @mlfletcher1988 is talking about. |
This comment has been minimized.
This comment has been minimized.
this looks as a very specialized use of the interceptors and TPL. I'm not sure it makes sense to add support for this at Castle.Core level, especially since TPL comes with its own pandora's box. |
Not really, it would just be a really good feature to allow async calls to be made before the intercepted invocation is proceeded. |
I'll leave this up to @jonorossi and @kkozmic then. |
I see no compelling reason. IMO it's a clear case of cost far outweighing the benefit |
With some architecture advice I would be happy to implement this as a proof of concept. Is there developer documentation/guidelines for contributing to the project? |
@ghost - Contributing guide here. https://github.com/castleproject/Core/blob/master/CONTRIBUTING.md |
Hi everyone, Is there any progress being made on this? |
@alex-training not an optimal solution (over there we also concluded the fixes are best made in Core) but https://github.com/JSkimming/Castle.Core.AsyncInterceptor at least helps with some cases of this. |
This stackoverflow question has the current options: https://stackoverflow.com/questions/28099669/intercept-async-method-that-returns-generic-task-via-dynamicproxy I am not aware of anyone working on DynamicProxy to contribute changes in this area. If someone wanted to put forward a summary of the changes that would need to be made to DP to support this we can consider this. |
My opinion on this hasn't changed since we first looked at this. Are there cases where this solution (or a variation of thereof) won't solve? |
@kkozmic not that I'm aware of, I'm of the same opinion, however I've wondered previously whether a helper extension method (e.g. |
@kkozmic All presented solutions are only workarounds, the biggest challenge is performance. Because in order to intercept asynchronous method with a result we have to use reflection:
In the https://github.com/JSkimming/Castle.Core.AsyncInterceptor library it is addressed via hacks around generics:
Nowadays asynchronous operations are widespread so I it would be nice to have a native support of them in the Core |
Just implement a layer of code generation in a base interceptor, maybe even with lambdas. I really dont see why Castle.Core needs to dabble with TPL. |
@hammett Well, code generation is actually what Castle.Core is responsible for. Imagine you want to introduce a caching layer for you async methods, such task should be easily implemented as follows:
I do not say we need some ProceedAsync, rather some efficient way to work with async methods out of the box. However, anyway, would you be so kind to present any examples of code generation? |
application? I said "base interceptor". Anyways, if you don't know how, that's a valid argument, but then I'd encourage you to work with @JSkimming. DP doesn't need to know about TPL coz it's not a special IL construct or metadata info. It's a compiler trick, as it was said before. DP is already a complex beast, and we all know each feature comes with a long tail. |
@hammett My point is that currently, implementing of such trivial tasks as caching, logging, retrying takes too many efforts with doubtful outcome (performance penalties) |
For example, PostSharp has such InvokeAsync method: http://doc.postsharp.net/m_postsharp_aspects_methodinterceptionaspect_oninvokeasync_5ba43ce6 And here is an auto-retry example: |
Regarding the message above, I’ve considered code generation in the async interceptor library. There are a couple of problems around how things are implemented in Castle today that require some major hacks from us over there (castle expects that when control comes back to it, the method is finished executing, which isn’t true in async). We could potentially (between myself and @JSkimming) try to contribute back to Castle, but we both felt the Castle people were much more proficient with such generation, the overall framework, and likely async than we are. |
I'll add it to my backlog :) Sadly, as I have a solution that's "good enough" for right now, I don't expect it to get done until end of year :( [well, unless it's dynamic interception for tracing purposes profiles to be too unperformant] @alex-training, I'd recommend giving the interceptor a try and seeing if it's really not performant enough. Further performance improvements can come later via a nuget upgrade if it's "good enough" perf for you right now. Note though, I'm not sure if we've documented it officially, but it will only work as long as your first "await" is |
@ndrwrbgs I really appreciate your efforts. Hope that more people will be interested in finding a real performant solution. I'll definitely give a try to the interceptor, although I have some concerns regarding underlying ConcurrentDictionary. |
I'm happy to give it a shot it, though I'm not sure what "it" is.
Background regarding current limitations of AsyncInterceptor@ndrwrbgs and I have hit somewhat of an impasse with AsyncInterceptor whereby we cannot make true1 asynchronous calls before calling To cut a long story short, this is because AbstractInvocation is state-full specifically We concluded that supporting changes are required in Castle.Core or we need to replace, if that's possible, the implementations of @ndrwrbgs, please do correct me if I've misrepresented things as you see it. Background regarding performance of AsyncInterceptorI've used this library in many live systems, though I fully concede this is anecdotal and YMMV. You should conduct performance testing if you're writing performance critical code, but that statement is true regardless of whether one is using third-party libraries or not. @ndrwrbgs and I have discussed producing some performance metrics, though it's not been high on my list because the performance of AsyncInterceptor is good enough for my needs. 1. true asynchronous being calls that return an incomplete Task. Task.Yield() is sufficient to exercise the problem Task.FromResult() or Task.CompletedTask is not. |
So, I took @brunoblank 's workaround and adapted it to my case (without IAsyncInvocation's Proceed returning a task but simply using regular IInvocation and returning the Task in Invocation.ReturnValue). Long story short, I think it does work, and I tested it with nested interceptors having an await before calling Proceed. Then I compared Castle's original AbstractInvocation and my version of @brunoblank 's side by side.... And I think the only real difference between the two is the fact that @brunoblank uses Enumerator instead of an AbstractInvocation's index to go over the chain of interceptors - and so in effect @brunoblank logic never decrements the index back as each consecutive interceptor is done with its job and is ready to return up the stack. So my question: wouldn't it be sufficient to remove the following code to fix AbstractInvocation?
If I'm missing something here, can someone explain? |
@zvolkov I am glad the workaround work for you. First off, there is a problem with the workaround as it does not detect if there are multiple calls to the I think the real reason why the workaround works and not AbstractInvocation is that the To implement this fix in Core would be a breaking change as IInterceptor.Proceed is a void method. If the change was made to return I have pushed a fix for if (ReferenceEquals(invocation.InvocationTarget, invocation.Proxy))
{
throw new InvalidOperationException(
"This is a DynamicProxy2 error: invocation.Proceed() has been called more times than expected." +
"This usually signifies a bug in the calling code. Make sure that the last interceptor" +
" selected for the method '" + Method + "'" +
" calls invocation.Proceed() at most once.");
} |
Hey @brunoblank thanks for getting back. As for detecting multiple calls to Proceed, I do not believe it is framework's responsibility to enforce the proper use of design. I'm fine with having this responsibility lie on developer, as long as the restriction is clearly documented. As for overwriting ReturnValue, I think I disagree with your assessment both conceptually and based on my real testing. Conceptually, an await operator operates on an object (IAwaitable or really anything that implements GetAwaiter() method or extension method) - not on an expression (inv.ReturnValue). So once the await has evaluated the expression and got its object, even if the value of the expression changes (as the nested interceptors overwrite ReturnValue) the await will still "look" at the original object it "saw" the first time. This is the behavior I see in my real testing, with nested interceptors updating ReturnValue to their respective Tasks, as they bubble up the stack, and I'm 99% sure it works correctly even after the innermost task completes and the chain of continuations "magically" continues with the next line after the innermost await. Each continuation completes its own Task which is what the next outer await "remembers" and "looks at" since it saw it the first time. In short, I believe Castle's separation between CreateClassProxy remains a problem (as does any method that creates InheritanceInvocation instead of CompositionInvocation). It's nice that you submitted a fix to detect the infinite loop and throw a proper exception. Of course, it would be much nicer if Castle's multiple implementations of IInvocation.InvocationTarget did not violate Liskov Substitution Principle and returned the ultimate Target regardless of whether it's Inheritcance- or Composition-style proxy. Something for the core developers to think about, I guess. |
@zvolkov good analyze! I have reviewed and I agree with you regarding the I have done some testing with removing the This makes the straight forward async case work, but then there is an unit-test failing: I checked the test and realized that the workaround does not actually work the same way and has to be rewritten a bit. I think it is best explained with this example. My initial though on this is that it could possibly be solved using a recursive method keeping track of the "next in line" instead of a counting up / down the I will post an update after once I have had time to do some implementation and testing. |
I have now updated the workaround to be "recursive style" commit. My testing so far indicates it works for async and also calling proceed multiple times invoke the expected interceptor/target. I have also removed the IAsyncInvokation and IAsyncInterceptor as per @zvolkov insights that the This should hopefully give the core developers and god idea on how to implement a fix in Core. I also agree with @zvolkov:
|
@brunoblank you are completely right about calling Proceed multiple times in case of multiple retries happening inside a resilient interceptor. It is the scenario that decrementing the interceptor index was meant to address. I see your recursive logic instantiates new Invocation for every level of nesting. That's a nice way to keep track of the index, with zero changes to the user API (IInvocation/IInterceptor) but I'm not sure about performance impact of creating multiple instances of Invocation. It would be nice to find a way to support the interceptor retries without instantiating a separate Invocation object for each level of interception nesting and with minimal changes to the IInvocation/IInterceptor API... Here is one idea I have:
basically, add a special overload of Proceed that restarts the invocation chain from a given interceptor. This would be called by those interceptors that need to await/invoke Proceed multiple times. |
@zvolkov I like your idea and was thinking in the same direction. That would be a breaking change and that would maybe be something for the next major release.
I totally agree, I don't think this is the solution more like giving some ideas on how to adress it. I have been looking at how asp.net core implements middleware which is working very similar comparing I think there are nice features that could be implemented if the var proxy = ProxyGenerator
.CreateClassProxyWithTarget(new TargetClass())
.Use(async (invocation, next) =>
{
Console.WriteLine("-- before --");
await next();
Console.WriteLine("--- after ---");
})
.UseInterceptor<MyOtherInterceptor>()
.Build(); public class MyOtherInterceptor
{
private readonly RequestDelegate _next;
public MyOtherInterceptor(RequestDelegate next)
{
_next = next ?? throw new ArgumentNullException(nameof(next));
}
public async Task Intercept(IInvocation invocation)
{
Console.WriteLine("--- before ---");
await _next(invocation);
Console.WriteLine("--- after ---");
}
} |
@zvolkov I have updated the workaround project with parts of the changes discussed above (commit). @jonorossi please have a look at the workaround example, I think it would be fairly straight forward to implement this in Core, however this is a breaking change ( public delegate void InvocationDelegate(IInvocation invocation);
public interface IInterceptor
{
void Intercept(IInvocation invocation, InvocationDelegate proceed);
} |
I created an example PR #337 last year to demonstrate the issue (see my comment above). I've just rebased it off master and incorporated @brunoblank PR #428 to see if the changes address my example. It does, great work @brunoblank 👍 see my new PR #429 for the results. |
Many thanks guys for continuing to work on this, sorry for the silence on my end. Great to see the changes are quite minimal, however this breaking change scares me because We'd need to either support this callback I know I've said earlier in this thread that I'd prefer not to create an Thoughts? Opinions? |
Good feedback @jonorossi, I understand and share your concerns regarding changing the I tried the wrapper approach first but got issues with I also got into some tricky parts that I did not solve for the create new interceptor per One enabling approach could be to make the |
I've studied this issue and looked at some linked code, however without being able to try things out right now, so please apologize if I still misunderstand. It appears that the one single problem to be solved in DP was, and still is, the statefulness of You have discussed various workarounds around this central issue, e.g. building a chain of several distinct invocation objects instead of just one, or giving What I am wondering, without having studied this completely through, is whether it would be possible to extract This would relieve GC pressure, but still be a breaking change, so as @jonorossi said it would possibly have to be done side-by-side to the existing API. Given interceptors that support some new This is just an idea off the top of my head, I'll think about this some more ASAP. |
@stakx yes, that's a good summary. One thing worth adding, which is solved by the proposal by @brunoblank, is the This only manifests as an issue if the interceptor executes asynchronously before calling The idea of creating a side-by-side implementation seems like a reasonable compromise given the constraints. Something like @brunoblank's implementation could work, though I like the idea of changing public interface IInvocation2
{
// All the same properties and methods as IInvocation except ...
// Remove ReturnValue.
//object ReturnValue { get; set; }
// Change Proceed to return the return value, or null for void methods.
object Proceed();
} public interface IInterceptor2
{
// Intercept now needs to return the return value.
object Intercept(IInvocation2 invocation);
} A no-op implementation of IInterceptor2 could be like this: public class NoOpInterceptor : IInterceptor2
{
public object Intercept(IInvocation2 invocation)
{
return invocation.Proceed();
}
} This would allow an interceptor to do something like this. public class PointlessInterceptor : IInterceptor2
{
public object Intercept(IInvocation2 invocation)
{
return JustToBeAsync(invocation);
}
public async Task JustToBeAsync(IInvocation2 invocation)
{
await Task.Yield();
// Assume the return value is a Task.
Task realReturnValue = (Task) invocation.Proceed();
await realReturnValue;
await Task.Yield();
}
} What do you think? |
It's still too early for me to make any definite judgement on concrete new APIs. For now, I just want to get a full understanding of the problem(s) that we're trying to solve... so thank you for reminding me of the return value aspect.
That being said, pulling the return value out of As soon as I find a good chunk of thinking time, I'll check out @brunoblank's proposal & the other open PR to better understand how you guys arrived at this solution. |
TL;DR:We don't need to make breaking changes to DynamicProxy's API to enable async interceptors. The only thing stopping us appears to be the Do we need an
|
Many thanks @stakx for pushing ahead on this one. I agree we need to fix the I'll add my comments to #439. |
Is AsyncInterceptor ready? |
Are async interceptors in the pipeline for this library?
This would be such a good feature, as it would allow for async actions before the invocation is proceeded.
I would not mind contributing to this, as long as I could get some architecture advice.
The text was updated successfully, but these errors were encountered: