IAsyncInterceptor with return values #40

brunoblank · 2018-02-19T22:54:07Z

Make IAsyncInterceptor easier use.

Implementation can be async directly.
await can be used on invocation.Proceed() as it returns the value including Task support

…ll be propageted all the way up to the caller.

codecov · 2018-02-19T23:11:07Z

Codecov Report

Merging #40 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master    #40   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           5     10    +5     
  Lines         229    254   +25     
  Branches        7      4    -3     
=====================================
+ Hits          229    254   +25

Impacted Files	Coverage Δ
...le.Core.AsyncInterceptor/AsyncTimingInterceptor.cs	`100% <ø> (ø)`	⬆️
....Core.AsyncInterceptor/ProxyGeneratorExtensions.cs	`100% <100%> (ø)`	⬆️
...Interceptor/Invocations/AsyncFunctionInvocation.cs	`100% <100%> (ø)`
...ore.AsyncInterceptor/Invocations/InvocationBase.cs	`100% <100%> (ø)`
...AsyncInterceptor/Invocations/FunctionInvocation.cs	`100% <100%> (ø)`
...stle.Core.AsyncInterceptor/AsyncInterceptorBase.cs	`100% <100%> (ø)`	⬆️
...ore.AsyncInterceptor/ProcessingAsyncInterceptor.cs	`100% <100%> (ø)`	⬆️
....AsyncInterceptor/AsyncDeterminationInterceptor.cs	`100% <100%> (ø)`	⬆️
...e.AsyncInterceptor/Invocations/ActionInvocation.cs	`100% <100%> (ø)`
...ncInterceptor/Invocations/AsyncActionInvocation.cs	`100% <100%> (ø)`
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b4fd89f...5a5ad5e. Read the comment docs.

brunoblank · 2018-02-20T08:11:38Z

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.

@JSkimming take your time, I have no intention of stressing this through. All help / thoughts and feedback are welcome!

I realized the purpose of AsyncInterceptorBase as a bridge between the synch to asynch. I will look into adding this functionality back.

CompletedInvocation I am still a bit uncertain about the intention with this one. Are both of the virtual method supposed to be there?

Looking at the code-coverage, Adding a tests for InvocationBase will bring coverage back to around 100%

JSkimming · 2018-02-20T08:25:08Z

@brunoblank Thanks for this.

Looking at the code-coverage, Adding a tests for InvocationBase will bring coverage back to around 100%

Yes, it would be good to get the coverage back up, and while it was 100% I know it is sometimes unachievable, though the higher the better.

alex-training · 2018-02-20T10:27:10Z

src/Castle.Core.AsyncInterceptor/InterceptorAdapter.cs

+        /// <param name="invocation">The method invocation.</param>
+        public void Intercept(IInvocation invocation)
+        {
+            try


Personally previously (with GetMethodType helper and MethodType enum) it was easier to read this method

Yes, that makes sense. Good call

alex-training · 2018-02-20T10:29:47Z

src/Castle.Core.AsyncInterceptor/InterceptorAdapter.cs

+                }
+                else if (typeof(Task).IsAssignableFrom(invocation.Method.ReturnType))
+                {
+                    invocation.ReturnValue = InterceptAsyncFunctionMethodInfo


Is there any reason to remove the "concurrent dictionary" implementation based on method delegates?

No, I was under the assumption that performance cost was Getting the MethodInfo and not calling Invoke. After doing a few benchmarks it was clear that the AsyncDeterminationInterceptor was superior in performance.

Reverted back to original implementation with the addition of Function handling

alex-training · 2018-02-20T10:32:11Z

src/Castle.Core.AsyncInterceptor/InterceptorAdapter.cs

+                }
+                else
+                {
+                    invocation.ReturnValue = InterceptFunctionMethodInfo.MakeGenericMethod(invocation.Method.ReturnType)


The same issue with reflection

some info regarding invoking methods performance penalties
https://codeblog.jonskeet.uk/2008/08/09/making-reflection-fly-and-exploring-delegates/

Thank you for the link! Reverted back to AsyncDeterminationInterceptor

alex-training · 2018-02-20T10:51:01Z

src/Castle.Core.AsyncInterceptor/Invocations/InvocationBase.cs

+        /// </param>
+        protected InvocationBase(IInvocation invocation)
+        {
+            Invocation = invocation;


Don't you want to implement the Proceed method as well?
Here I see the mix of IInvocation and IAsyncInvocation methods

Good question.

I have intentionally not added Proceed and ReturnValue to IAsyncInvocation as it should not be visible to the Interceptor.

The return-value handling and proceed is implemented separately in each pair of

class FunctionInvocation<TResult> : InvocationBase, IFunctionInvocation<TResult>

The InvocationBase class basically wraps IInvocation from Castle.

alex-training · 2018-02-20T10:56:24Z

src/Castle.Core.AsyncInterceptor/IAsyncActionInvocation.cs

+        ///   <see cref = "NotImplementedException" /> will be thrown.
+        /// </remarks>
+        /// <returns>The asynchronous task</returns>
+        Task Proceed();


What about naming ProceedAsync?

Personally, I don't like to have the Async suffix.

But it is important to express intent and since the synchronous version is called Proceed is a good idea to change to ProceedAsync to be extra clear.

alex-training · 2018-02-20T10:56:32Z

src/Castle.Core.AsyncInterceptor/IAsyncFunctionInvocation.cs

+        ///   <see cref = "NotImplementedException" /> will be thrown.
+        /// </remarks>
+        /// <returns>The return value of the intercepted call</returns>
+        Task<TResult> Proceed();


What about naming ProceedAsync?

Personally, I don't like to have the Async suffix.

But it is important to express intent and since the synchronous version is called Proceed is a good idea to change to ProceedAsync to be extra clear.

…nous to Action / Function differentiation

brunoblank · 2018-02-20T22:52:11Z

Thanks @alex-training for good comments. I have updated PR.

The AsyncInterceptorBase and CompletedInvocation still needs work through as I was focusing on the review comments.

brunoblank · 2018-02-21T07:13:00Z

I did a small comparison benchmark comparing MethodInfo.Invoke and Create.Delegate

                   Method |         Mean |      Error |     StdDev |
------------------------- |-------------:|-----------:|-----------:|
                 Baseline |     3.691 ns |  0.0704 ns |  0.0588 ns |
         MethodInvokation | 1,331.831 ns | 12.1414 ns | 10.1386 ns |
   StaticMethodInvokation | 1,210.952 ns | 12.5258 ns | 11.7166 ns |
           CreateDelegate | 3,527.390 ns | 35.4130 ns | 31.3927 ns |
       DictionaryDelegate |    37.906 ns |  0.4435 ns |  0.4149 ns |
           StaticDelegate |     7.989 ns |  0.1237 ns |  0.1097 ns |

Baseline is using the Generic approach directly. The DictionaryDelegate is using the ConcurrentDictinary approach implemented now. The StaticDelegate version is just to compare the dictionary lookup cost.

JSkimming · 2018-02-21T07:13:39Z

@alex-training thanks for joining in. It's great to get further input. I'll start weighing in soon.

I've never quite known whether this library is receiving much usage, the engagement of others is rather heartening.

I'm open to wider administration such that other interested parties can join in with merging pull requests and pushing to NuGet. If people are interested, let's discuss that in another thread.

alex-training · 2018-02-21T12:37:07Z

@brunoblank , @JSkimming
Thanks for your efforts and contribution.
I still hope the Castle guys will add support of async/await to the Core ...

… tests

JSkimming · 2018-02-23T08:11:32Z

@brunoblank I've just had a look and made a few minor changes. I've restored the intent of the WhenExceptionInterceptingAnAsynchronousMethodThatThrowsASynchronousException test.

I've added a comment stating the intent of the test, which remains the same, what has changed is the behaviour of async interception. Hence, I've changed the tests to reflect the new behaviour rather the class being intercepted.

So far I'm liking the changes 👍

brunoblank · 2018-02-23T10:51:18Z

Nice, I understand the intent of the WhenExceptionInterceptingAnAsynchronousMethodThatThrowsASynchronousException test. Thanks for clarifying.

I am still a bit confused about the CompletedInvocation in ProcessingAsyncInterceptor. Where the with returnValue is calling the one without. Is two methods needed? Also have you considered making them Task based?

The ProxyGeneratorExtensions class is missing the CreateInterfaceProxyWithoutTarget from Castle. Should I add them?

JSkimming · 2018-02-26T07:19:17Z

@brunoblank Thanks for that last commit. I'm afraid my weekend was busier than expected and I've not been able to devote much time to this. I will endeavour to review it soon.

brunoblank · 2018-03-11T20:45:13Z

Hi @JSkimming, how are things looking at your end?

JSkimming · 2018-03-11T22:04:26Z

@brunoblank rather hectic. We have a new product going live on Tuesday and I've not been able to devote any free time to this in recent weeks.

I hope to get some time next weekend.

brunoblank · 2019-04-09T21:54:30Z

@JSkimming I have spent some time looking at this feature again. While troubleshooting the problem in core and understand more of what the problem was. I think this PR solves a few of the problems. But I am not happy with it. There are some key benefits here, the key thing is the return-values in IAsyncInterceptor.

Then, after the discussions regarding Task.Run().GetAwaiter().GetResult() problem in AsyncInterceptorBase. I think the AsyncInterceptorBase should be deleted. But, it brings nice abstract methods.

        protected abstract Task<TResult> InterceptAsync<TResult>(
            IInvocation invocation,
            IInvocationProceedInfo proceedInfo,
            Func<IInvocation, IInvocationProceedInfo, Task<TResult>> proceed);

My suggestion is to update the IAsyncInterceptor to have that syntax (the proceedInfo can be abstracted away to make it easier for the users). So it would look something like this:

public interface IAsyncInterceptor
{
    void InterceptAction(IAsyncInvocation invocation, Action proceed);

    TResult InterceptFunction<TResult>(IAsyncInvocation invocation, Func<TResult> proceed);

    Task InterceptAsyncAction(IAsyncInvocation invocation, Func<Task> proceed);

    Task<TResult> InterceptAsyncFunction<TResult>(IAsyncInvocation invocation, Func<Task<TResult>> proceed);
}

JSkimming · 2019-04-09T23:54:14Z

@brunoblank I've brought this discussion, along with a couple of other's into issue #60, I'd like to see if a slightly broader discussion can be had first, though that doesn't mean we can't come back to this later

Bruno added 4 commits February 19, 2018 22:21

Updated IAsyncInterceptor and updated surounding code.

10a8905

Fix test with faulted task. If throw is done without any await, it wi…

3c26a76

…ll be propageted all the way up to the caller.

Added the synchronous versions for the TestAsyncInterceptorBase

315576f

Added CompleteInvocation for 'void' methods

115fbf4

brunoblank mentioned this pull request Feb 19, 2018

Breaking Change: Simplified interceptor implementation #39

Closed

reverted change to .net471 for test project

f7fbe4b

JSkimming self-assigned this Feb 20, 2018

JSkimming self-requested a review February 20, 2018 08:21

JSkimming added the enhancement label Feb 20, 2018

alex-training reviewed Feb 20, 2018

View reviewed changes

Bruno added 3 commits February 20, 2018 23:01

Changed back to AsyncDeterminationInterceptor, with change of synchro…

420b596

…nous to Action / Function differentiation

Added unit-tests for InvocationBase

b8d9586

Renamed Proceed to ProceedAsync to show intent

6321dc6

Re-implemented the AsyncInterceptorBase functionality

2e8692e

JSkimming added 3 commits February 23, 2018 07:16

Deleted temporary merge files and fixed typo in test class name

599991e

Styling and code reordering to make differences easier to identify

f53ec11

Restored the intention of the sync exception thrown from async method…

2aea5c9

… tests

Added Proxy Generator Extension for: CreateInterfaceProxyWithoutTarget

5af4798

updated castle-core and used capture-proceed-info

5a5ad5e

JSkimming mentioned this pull request Apr 9, 2019

Discussion: Directions to take async interception #60

Open

IAsyncInterceptor with return values #40

Are you sure you want to change the base?

IAsyncInterceptor with return values #40

Uh oh!

Conversation

brunoblank commented Feb 19, 2018

Uh oh!

codecov bot commented Feb 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

brunoblank commented Feb 20, 2018

Uh oh!

JSkimming commented Feb 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brunoblank commented Feb 20, 2018

Uh oh!

brunoblank commented Feb 21, 2018

Uh oh!

JSkimming commented Feb 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alex-training commented Feb 21, 2018

Uh oh!

JSkimming commented Feb 23, 2018

Uh oh!

brunoblank commented Feb 23, 2018

Uh oh!

JSkimming commented Feb 26, 2018

Uh oh!

brunoblank commented Mar 11, 2018

Uh oh!

JSkimming commented Mar 11, 2018

Uh oh!

brunoblank commented Apr 9, 2019

Uh oh!

JSkimming commented Apr 9, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Feb 19, 2018 •

edited

Loading

JSkimming commented Feb 21, 2018 •

edited

Loading