Discussion: Directions to take async interception

[JSkimming]

We've had several discussions on issues and pull requests on this repo, and on Castle.Core, and I wanted to bring it together (if possible).

Firstly, I want to thank you guys for getting involved. I imagine we've all got day jobs, so any time we can give to this is fantastic.

Here is my current thinking, it's by no means an exhaustive list, it's just what's top of mind at the moment.

1. The design of AsyncInterceptorBase is a mistake.
   • In attempting to provide just two methods to override, both of which are asynchronous, it has made synchronous interception problematic with the potential of causing deadlocks when transitioning back from the asynchronous interceptor to the underlying synchronous method being intercepted.
   • Here's my comment on PR Use the new Castle.Core CaptureProceedInfo() to make AsyncInterceptor work for reals #54 going into a little more detail.
   • I, therefore, agree with @brunoblank in his comment here on PR IAsyncInterceptor with return values #40, specifically "I think the AsyncInterceptorBase should be deleted. But, it brings nice abstract methods."
2. What to do with return value?
   • I think one of the challenges we've had with async before Proceed has been to do with the return value being out of band and as a result out of our control.
   • I like the idea raised by @brunoblank in PR IAsyncInterceptor with return values #40, and I wonder what other's think.
   • I believe you @stakx think differently, though I can't remember the comment that gave me that impression.
3. Closely align with the design of Castle.Core
   • This is related to the return value discussion since the design of the IInvocaton has the return value as a property.
   • I think it is a benefit to match the design of Castle.Core as it adheres to the Principle of least astonishment and developers familiar with Castle.Core will find it easier to pick up AsyncInterceptor.
   • Close alignment to Castle.Core may improve the chances of this becoming incorporated into Castle.Core. This is something I considered when I first started this library, and I've received comments along that line on other issues. Thoughts @stakx?

The current alpha introduces some breaking changes, and I believe we must introduce more, given point 1. I think now is an excellent time to tackle some other gnarly issues if you guys are happy to get involved?

All views regretfully received.

[brunoblank]

When using the Castle.Core - IInterceptor for synchronous operations is straight forward and works very well. So having the IAsyncInterceptor align with Castle.Core being asynchronous would look like this

note: that there must be two separate methods for synchronous vs asynchronous as we do not want to introduce sync-over-async or async-over-sync problems

public interface IAsyncInterceptor
{
  void Intercept(IInvocation invocation);
  Task InterceptAsync(IInvocation invocation);
}

However, when trying to use the same concept for asynchronous is hard. Accessing the ReturnValue and casting it is problematic as Task<object> is not compatible with Task<string>. This could be solved by adding a generic based asynchronous "overload".

public interface IAsyncInterceptor
{
  void Intercept(IInvocation invocation);
  Task InterceptAsync(IInvocation invocation);
  Task InterceptAsync<TResult>(IInvocation invocation);
}

So when implementing an interceptor based on this interface (not addressing the async before proceed problem) is when can/should invocation.ReturnValue be read/written. Calling Proceed in the asynchronous case must read the return-value and await and the method must also set return-value before doing any await in order not to break.

I think this should be handled by AsyncInterceptor. This can be solved by taking care of return-values which would make this IAsyncInterceptor look like this:

public interface IAsyncInterceptor
{
    void Intercept(IInvocation invocation);
    TResult Intercept<TResult>(IInvocation invocation);
    Task InterceptAsync(IInvocation invocation);
    Task<TResult> InterceptAsync<TResult>(IInvocation invocation);
}

From the discussions in PR 428 - castle.core.

Personally, I think that moving Proceed out of IInvocation might be a good thing, as it arguably improves the abstraction of the latter: IInvocation would then (more than now) describe an invocation without tying it to the interception pipeline that processes it. This would arguably fit its name better. So far, so good.

Now we make break out the Proceed usage to own method (also: handling the CaptureProceedInfo in library).

public interface IAsyncInterceptor
{
    void Intercept(IInvocation invocation, Action proceed);
    TResult Intercept<TResult>(IInvocation invocation, Func<TResult> proceed);
    Task InterceptAsync(IInvocation invocation, Func<Task> proceed);
    Task<TResult> InterceptAsync<TResult>(IInvocation invocation, Func<Task<TResult>> proceed);
}

The the last touch is to make an IAsyncInvocation that hides Proceed and ReturnValue from IInvocation just to remove possibility to use it the wrong way.

public interface IAsyncInterceptor
{
    void Intercept(IAsyncInvocation invocation, Action proceed);
    TResult Intercept<TResult>(IAsyncInvocation invocation, Func<TResult> proceed);
    Task InterceptAsync(IAsyncInvocation invocation, Func<Task> proceed);
    Task<TResult> InterceptAsync<TResult>(IAsyncInvocation invocation, Func<Task<TResult>> proceed);
}

[stakx]

I agree that it would be nice to make your library have a very similar API surface to Castle's.

@brunoblank's design comes close to how I would model these interfaces. In addition, I would like to suggest the following:

• Drop the proceed parameter. If you're going to have an IAsyncInvocation anyway, just let it have an awaitable method Task ProceedAsync(). Its meaning is the same as in the sync case: execute the rest of the interception pipeline; only here we can await the moment when interception gets back to us.

• Instead of IAsyncInterception, define a very slim AsyncInterception value type wrapper/adapter around IInvocation. That wrapper's main responsibilities are (a) to remove "dangerous" methods like Proceed or SetArgumentValue; (b) have its own ReturnValue buffer that gets extracted from/applied to the underlying invocation's Task-typed ReturnValue (which will get overwritten all the time, messy that); and (c) have properties/methods for everything else in IInvocation that makes sense in an async context and simple delegate to the underlying IInvocation.

• Forget the IAsyncInterceptor interface. Just have your abstract AsyncInterceptor base class with some protected abstract members. The main value of your library is that it provides a base implementation of IInterceptor.Intercept that deals with the ugly details that noone will want to implement again from scratch (which they'd have to do when implementing an interface).

[stakx]

Btw. thank you @JSkimming for asking for my opinion... I appreciate it, even though I have hardly been involved in this project up until now. I am happy to draw up a more detailed draft similar to how @brunoblank did above sometime during the weekend, if that would be deemed welcome/useful.

[JSkimming]

Thanks for the comments @brunoblank & @stakx.

thank you @JSkimming for asking for my opinion... I appreciate it

The more, the merrier 😃

I am happy to draw up a more detailed draft similar to how @brunoblank did above sometime during the weekend, if that would be deemed welcome/useful.

Yes please 👍

I've read both of your detailed comments, but I don't have the head-space to think things through at the moment, I'll try and give it some time this weekend, though I've tried and failed to meet that commitment in the past. 🤷‍♂️

I have given some thought on what I'd like to achieve.

I want to provide a layering of features, where consumers of this library can take as much or as little as they like.

So at the lowest level something like the AsyncDeterminationInterceptor, (I don't like the name, but still can't think of anything better) where it adds very little overhead to a pure Castle.Core implementation. AsyncDeterminationInterceptor pretty much does nothing more than solve the problem of the SO question that started this all off.

Then add further functionality on top of that, to make implementing common scenarios easier. For instance AsyncTimingInterceptor, which was a problem I'd solve several times on different projects by lifting the same code.

Also, one thing I'd like to avoid, which is why I want to remove AsyncInterceptorBase, is having a detrimental effect on implementing synchronous interception within the same interceptor.

Thanks again for the input.

[JSkimming]

Also, @stakx if you're interested, I'd be happy to have you join the collaborators.

[stakx]

Also, @stakx if you're interested, I'd be happy to have you join the collaborators.

@JSkimming, thank you very much for the offer. Interested, always (I love technical riddles. 😃), right now I hardly know this project's code base, so I suggest I'll start the humble way and simply submit PRs if I see something worth doing. Once I'm more familiar with the project, I'll be happy to join!

In the meantime, let me just put out an idea or two.

I am happy to draw up a more detailed draft similar to how @brunoblank did above [...].

What follows in the code block below is an illustration of what I suggested above.

If I were to build an async interceptor base class, and I wanted to stay as close as possible to DynamicProxy's API, this is how I would probably do it:

// No `IAsyncInterceptor` interface; users of your library are using it precisely
// because they do not want to implement async interception themselves!
public abstract partial class AsyncInterceptor : IInterceptor
{
    protected AsyncInterceptor() { }

    // Those must be implemented by users. Note the lack of an `InterceptAsync`
    // method returning a `Task<T>`. Interceptors do not return a return value
    // (as there may not be one in every case); this holds for both the sync and
    // async case. 
    protected abstract void Intercept(IInvocation invocation);
    protected abstract Task InterceptAsync(AsyncInvocation invocation);

    // Implemented by the base class. The async machinery here looks at the
    // method and decides which of the above methods to delegate to. 
    void IInterceptor.Intercept(IInvocation invocation) { ... }
}

// Note that this does not implement Core's `IInvocation`, which would be
// bad as it would inherit "dangerous" members like `Proceed` (which may
// malfunction in the async case), `ReturnValue` (which might not take any
// effect in the calling code), `SetArgumentValue` (by-ref arguments don't
// play well with `async/await`), etc. Instead, it *wraps* an `IInvocation`!
//
// This type could possibly be hidden behind an `IAsyncInvocation` interface,
// or be made abstract, although the benefits of that are perhaps small.
public sealed partial class AsyncInvocation
{
    private readonly IInvocation invocation;

    // Note the `readonly`. Should be captured in the ctor before any async.
    private readonly IInvocationProceedInfo proceed;

    internal AsyncInvocation(IInvocation invocation) { ... }

    // Have properties/methods that delegate to the wrapped invocation
    // when it is safe to do so:
    public MethodInfo Method => this.invocation.Method;
    public object Proxy => this.invocation.Proxy;
    ...

    // Convert `object[] Arguments` into a read-only collection, like is the
    // case with the underlying `ReturnValue`, modifying by-ref `Arguments`
    // will have no effect after the first `await`.
    public IReadOnlyList<object> Arguments => this.invocation.Arguments;

    // A return value buffer of its own. This is to guarantee that each
    // invocation gets its own `Result` preserved and not constantly
    // overwritten. This also means we don't need to constantly convert
    // between `T` and `Task<T>` when reading from / writing to the underlying
    // `ReturnValue`.
    //
    // The semantics here are that for an intercepted method with a return
    // type of `Task<T>`, this property would hold a value of type `T`. (Therefore,
    // setting this for a method returning `Task` should throw an exception.)
    // That's also the reason why it's called `Result`, not `ReturnValue`.
    public object Result { get; set; }

    // Replaces the `Proceed` method. Takes care to store the underlying
    // `ReturnValue` before proceeding, then extracting the new return value
    // after proceeding and storing it in its own `Result` (above), and
    // finally restoring the original underlying `ReturnValue`.
    public Task ProceedAsync() { ... }
}

See this Gist for a working (but simplistic) implementation.

Additional abstractions may be added inside the async machinery, e.g. if you want it to be able to deal with awaitable types other than the framework's own Task, Task<T> and ValueTask<T>. You'd perhaps need to build a registry of "awaitable type handlers" that can recognize their being responsible for some return type, and provide for it a "completion source" (think TaskCompletionSource<T> but for types other than Task<T>) for the async interceptor to use. Such a registry would then be injected into AsyncInterceptor (with a sensible default provided out of the box).

(Not sure how all of this can be done without adding a ton of complexity and runtime overhead, so perhaps I'd have several base classes of AsyncInterceptor: TaskAsyncInterceptor, which would be specialized for the BCL awaitable types, and AwaitableAsyncInterceptor which would have the more flexible registry approach.)

I want to provide a layering of features, where consumers of this library can take as much or as little as they like.

I suppose facilities provided by the library might not all build on top of the same basis. For example, would you build a timing interceptor on top of IInterceptor, TaskAsyncInterceptor, or AwaitableAsyncInterceptor? Depending on your choice, it'll be more or less limited with respect to the supported return types.

DynamicProxy's solution to this problem would perhaps be to separate different concerns into separate interceptors and compose them into a pipeline. That is, solve the problem through composition, not inheritance. It should be possible to freely mix and match AsyncInterceptor with any other IInterceptor. (Incidentally, this reveals a flaw in my above Gist, namely that my ProceedAsync implementation assumes that later interceptors always set a return value, which isn't necessarily true.)

[brunoblank]

Good breakdown @stakx!

Personally I really dislike partial classes from external libraries and would prefer an interface.

Having the object Result as a property has some pros and cons.

The major pro is that if the interceptor isn't interested in the actual value then it does not have to do anything with it.
The major con is that if the value is needed then the user has to MethodInfo.Invoke to be able to use generics. In this case it is a bit unclear if the Result is the actual T or a Task<T>.

I think the return value approach is easier to understand and work with.

The thin AsyncInvocation class is nice.

You'd perhaps need to build a registry of "awaitable type handlers" that can recognize their being responsible for some return type.

Yes agree, this is definitely something to investigate.

I am throwing an idea into the mix here to see what you think.

One "scary" part of this is the rather large surface to touch to cover everything in the IAsyncInterceptor (many methods to implement). Lets say there are three interfaces, IInterceptor, IAsyncActionInterceptor, IAsyncFuncInterceptor (disregard the names), which only defines the appropriate method.
The difference would be that only the matching interceptor would be invoked.

[stakx]

Hmm @brunoblank, I think for once I cannot agree. (Or I may simply misunderstand what you wrote.)

Personally I really dislike partial classes from external libraries and would prefer an interface.

There's no such thing as a "partial class from external libraries". partial only means that you can split a class across several source code files, and that these separate definitions will be merged back together at compile time. It's purely a C# language / source code construct. partial does not exist at the metadata / assembly level.

(I only used the partial keyword above to hint that I may have omitted some members not crucial to an understanding of my proposal. But you can disregard that keyword if you prefer.)

I think the return value approach is easier to understand and work with.

... except that the problems you described for a Result property don't go away when return-ing instead. On the contrary, they get worse: Now every interceptor has to return something, even when they don't want to. And if the return type is object, you still don't know whether you should return a Task<T> or just a T. Since you cannot name the return value parameter of a method, you've lost the ability to name the output "slot" to give a hint. Using a property OTOH gives you that opportunity: Result (as opposed to ReturnValue) is a hint at Task<T>.Result`, i.e. you should set it to the task's result, not set it to a new task wrapping such a result.

Granted, in the async case, you usually do need to return a Task or Task<TResult> instance anyway. (The async void case could be debated.) But the "what to return if you don't want to update the return value" problem still persists.

Both approaches have their pros and cons. My suggestion was simply to stay close to how DynamicProxy works.

the rather large surface to touch to cover everything in the IAsyncInterceptor (many methods to implement)

Are you talking of present-day IAsyncInterceptor, or AsyncInterceptor from my post? If the latter, I'm not sure I understand. Two methods (one for sync, one for async) are a large surface area?

Lets say there are three interfaces, IInterceptor, IAsyncActionInterceptor, IAsyncFuncInterceptor (disregard the names), which only defines the appropriate method. The difference would be that only the matching interceptor would be invoked.

Say you write an interceptor that performs an async log operation. It only wants to log the invocation; it never wants to return a value. You now have to write the same identical interceptor at least twice, once for void methods (actually, for Task-returning methods), and once for non-void methods (or rather, Task<TResult>-returning ones). If your proxied type has methods of both categories, then you'll have to have both of these identical interceptors in the interception pipeline to cover all of the type's methods. That scheme sounds supremely impractical to me, to be honest. (DynamicProxy solves this by not forcing interceptors to do anything at all, they can be no-ops. Let each interceptor inspect the current invocation and decide what's the appropriate action to take.)

[brunoblank]

There's no such thing as a "partial class from external libraries". partial only means that you can split a class across several source code files, and that these separate definitions will be merged back together at compile time. It's purely a C# language / source code construct. partial does not exist at the metadata / assembly level.

Yes you are right. I was going to write I dislike partial classes as a concept and avoid using them, and I prefer interfaces from external libraries instead of classes. Thanks for pointing it out (I often don't get what I think into what I write :( )

Lets consider this interface:

public interface IAsyncInterceptor
{
    Task<TResult> InterceptAsync<TResult>(IAsyncInvocation<TResult> invocation);
}

So if have an implementation that proceeds it would look like this:

public class AsyncInterceptor: IAsyncInterceptor
{
    public async Task<TResult> InterceptAsync<TResult>(IAsyncInvocation<TResult> invocation)
    {
       ...
       TResult result = await invocation.Proceed();
       ...
       return result;
    }
}

And yes you would be required to capture and return the result from the inner proceed. And if you don't want to proceed you would have to return something, and returning default(T) might not seem obvious.

On the other hand with the Result property I would have to do this :

public class MyAsyncInterceptor: AsyncInterceptor 
{
    protected override async Task InterceptAsync(AsyncInvocation invocation)
    {
        ...
        await invocation.Proceed();
        object result = invocation.Result;
        ...
    }
}

Aha, I now understand that the Result property is the unwrapped result (the T from Task<T>). This makes sense if also all the other methods like ReturnType would be unwrapped.

Why not call it ReturnValue as it would not be the IInvocation.ReturnValue anyway?

In the first approach it relies on generics and do the MethodInfo.Invoke under the surface and you get two methods to implement (only considering Task based methods). The second approach only have one method to implement but you have to do the MethodInfo.Invoke yourself (if you need to). However, it still does it under the surface but does not expose it to the user.

I would say the second approach aligns closer with Castle (and leaves the messy MethodInfo.Invoke to the user).

Say you write an interceptor that performs an async log operation. It only wants to log the invocation; it never wants to return a value. You now have to write the same identical interceptor at least twice, once for void methods (actually, for Task-returning methods), and once for non-void methods (or rather, Task-returning ones). If your proxied type has methods of both categories, then you'll have to have both of these identical interceptors in the interception pipeline to cover all of the type's methods. That scheme sounds supremely impractical to me, to be honest.

That implementation would just implement all interfaces.

[brunoblank]

Are you talking of present-day IAsyncInterceptor, or AsyncInterceptor from my post? If the latter, I'm not sure I understand. Two methods (one for sync, one for async) are a large surface area?

That was from my example above - four methods to implement.