Proper way to implement methods that return Task<T> [duplicate]

Question

For simplicity let's imagine we have a method that should return an object while doing some heavy operation. There're two ways to implement:

public Task<object> Foo()
{
    return Task.Run(() =>
    {
        // some heavy synchronous stuff.

        return new object();
    }
}

And

public async Task<object> Foo()
{
    return await Task.Run(() =>
    {
        // some heavy stuff
        return new object();
    }
}

After examining generated IL there're two completely different things generated:

.method public hidebysig 
    instance class [mscorlib]System.Threading.Tasks.Task`1<object> Foo () cil managed 
{
    // Method begins at RVA 0x2050
    // Code size 42 (0x2a)
    .maxstack 2
    .locals init (
        [0] class [mscorlib]System.Threading.Tasks.Task`1<object>
    )

    IL_0000: nop
    IL_0001: ldsfld class [mscorlib]System.Func`1<object> AsyncTest.Class1/'<>c'::'<>9__0_0'
    IL_0006: dup
    IL_0007: brtrue.s IL_0020

    IL_0009: pop
    IL_000a: ldsfld class AsyncTest.Class1/'<>c' AsyncTest.Class1/'<>c'::'<>9'
    IL_000f: ldftn instance object AsyncTest.Class1/'<>c'::'<Foo>b__0_0'()
    IL_0015: newobj instance void class [mscorlib]System.Func`1<object>::.ctor(object, native int)
    IL_001a: dup
    IL_001b: stsfld class [mscorlib]System.Func`1<object> AsyncTest.Class1/'<>c'::'<>9__0_0'

    IL_0020: call class [mscorlib]System.Threading.Tasks.Task`1<!!0> [mscorlib]System.Threading.Tasks.Task::Run<object>(class [mscorlib]System.Func`1<!!0>)
    IL_0025: stloc.0
    IL_0026: br.s IL_0028

    IL_0028: ldloc.0
    IL_0029: ret
}

And

.method public hidebysig 
    instance class [mscorlib]System.Threading.Tasks.Task`1<object> Foo () cil managed 
{
    .custom instance void [mscorlib]System.Runtime.CompilerServices.AsyncStateMachineAttribute::.ctor(class [mscorlib]System.Type) = (
        01 00 1a 41 73 79 6e 63 54 65 73 74 2e 43 6c 61
        73 73 31 2b 3c 42 61 72 3e 64 5f 5f 31 00 00
    )
    .custom instance void [mscorlib]System.Diagnostics.DebuggerStepThroughAttribute::.ctor() = (
        01 00 00 00
    )
    // Method begins at RVA 0x2088
    // Code size 59 (0x3b)
    .maxstack 2
    .locals init (
        [0] class AsyncTest.Class1/'<Foo>d__1',
        [1] valuetype [mscorlib]System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1<object>
    )

    IL_0000: newobj instance void AsyncTest.Class1/'<Foo>d__1'::.ctor()
    IL_0005: stloc.0
    IL_0006: ldloc.0
    IL_0007: ldarg.0
    IL_0008: stfld class AsyncTest.Class1 AsyncTest.Class1/'<Foo>d__1'::'<>4__this'
    IL_000d: ldloc.0
    IL_000e: call valuetype [mscorlib]System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1<!0> valuetype [mscorlib]System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1<object>::Create()
    IL_0013: stfld valuetype [mscorlib]System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1<object> AsyncTest.Class1/'<Foo>d__1'::'<>t__builder'
    IL_0018: ldloc.0
    IL_0019: ldc.i4.m1
    IL_001a: stfld int32 AsyncTest.Class1/'<Foo>d__1'::'<>1__state'
    IL_001f: ldloc.0
    IL_0020: ldfld valuetype [mscorlib]System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1<object> AsyncTest.Class1/'<Foo>d__1'::'<>t__builder'
    IL_0025: stloc.1
    IL_0026: ldloca.s 1
    IL_0028: ldloca.s 0
    IL_002a: call instance void valuetype [mscorlib]System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1<object>::Start<class AsyncTest.Class1/'<Foo>d__1'>(!!0&)
    IL_002f: ldloc.0
    IL_0030: ldflda valuetype [mscorlib]System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1<object> AsyncTest.Class1/'<Foo>d__1'::'<>t__builder'
    IL_0035: call instance class [mscorlib]System.Threading.Tasks.Task`1<!0> valuetype [mscorlib]System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1<object>::get_Task()
    IL_003a: ret
}

As you can see in the first case logic is straightforward, lambda function is created and then call to the Task.Run is generated and the result is returned. In the second example instance of AsyncTaskMethodBuilder is created and then task is actually built and returned. Since I always expected foo method to be called as await Foo() at some higher level, I have always used the first example. However, I see the latter more often. So which approach is correct? What pros and cons do each have?

---

Real world example

Let's say we have UserStore which has method Task<User> GetUserByNameAsync(string userName) which is used inside web api controller like:

public async Task<IHttpActionResult> FindUser(string userName)
{
    var user = await _userStore.GetUserByNameAsync(userName);

    if (user == null)
    {
        return NotFound();
    }

    return Ok(user);
}

Which implementation of Task<User> GetUserByNameAsync(string userName) would be correct?

public Task<User> GetUserByNameAsync(string userName)
{
    return _dbContext.Users.FirstOrDefaultAsync(user => user.UserName == userName);
}

or

public async Task<User> GetUserNameAsync(string userName)
{
    return await _dbContext.Users.FirstOrDefaultAsync(user => user.UserName == username);
}

Upanishad

Related: Any difference between “await Task.Run(); return;” and “return Task.Run()”? – Kreg

Answer 1

As you can see from the IL, async/await creates a state machine (and an additional Task) even in the case of trivial async tail calls, i.e.

return await Task.Run(...);

This leads to performance degradation due to additional instructions and allocations. So the rule of thumb is: if your method ends with await ... or return await ..., and it is the one and only await statement, then it's generally safe to remove the async keyword and directly return the Task that you were going to await.

One potentially unintended consequence of doing this is that if an exception is thrown inside the returned Task, the outer method will not appear in the stack trace.

There's a hidden gotcha in the return await ... case too though. If the awaiter is not explicitly configured not to continue on captured context via ConfigureAwait(false), then the outer Task (the one created for you by the async state machine) cannot transition to completed state until the final postback to the SynchronizationContext (captured just before the await) has finished. This serves no real purpose but can still result in a deadlock if you block on the outer task for some reason (here's a detailed explanation of what happens in such case).

Chiarra

Thanks. Well explained answer. Stacktrace is never worth a performance, imho. Besides that is there any reason why people use trivial async tail calls? – Upanishad

@Leri, I can't think of any other reason to do this, and I'm not seeing anything in this Roslyn team discussion on making this a compiler optimization moving forward: github.com/dotnet/roslyn/issues/1981 – Chiarra

I see. Well, from this point I think the answer to my question is pretty obvious. Thanks. – Upanishad

Answer 2

So which approach is correct?

Neither.

If you have synchronous work to do, then the API should be synchronous:

public object Foo()
{
    // some heavy synchronous stuff.

    return new object();
}

If the calling method can block its thread (i.e., it's an ASP.NET call, or it's running on a thread pool thread), then it just calls it directly:

var result = Foo();

And if the calling thread can't block it's thread (i.e., it's running on the UI thread), then it can run Foo on the thread pool:

var result = await Task.Run(() => Foo());

As I describe on my blog, Task.Run should be used for invocation, not implementation.

---

Real World Example

(which is a completely different scenario)

Which implementation of Task GetUserByNameAsync(string userName) would be correct?

Either one is acceptable. The one with async and await has some extra overhead, but it won't be noticeable at runtime (assuming the thing you're awaiting actually does I/O, which is true in the general case).

Note that if there's other code in the method, then the one with async and await is better. This is a common mistake:

Task<string> MyFuncAsync()
{
  using (var client = new HttpClient())
    return client.GetStringAsync("http://www.example.com/");
}

In this case, the HttpClient is disposed before the task completes.

Another thing to note is that exceptions before returning the task are thrown differently:

Task<string> MyFuncAsync(int id)
{
  ... // Something that throws InvalidOperationException
  return OtherFuncAsync();
}

Since there is no async, the exception is not placed on the returned task; it is thrown directly. This can confuse the calling code if it does something more complex than just awaiting the task:

var task1 = MyFuncAsync(1); // Exception is thrown here.
var task2 = MyFuncAsync(2);
...
try
{
  await Task.WhenAll(task1, task2);
}
catch (InvalidOperationException)
{
  // Exception is not caught here. It was thrown at the first line.
}

Telegony

+1. Well, for the first part of your answer, Task.Run was just for demonstration. And for the second part you're totally right if one's application uses exceptions to control application flow. However, I let my exceptions be exceptions i.e. in my current architecture, if exception is thrown means something awfully wrong has happened, so I don't handle them as you've shown. – Upanishad

You don't use databases and http client and other classes with using? Even if you can't handle the exception you still should do proper cleanup @Leri. The second part of this answer is the important one. – Cuvette

@BenjaminGruenbaum For web development no. I let di scope do that for me. – Upanishad

Answer 3

To my knowledge, the only reason to mark your method with the async keyword is to enable the usage of await as a placeholder for the compiler to create the callback point in the IL. That is, the word "await" could actually be used as a variable name, or have different meaning outside the context of an async marked method. "Await's" appearance in an async marked method will cause the compiler to create a callback checkpoint, implemented as Movenext(), for each awaited/blocked call and subsequent SetResult(). This will indeed create more overhead as Tasks (and all local variables therein),like all anonymous methods, are stored on the heap and must be allocated and GC'd at runtime.

Even if your method is not marked as async, you call still and awaited calls (hack-ish, and not ideal), but you have to fake the compiler out by doing it via an async lambda...

Task doSomething()
{
   /*Technically, the await is not in the doSomething method, but in a 
   newly created anonymous Action delegate, so the rule still holds; I 
    just created a different scope.  I believe you could do the same 
    with an inline sub-method. */

   Action a=async()=await doWorkAsync();
   a();
}

This type of hack-ish solution is often accompanied with poorly designed systems, or businesses that push for version 2 when version 1 doesn't support their desired functionality (bad architecture, but it happens all the time).

Generally, if I don't need to await something in my method (perhaps due to some prerequisite dependence value), then I do not mark that method as async, and return the Task itself. I seen many apps that async and await from UI to service layer to business to data, etc, and the allocation, deallocation, and repeat (the creation of a Task to then return its Result to one tier, that then rewraps that result into another Task for another tier to repeat the same wasteful process) ad infinity...is humorous.

From my point of view, the only one that needs to await any call it the one needing the Task.Result, and often that might only be the consumer of the method. NOTE that you can await any method that returns a Task, but you can not await INSIDE of a method that is not marked as async; so, I can await my async results from my consuming app/service, and only await what dependence Results I need elsewhere.

You can access Task.Result in a method that is not marked as async, which gives you the way result and await, but doing this can lead to some VERY hard deadlocks to debug, so be careful. You can find info here about that. I can drive a car with my feet, but that doesn't mean it is TO BE DONE.

There are issues, mentioned above, with exceptions, but generally those issues arise from devs handling exception incorrect (not using the ExceptionDispatcher return such that Stace info isn't lost).

Stephen Toub is one of the primary creators of the Task functionality. He's a sharp guy and has some useful videos online.

Lott

"This will indeed create more overhead as Tasks (and all local variables therein), like all anonymous methods, are stored on the heap and must be allocated and GC'd at runtime." -- Have you tested this theory experimentally? Try testing it using GC.GetTotalAllocatedBytes method, in Release mode, on a recent version of the .NET runtime, and see what happens. – Honoria

I watched a video presented by Stephen Toub about optimizing Task operations. It was one of the methods he mentioned, as well as caching Task that have common results (bool, int, and object [when it returns null]) and a few others methods as well. I believe it was MS Deep dotNET series. Check it out, but Stephen does benchmark the methods I referenced, both on memory allocations and GC cycles required. – Lott

It was an old video though, 8 yrs. It was titled The Zen of Async.. Here’s the link… youtu.be/zjLWWz2YnyQ?feature=shared – Lott

You are claiming in your answer that adding async/await results in additional allocations, on top of the allocations caused by the Task.Run or the _dbContext.Users.FirstOrDefaultAsync. Does Stephen Toub's video validate your claims? It's a 67 minutes video. At what minute in the video does the confirmation occur? Be aware that each .NET version release comes with numerous performance improvements. At 26 Sept 2015 (date of the video) .NET Core was not even a thing yet (.NET Core 1.0 was released on June 27, 2016). – Honoria

If I understand Stephen’s video correctly, due to the nature of closures in lambda expressions, variable’s state at the time of delegate/lambda creation are encapulated within the ExecutionContext, and carried with the actual method itself. The ExecutionContext is the state that goes with any call, sync or async, and when we switch between working and awaiting, that state must be persisted. This switch is commonly called a context switch. But, back to the point. That serialized and deserialized context is allocated and deallocated and reallocated over and over. … – Lott

To be honest, an async call on a UI doesnt much affect the overall performance, but when that type of activity is compounded by 100+ users, perhaps some of them doing iteratlive or cyclid operations (foreach) in the 1000+ calls on an application server, then it gets ugly fast. Given the “scale out”-ability of cloud implementations, this is basically solved by throwing hardware at the problem. Latency is the biggest problem, after all, in cloud. But, it’s a concern for library developers nonetheless. Context Switching has an exponentially detrimental effect on performance. – Lott

At some point, throwing more hardware at the problem had diminishing results. That is to say, going from 2 to 4 processors might have increased performance by 100%, but going from 64 to 128 wont have the same increase. The context switching incurred increases as request/transactions increase and state switching between contexts commensurately, and rightly, increases. – Lott

Yeah, Stephen’s video confirms what I was saying; however, it is an old video and older version of .NET he was working in. Nevertheless, understanding ExecutionContext and lambda closures as they work in the current version, the problems he adddresses in that video is still an issue. Context switching, too, is always been a problem with computers, since day 1 of their creation. Heck, that’s even a problem for humans too;the more things I work on, the less things I get done, as I spend more or more energy/time restarting task than I do actually DOING them, an async cost in all arenas of life – Lott

_dbContext is a Entity Framework nomenclature. I dont use EF, and dont much like ORMs, to be honest. I actually know RDBMS and prefer stored procs, but to each their own. That’s an different topic completely. I was referring to ExecutionContext, a .net framework type, that travels with EVERY .net method call, to a more or less degree. EC is the parent container for all other contexts (Security, Synchronization, etc.). – Lott