ArxOne / MrAdvice

.NET aspect weaver (build task under NuGet package)
MIT License
311 stars 45 forks source link

Try Catch aspect on async void method doesn't work #119

Open hhblaze opened 7 years ago

hhblaze commented 7 years ago

Hi, we have another problem with try/catch aspect described here

https://github.com/ArxOne/MrAdvice/issues/118

It just doesn't come to catch block in case when unhandeled exception occurs in "async void" method:

 public async void AddWhatever23()
            {
                throw new Exception("");
            }

with public async Task AddWhatever23() - all OK.

Any ideas of handling such case?

picrap commented 7 years ago

This is normal behavior. You're probably out of the advice before the method has ended. Because the method returns nothing (a void), the advice has no way to await it. You probably also notice during build phase that MrAdvice returned a warning regarding weaving async void methods 😉

aliegeni commented 7 years ago

Agree that this is expected behaviour of async void. In general the guideline is to avoid async void except when used in an event handler. Should I avoid 'async void' event handlers? Async/Await - Best Practices

Because method return void and there is nothing what could be awaitable, the only way to catch exception is to do try/catch in the method itself!

So the question is how to use Mr.Advice to apply exception "swallowing" aspect on those methods (inject try/catch) in case when developer forgotten to do it by himself? Is it even possible to inject inside of method or there is only decorating aspect?

picrap commented 7 years ago

This is the reason why Mr.Advice gives a warning when weaving async void methods :stuck_out_tongue:

aliegeni commented 7 years ago

Things gets more complicated when we use async lambdas. Practically it's the same async void problem but more hidden! As we know async lambdas could return void or Task. Consider the following example:

        public bool MyTest()
        {
            try
            {
                DoSomething("Here is a problem", async () =>
                {
                    await Task.Delay(500);
                    throw new Exception("The exception tricky to catch!");
                });
                return true;
            }
            catch (Exception ex)
            {
                Console.WriteLine($"[E] {ex.GetType().Name}: {ex.Message}");
                return false;
            }
        }

        public void DoSomething(string args, Action callBack)
        {
            Console.WriteLine(args);
            callBack();
        }

Despite you put all your code in try/catch in ordinary sync metHode, you never caught that exception, neither Mr.Advice gives you a warning! One must put anothertry/catch block in lambda itself and that's not obvious to see.

picrap commented 7 years ago

Where is your advice applied? Could you post an archive with your test solution here?

aliegeni commented 7 years ago

I'm not consider it as a bug, it just a way it is - there are exceptions we just can't handle in aspect. WsnAspectTest.zip

picrap commented 7 years ago

Tough one! The generated lamba is not a method, hence the non-detection. I tag this as a bug, even though I don't have any idea on how to fix this.

picrap commented 7 years ago

OK, it is actually a method, but I was excluded from weaving because it is compiler-generated and caused problems on release build. Still a bug anyway...

dougcunha commented 6 years ago

I don’t know how they do it, but actually it’s possible to write a Postsharp advice that is able to handle exceptions on async void methods. I’ve just tried and it worked. Maybe by injecting a .ContinueWith or something like that....

picrap commented 6 years ago

Hi @dougcunha, I didn't look at the generated code, however they probably change the code of the advised method in order to make it async Task instead of making it async void, because otherwise there is no way to track its execution.

picrap commented 6 years ago

Also, they have a full team, paid by customers. On the other hand, I'm alone and work for free 😜

hhblaze commented 6 years ago

For now we can live with that by manual code reformatting (probably, once it can be done automatically):

[ExcludePointcut] //or exclude all async voids on the level of assembly
async void X(pars)
{  
   await X_Wrapper(pars); 
} 

//MrAdvice is in full control here
async Task X_Wrapper(pars)
{
 //implementation 
}

Lambdas also must be rewritten...

dougcunha commented 6 years ago

Also, they have a full team, paid by customers. On the other hand, I'm alone and work for free

Sure, I wasn't saying otherwise. Sorry if I sounded rude. My intention was just to point that maybe there's a way to solve this problem. I sincerely appreciate your work. All the best!

dougcunha commented 6 years ago

For now we can leave with that by manual code reformatting (probably, once it can be done automatically):

[ExcludePointcut] //or exclude all async voids on the level of assembly
async void X(pars)
{  
   await X_Wrapper(pars); 
} 

//MrAdvice is in full control here
async Task X_Wrapper(pars)
{
 //implementation 
}

Lambdas also must be rewritten...

I've been doing that, it's the best we can do now. Thank you!

picrap commented 6 years ago

@dougcunha you missed the point in my answer; it was a smiling emoji. So no problem!

PankajRawat333 commented 5 years ago

@picrap Is there any recommended solution/workaround for this problem? exception in Async Method

picrap commented 5 years ago

I replied ;)