-
Notifications
You must be signed in to change notification settings - Fork 42
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
Breaking Change: Simplified interceptor implementation #39
Comments
Hi @brunoblank, I'm happy to consider anything, breaking change or not. Is this related to this discussion castleproject/Core#145? Can you submit a PR for review? |
@JSkimming - Have a prototype that helps us find the answer to castleproject/Core#145 over here: https://github.com/fir3pho3nixx/Core/pull/28 Test passes but would love to hear your input. The answer lies in there somewhere unless I got it completely wrong. |
Hi @JSkimming, I am happy to submit a PR. but I need some time to get one together. It does not have anything to do with I have been using Castle.Core for many years and am a big fan of it. You can find one of my projects here: web-api-client. @fir3pho3nixx I had a quick look at your changes and there was alot of code. What I noticed was you removed the |
Did not notice a PR on Castle.Core, did you submit one? I find this problem incredibly interesting.
I agree. Could be cleaned up. However it is interesting to note that @JSkimming PR causes a deadlock and my approach does not. Did you notice my ProceedAsync method here? |
I did not submit a PR on
Yes you are using NitoEx
|
But more specifically.
I help out on Castle and I am trying to understand this.
Invocation.Proceed is a synchronous process. Period. Expand your argument. |
Let me try to explain. There is a huge difference between a synchronous call and a blocking call. public interface IHello { Task World(); }
This is absolutely true. There is no possibility to do anything asynchronous here. To come around this you set the ReturnValue and return (without blocking). It is now up the caller of If you on the other hand do a blocking call on the thread that calls The solution I did was this (for the internal class InterceptorAdapter : IInterceptor
{
private readonly IAsyncInterceptor[] _interceptors;
public InterceptorAdapter(IAsyncInterceptor[] interceptors)
{
_interceptors = interceptors;
}
public void Intercept(IInvocation invocation)
{
invocation.ReturnValue = new AsyncActionInvocation(invocation, _interceptors).Proceed();
}
}
internal class AsyncActionInvocation : InvocationBase, IAsyncActionInvocation
{
public AsyncActionInvocation(IInvocation invocation, IEnumerable<IAsyncInterceptor> interceptors)
: base(invocation, interceptors)
{
}
public Task Proceed()
{
if (Enumerator.MoveNext())
return Enumerator.Current.InterceptAsyncAction(this);
ThrowIfNoInvokationTarget();
return (Task)Invocation.GetConcreteMethodInvocationTarget().Invoke(
Invocation.InvocationTarget,
Invocation.Arguments);
}
} |
@brunoblank Thanks for your explanation. @StephenCleary also left an explanation on my PR not to use AsyncContext either because it does not work in all situations. |
@brunoblank I'm more than happy to wait. If you create a PR from your fork, and ensure I can commit to your forked repo (which is the default behaviour for GitHub) I'll try and join in. @fir3pho3nixx Thanks for getting involved. I've not had a deep look at your PR, but if it's blocking it is the situation I'm trying to avoid. I'm not sure how to resolve this async issue without changing Core, if @brunoblank has an answer that would be awesome as it has thus far eluded @ndrwrbgs and myself. |
I have now pushed a version to: feature-branch This change only contains the change of I had to abandon the async / proceed change as this.Invocation.GetConcreteMethodInvocationTarget().Invoke(
this.Invocation.InvocationTarget,
this.Invocation.Arguments); Caused a loop for There are a handful of tests that are failing now, I have fixed them (not pushed), but I am not sure I fully understand the helper methods. Please review / test it out. @JSkimming you should have write access to the fork. |
@brunoblank Thanks, I've just had a quick look. There's a lot to take in and I may not get a chance to give it a good look until the weekend, though I'll try and have a look sooner. In principle, I'm happy for such extensive changes, it's good to get the input of others, but I do want to get a deep understand, and that may take me a bit of time, which is challenging to fit around the day job. If you can push the fixes for the failing tests that would be great. Also, please do submit a PR, it doesn't have to ready to go, it just gives us something to discuss, and it also kicks off the CI, with all the tests and code coverage metrics. |
I have created a PR #40 Closing this issue here and discussion should continue in on the PR Thanks |
Hi,
Great work!
I have maintained my own class library for this and I believe the
IAsyncInterceptor
could be changed a bit to make usingasync / await
less error prone.Today (the helper function is needed and setting the
ReturnValue
):This could be hidden in the library by changing the
IAsyncInterceptor
to:Where the
IInvocation
interface'sProceed
method matches theReturnType
Now when implementing an interceptor, you would be able to use
async / await
directly.Example:
Is this something you would consider?
Thanks
/Bruno
The text was updated successfully, but these errors were encountered: