Breaking Change: Simplified interceptor implementation

[brunoblank]

Hi,

Great work!

I have maintained my own class library for this and I believe the IAsyncInterceptor could be changed a bit to make using async / await less error prone.

Today (the helper function is needed and setting the ReturnValue):

public void InterceptAsynchronous(IInvocation invocation)
{
    invocation.ReturnValue = InternalInterceptAsynchronous(invocation);
}

private async Task InternalInterceptAsynchronous(IInvocation invocation)
{
    // Step 1. Do something prior to invocation.

    invocation.Proceed();
    var task = (Task)invocation.ReturnValue;
    await task;

    // Step 2. Do something after invocation.
}

This could be hidden in the library by changing the IAsyncInterceptor to:

    public interface IAsyncInterceptor
    {
        void InterceptAction(IActionInvocation invocation);

        Task InterceptAsyncAction(IAsyncActionInvocation invocation);

        Task<T> InterceptAsyncFunction<T>(IAsyncFunctionInvocation<T> invocation);

        T InterceptFunction<T>(IFunctionInvocation<T> invocation);
    }

Where the IInvocation interface's Proceed method matches the ReturnType

    public interface IAsyncFunctionInvocation<T> : IInvocationEx
    {
        Task<T> Proceed();
    }

    public interface IAsyncActionInvocation : IInvocationEx
    {
        Task Proceed();
    }
   ... etc ...

Now when implementing an interceptor, you would be able to use async / await directly.
Example:

    internal class MyAsyncInterceptor : IAsyncInterceptor
    {
        public void InterceptAction(IActionInvocation invocation)
        {
            invocation.Proceed();
        }

        public async Task InterceptAsyncAction(IAsyncActionInvocation invocation)
        {
            await Task.Delay(500);
            await invocation.Proceed();
            await Task.Delay(500);
        }

        public async Task<T> InterceptAsyncFunction<T>(IAsyncFunctionInvocation<T> invocation)
        {
            await Task.Delay(500);
            var rv = await invocation.Proceed();
            Console.WriteLine($"rv: {rv}");
            await Task.Delay(500);
            return rv;
        }

        public T InterceptFunction<T>(IFunctionInvocation<T> invocation)
        {
            return invocation.Proceed();
        }
    }

Is this something you would consider?

Thanks
/Bruno

[JSkimming]

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.

[brunoblank]

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 Core#145, even though I was able to solve the await before Proceed call based on the discussion.

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 await before Proceed can and the Task helpers had a few Wait and .GetAwaiter.GetResult() that will turn into a blocking call, which you want to avoid when going async.

@brunoblank

It does not have anything to do with Core#145, even though I was able to solve the await before Proceed call based on the discussion.

Did not notice a PR on Castle.Core, did you submit one? I find this problem incredibly interesting.

I had a quick look at your changes and there was alot of code. What I noticed was you removed the await before Proceed can and the Task helpers had a few Wait and .GetAwaiter.GetResult() that will turn into a blocking call, which you want to avoid when going async.

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?

[brunoblank]

Did not notice a PR on Castle.Core, did you submit one? I find this problem incredibly interesting.

I did not submit a PR on Castle but was able to fix the issue by implementing the next interceptor logic in one interceptor adapter (which is a workaround until Castle fixes the issue). I will be part of my PR here.

Did you notice my ProceedAsync method here?

Yes you are using NitoEx AsyncContext.Run which will turn into a blocking call and run the logic inside asynchronously.

sealed class AsyncContext : IDisposable
{
  // Queues an action for execution, and begins executing all actions in the queue.
  // This method returns when all actions have been completed and the outstanding asynchronous operation count is zero.
  // Returns the result of the task. This method will unwrap and propagate exceptions.
  public static void Run(Action action);
  public static TResult Run<TResult>(Func<TResult> action);
  public static void Run(Func<Task> action);
  public static TResult Run<TResult>(Func<Task<TResult>> action);
  ...

I did not submit a PR on Castle but was able to fix the issue by implementing the next interceptor logic in one interceptor adapter (which is a workaround until Castle fixes the issue). I will be part of my PR here.

But more specifically.

which is a workaround until Castle fixes the issue

I help out on Castle and I am trying to understand this.

Yes you are using NitoEx AsyncContext.Run which will turn into a blocking call and run the logic inside asynchronously.

Invocation.Proceed is a synchronous process. Period. Expand your argument.

[brunoblank]

Let me try to explain. There is a huge difference between a synchronous call and a blocking call.
If you intercept an asynchronous method:

public interface IHello { Task World(); } 

Invocation.Proceed is a synchronous process. Period. Expand your argument.

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 IHello.World to block on the Task until it is ready or await the result. If an await is done before the Proceed call the bug Core#145 is triggered.

If you on the other hand do a blocking call on the thread that calls Intercept, then the bug is not triggered. But blocking asynchronous call is bad practice Don't block async code

The solution I did was this (for the Task based interception):

  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.

[JSkimming]

Hi @JSkimming, I am happy to submit a PR. but I need some time to get one together.

@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.

[brunoblank]

I have now pushed a version to: feature-branch

This change only contains the change of IAsyncInterceptor.

I had to abandon the async / proceed change as

                this.Invocation.GetConcreteMethodInvocationTarget().Invoke(
                    this.Invocation.InvocationTarget,
                    this.Invocation.Arguments);

Caused a loop for CreateClassProxy<TClass>. Invocation.InvocationTarget was pointing to the generated proxy and not the actual target 😞

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.

[JSkimming]

@brunoblank Thanks, I've just had a quick look. ExceptionDispatchInfo I've not come across that before, looks interesting 👍

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.

[brunoblank]

I have created a PR #40

Closing this issue here and discussion should continue in on the PR

Thanks
/Bruno