Skip to content
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

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

Open
hhblaze opened this issue Sep 1, 2017 · 17 comments
Open

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

hhblaze opened this issue Sep 1, 2017 · 17 comments

Comments

@hhblaze
Copy link

hhblaze commented Sep 1, 2017

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

#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
Copy link
Member

picrap commented Sep 1, 2017

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
Copy link

aliegeni commented Sep 1, 2017

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
Copy link
Member

picrap commented Sep 1, 2017

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

@aliegeni
Copy link

aliegeni commented Sep 2, 2017

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 another try/catch block in lambda itself and that's not obvious to see.

@picrap
Copy link
Member

picrap commented Sep 2, 2017

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

@aliegeni
Copy link

aliegeni commented Sep 2, 2017

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
Copy link
Member

picrap commented Sep 15, 2017

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 picrap added the bug label Sep 15, 2017
@picrap
Copy link
Member

picrap commented Sep 15, 2017

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
Copy link

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
Copy link
Member

picrap commented Oct 31, 2018

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
Copy link
Member

picrap commented Oct 31, 2018

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

@hhblaze
Copy link
Author

hhblaze commented Oct 31, 2018

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
Copy link

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
Copy link

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
Copy link
Member

picrap commented Oct 31, 2018

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

@PankajRawat333
Copy link
Contributor

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

@picrap
Copy link
Member

picrap commented Jan 9, 2019

I replied ;)

@picrap picrap added enhancement and removed bug labels Apr 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants