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

AsyncAwaitMayBeElidedHighlighting on async void shouldn't appear #28

Open
barishamil opened this issue Feb 22, 2018 · 7 comments
Open

Comments

@barishamil
Copy link

No description provided.

@paulirwin
Copy link

paulirwin commented Mar 22, 2018

So there's nothing wrong with the idea of eliding async/await in async void methods, the problem is that if you do that currently, you get bad syntax because it puts in the return keyword. Example:

        public async void Foo()
        {
            await BarAsync();
        }

        public Task BarAsync()
        {
            return Task.CompletedTask;
        }

Today, if you choose to elide async/await inside Foo, you end up with:

        public void Foo()
        {
            return BarAsync();
        }

Which obviously is a compiler error. When the return type of the method is void, return should be omitted when eliding async/await. i.e.:

        public void Foo()
        {
            BarAsync();
        }

There is no problem in doing it this way for most applications. I just tested it with a WPF app and the Dispatcher still caught an exception fine. It may cause issues with exception handling in others.

Edit: clarified language

@mdpopescu
Copy link

mdpopescu commented Apr 4, 2020

I agree with Paul that - syntactically - his recommendation would work.

However, I believe this shouldn't be suggested at all for async void methods, because it changes the semantics:

        public async void Foo()
        {
            await Task.Delay(TimeSpan.FromSeconds(5));
        }

would now become

        public void Foo()
        {
            Task.Delay(TimeSpan.FromSeconds(5));
        }

which does not, in fact, wait for 5 seconds before exiting the method.

@paulirwin
Copy link

@mdpopescu Good point. Although given that async/await should only be properly elided for the last call in the method, there could not be any code after it anyways (or it would not be elided), so I believe that would not matter. It certainly wouldn't matter much to the caller, because as soon as it gets to await Task.Delay it will return immediately anyways. I might be missing something though, can you come up with an example where it would matter?

@mdpopescu
Copy link

I don't understand what you mean, @paulirwin -- I gave an example where the change you're suggesting (eliding the async/await) would cause issues in an async void method. I actually had that happen in a program - the add-on suggested changing it, I removed the return manually and then realized that the Task.Delay would now be ineffective, changing the meaning of the code.

@paulirwin
Copy link

@mdpopescu async void methods return at the first await, so the first example does not wait 5 seconds before returning to the caller, it returns nearly immediately. Eliding async/await in this example does the same thing, it invokes a new Task and immediately returns to the caller. Neither actually wait 5 seconds before doing their next thing, because there's no next thing to do (no code after the Delay). The delay finishes on another thread after 5 seconds in both cases and there's no continuation. Examples:

With async/await:

    class Program
    {
        static async Task Main(string[] args)
        {
            Foo();
            Console.WriteLine("Done calling foo");
            await Task.Delay(TimeSpan.FromSeconds(10));
        }

        public static async void Foo()
        {
            await Task.Delay(TimeSpan.FromSeconds(5));
        }
    }

When you run this, Done calling foo is printed to the console immediately, not after the 5 second delay has finished. (I have that 10 second delay added to Main just so that the program doesn't immediately end. And of course I can't await Foo() because it returns void.)

The (properly) elided version:

    class Program
    {
        static async Task Main(string[] args)
        {
            Foo();
            Console.WriteLine("Done calling foo");
            await Task.Delay(TimeSpan.FromSeconds(10));
        }

        public static void Foo()
        {
            Task.Delay(TimeSpan.FromSeconds(5));
        }
    }

Just like with the async/await version, Done calling foo is printed to the console immediately. There is little to no difference to the caller, since async void methods cannot be awaited. In this example that can be elided, they are "fire and forget" at the first Task invocation.

Say you were to have code after the delay, in which case it would matter:

    class Program
    {
        static async Task Main(string[] args)
        {
            Foo();
            Console.WriteLine("Done calling foo");
            await Task.Delay(TimeSpan.FromSeconds(10));
        }

        public static async void Foo()
        {
            await Task.Delay(TimeSpan.FromSeconds(5));
            Console.WriteLine("Done delaying");
        }
    }

In this case, Done calling foo is still printed to the console immediately, but 5 seconds later Done delaying is printed.

However, this version cannot not be elided, because there is code after the await. And I correctly do not get the hint to remove it in this last case.

The core bug here IMO is that choosing the eliding action adds a return statement, which is obviously invalid syntax in a void returning method. But I cannot come up in my mind with any example, even impractical ones, in which a properly elided async void method has any actual difference in behavior. I'm not saying there isn't one possibly, I'd love to know if there is. But the delay example behaves the same AFAICT both elided and async.

@paulirwin
Copy link

@mdpopescu After posting my last comment, I did think of one case where it makes a difference: debugging. In the elided version, if you step over the Task.Delay call you hit the closing curly brace immediately instead of after 5 seconds like you do in the async version. I believe this must be because of some kind of nop introduced at the end of the method in Debug mode to serve as a continuation for breakpoints. But I would not think it's worth it to remove this hint on async void for this reason alone. If this type of debugging in this case is important, the hint can be disabled with a comment. In this example, I would prefer to still elide it, so that it does not build the unnecessary async state machine.

@mdpopescu
Copy link

Ah... thank you. I did not realize that async void would return immediately (I had an event handler with that code and the only testing I did was with the debugger... which, as you point out, gave me a false idea). Thanks, I learned something new here!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants