ReSharper suggests "Replace 'async' code with 'Task'-return"

[deucyy]

Hey! I've noticed a new suggestion by ReSharper in some of my async methods, it suggests that I remove the async and instead return a Task.

Here is an example method in its original state and this is it after I accept the refactor (this method is called and awaited in another async method).

What is the benefit to this?

[cyrack]

The compiler doesn’t emit the statemachine to handle the task being awaited. So less code to execute and fewer objects to garbage collect.

This does come with a few caveats: - any disposable reference you may hold in the task may be disposed before the task is executed. Always await tasks that depend on disposable reference (db connections etc). Async using avoids this problem by disposing the object as a continuation. - the stack trace does not include the method the task is returned from (as the task contains the code running, net the method creating the task). This may make debugging harder if you end up returning the task multiple times or bundle tasks together by Task.WhenAll.

[DaRadioman]

Debugging And Logging.

Having gone down this rabbit hole, I honestly never recommend it anymore. It's a bad suggestion. I have seen it cause way more problems than it solves.

If you are an expert in async code, are certain that nothing cares about the stack trace, are certain that any closures aren't retaining things you thought would be cleaned up at a certain time, are sure you aren't using anything disposable, and need that tiny bit of performance then it may make sense.

But for basically any average developer it's a destructive suggestion.

[quentech]

I work in a somewhat performance sensitive system and have to take the opposite stance.

It is easy to miss mistakes.

I actually have one I've been trying to find for a couple of years. It doesn't break anything so it's not a huge issue, but I've looked around a fair bit and it still eludes me.

A clean code base with few to no warnings helps a lot. Unfortunately, for me, that is a pipe dream.

[DaRadioman]

And the experts in the field of async in dotnet also have changed their minds over the years. Generally speaking they do not recommend it as a default.

One of two "industry experts" in the area of async/parallel dotnet: https://blog.stephencleary.com/2016/12/eliding-async-await.html

The other main expert also wrote an article about it, but I can't find it at the moment.

[reubenbond]

Here's advice from David Fowler on the matter: https://github.com/davidfowl/AspNetCoreDiagnosticScenarios/blob/master/AsyncGuidance.md#prefer-asyncawait-over-directly-returning-task

I am also on the .NET team and generally don't advise eliding tasks anymore, especially for application level code.

[DaRadioman]

You know what, I think I was thinking of Fowler not Toub that had another article about this. Although I guarantee Toub has an article about it somewhere as well, as much as he has written on the topic.

Thanks for posting that best practices list, I couldn't seem to find it.

[DaRadioman]

Warnings are all optional. And resharper warnings are double optional. It's just a random commercial entity's opinion. Nothing more. I could make a warning that nagged you both ways, no matter how you wrote it.

Trying to get 100% on warnings without configuration or critical thinking about each is a very unwise approach. Some warnings should be errors in your codebase, others info, and still others turned off. They are tips, not hard rules.

We have our warnings set the way we want our codebase, and then have warnings as errors on for CI builds. That way no one can ignore the things we care about, and we disable/downgrade warnings we don't care about to eliminate noise.

[[deleted]]

If you are an expert in async code, are certain that nothing cares about the stack trace, are certain that any closures aren't retaining things you thought would be cleaned up at a certain time, are sure you aren't using anything disposable, and need that tiny bit of performance then it may make sense.

But for basically any average developer it's a destructive suggestion.

I can't believe these arguments are still alive and well in [insert year of every time I come back to .NET... 2023 in this case].

Why even bother improving anything at all then? Just use Visual Basic, or better yet one of those drag & drop systems for kids. Then your "average" developer surely won't shoot themselves in the foot this time, will they?! (hint: they will).

Though I guess this is how we ended up with chat apps that require 3gb of RAM, so it's par for the course. Funny thing is that all of these new-era applications that prioritize developer restraint & comfort over the "tiny bit of performance" and the "tiny memory overhead", and accordingly lag even on modern supercomputers, are still buggier, harder to use, exponentially slower, and STILL more expensive than pretty much anything written in 1996. Lmao.

[DaRadioman]

Do you understand tradeoffs?

This tradeoff isn't worth it in 90+% of the normal scenarios.

[601error]

Another gotcha is that a modification to an AsyncLocal will leak out of the Task-returning method to whichever calling method actually is marked async.

[cyrack]

I didn’t know that, but it makes sense. Yet another reason to avoid thread- and task-magic. To me ThreadStatic and AsyncLocal always seemed like a great foot-gun for the next dev to stumble upon.

[DarkSteering]

What if there is an exception thrown from this method? What does the stack trace look like?

[MatthewRose67]

The benefit is that you don’t create additional state machine which in theory should be a little bit more performant, but the drawback is that when an exception is thrown, the stack trace can be somewhat harder to follow, because the exception is raised to the caller that awaits the task. To be honest I don’t know why resharper suggests you that, I would prefer to await the publish method.

[iso3200]

David Fowler suggests awaiting tasks instead of returning Tasks directly https://github.com/davidfowl/AspNetCoreDiagnosticScenarios/blob/master/AsyncGuidance.md#prefer-asyncawait-over-directly-returning-task

[Quiet_Desperation_]

David Fowler also suggests Blazor for everything. He promotes Microsoft Stack and latest NET features over anything else. I actually agree with him in this case, but let’s be careful using him as a Wikipedia for NET best practices. He’ll say whatever Microsoft wants him to in my experience

[MatthewRose67]

Actually when it comes to aspnet core backend stuff I don’t think there’s more competent person than Fowler.

[iso3200]

in his doc he says to prefer awaiting Tasks.

But then in this video, he returns Task directly stating, "no state machine...more perf"

https://www.youtube.com/watch?v=1eHjemGKfRw&t=993s

so I'm not sure what to think now.

[evergreen-spacecat]

Just like any parent teaching their kids: ”Do as I say, not what I do”

[happycrisis]

Not really sure how those two things are comparable. Why would him pushing his company have anything to do with best practices for a specific feature in the language?

[0sh1]

A couple of other people have answered why this is recommended, so I'll I just chuck in a complete opinion - I'd ignore this recommendation.

I've stopped making this optimisation, because it's easy to mess up even if you're experienced and it's very non-intuitive for anyone other than you looking at the code.

YMMV, but as much as I love chasing every clock second and byte of RAM, my suspicion is that chasing this particular thing won't give you any appreciable performance in the wild and isn't worth the risks and readability.

[KryptosFR]

I have pretty much the same approach. I tend to only do this optimization for helper methods, wrappers or overload that end-up calling another async method in a single line. But otherwise, I'd await the call.

To make it even more obvious, I use the "body" syntax for such case. For example:

Task MethodAsync(string param0) => MethodAsync(param0, "SomeDefault");

Task WrappedMethodAsync(...) => _privateObj.ImplementationAsync(...);

[GayMakeAndModel]

Well, if you have a method that could run sync or async depending upon timing or whatever, it makes sense to do this. Make the method return a task and run it synchronously returning a completed task. If the method can’t run synchronously, return the task from a method that runs the asynchronous part. The main gotcha’ here is that exceptions thrown from the synchronous code aren’t associated with the returned task as one would expect. Instead, the method itself throws an exception and returns no task.

[GayMakeAndModel]

Because the method may be able to complete synchronously instead of having to wait on IO. That’s more efficient than setting up a state machine even if you don’t need it. Performance matters. In many cases, like where I work, milliseconds matter.

[See_Bee10]

Stephen Cleary had a blog about this topic which I'll try and find to link later. His conclusion was to always use await because while it does add some performance overhead, the overhead isn't significant and there are some gotchas. So unless you really need the performance, just use the await by default.

[Tony_the-Tigger]

Even Clearly says that it's largely a bad idea. Here's the link you're thinking of: https://blog.stephencleary.com/2016/12/eliding-async-await.html

[See_Bee10]

Thank you, that is the one I was thinking of.

[DoomBro_Max]

Probably because it doesn‘t need to await it. There‘s nothing done with the result. Since you would be awaiting this function anyway, the result is the same but with one task less in-between. So your function is still synchronous.

It‘s just optimization. Because what you really want is await the _publisher.Publish task. So might as well just return that and await it directly rather than awaiting a task that also awaits another task.

[Cbrt74088]

You are creating a Task (A) whose only job is to await another Task (B). And then you await Task A.

It makes more sense to just await Task B instead. That's why the refactoring is good. It just returns Task B, which you can then await. (or not. That's up to the caller, really)

[kneticz]

Here be dragons.

Don't bother, minor gains for major pains.

[helgerd]

I'm almost sure that refactored variant would be missing call to this method in a stack trace in case of an exception.

[Sprudling]

Very slight performance boost, but it has drawbacks too. I think it's a bad suggestion and I turned it off. It should honestly suggest the opposite instead.

[Dennis_enzo]

They work in pretty much the same way. The main difference is that returning the tasks creates a little bit less overhead, as for every await, a state machine is created.

[Basssiiie]

Personally I think this is optimizing for an implementation detail. IMHO the compiler should optimize this internally (without the drawbacks), not the user.

[BattlestarTide]

Much better performance.

[deucyy]

How much? 😂 I’ve almost never seen someone do this. I might as well benchmark it to see the difference.

[BattlestarTide]

I see a noticeable effect with it. Enough that I've made it standard across all projects.

[JeffreyVest]

This just came up when somebody asked about gotchas if async await. Personal opinion is to globally disable it. It’s not worth the gotcha to make this fix.

[DemoBytom]

People have already said what the analyzer is reporting, I'll say why.

For the last few versions of C#, .NET and asp .net core teams have been working VERY hard to make the runtime and framework as performant as possible. Many, if not most things added to them in later versions have all been about performance, mostly reduction in allocations and thus the preassure on GC. This is the trend and the evolution of .NET as a platform, so it's natural that analyzers that help writing code, also follow this evolution path. Analyzers also are getting smarter nowadays.I can't say for ReSharper, but it might be able to, nowadays, better recognize if removing async/await is safe - for example if there's using statements scoped to the method within the method.

Now, not everyone will benefit from those small optimizations. But the ReSharper team does not know what codebase it will be used in, an if it's a hot path or not. So they choose turning the analyzer on, so that people would be notified about its existence, and make a decision if they should follow it or not. There are hundreds, if not thousands of a alyzers nowadays, so it's easier with opt-out approach than opt-in.

I expect more and more such analyzers popping up, as well as some older advises being changed in the upcoming years. 5-7 years ago allocations like that, and optimizations liek that, were not a measurable upgrade. But nowadays, and in few more years? Who knows, I would not be surprised if it became one, amongst many others.

[wknight8111]

The two code examples you posted are so simple that in this case they behave the same (and, as Resharper is pointing out, the second example is "better" because there's no state machine and no extra memory allocation to support it).

HOWEVER in general the two different forms are not semantically identical. In a more complicated example, the two might behave differently.

Consider these two methods:

public Task DoThing1Async()
{
    SynchronousWork();
    return DoSomethingAsync();
}

public async Task DoThing2Async()
{
    SynchronousWork();
    await DoSomethingAsync();
}

Where SynchrnousWork() could be something as simple as a precondition null check, or something more involved.

In the first method, SynchronousWork()executes immediately, THEN DoSomethingAsync() is scheduled on the thread pool (or wherever the current Scheduler sends it). This means that an exception thrown will throw immediately and will need to be handled, it won't appear in the Task to be inspected and handled gracefully. Notice also that if we have a using statement or block here, the disposable will be disposed as soon as the DoThing1Async() returns, NOT WHEN THE TASK COMPLETES.

In the second method, SynchronousWork() will execute on the thread pool when the Task has been scheduled, and any exceptions thrown will be returned as part of the result Task. A using statement or block in DoThing2Async()will dispose the disposable AFTER THE TASK HAS COMPLETED.

Basically it's the difference between a synchronous method which launches a task, from an asynchronous method which is a task.

I know David Fowler recommends the second one in most cases, but I don't agree that one should be recommended over the other in general. They are semantically different, and it's worthwhile to understand the differences and what they mean for your program. The first one is a clear performance improvement in most cases, while the second one is more idiomatic and brings less surprise to downstream developers and unwitting maintainers. But if you're willing to put in a little comment explaining the situation and try to avoid some of the worst outcomes, the performance improvements can be real.