Return with await when wrapping old async pattern into TaskCompletionSource?

Question

I am studying C# Asnc-await pattern and currently reading Concurrency in C# Cookbook from S. Cleary

He discusses wrapping old non TAP async patterns with TaskCompletionSource (TCS) into TAP constructs. What I dont get is, why he just returns the Task property of the TCS object instead of awaiting it TCS.Task ?

Here is the example code:

Old method to wrap is DownloadString(...):

public interface IMyAsyncHttpService
{
    void DownloadString(Uri address, Action<string, Exception> callback);
}

Wrapping it into TAP construct:

public static Task<string> DownloadStringAsync(
    this IMyAsyncHttpService httpService, Uri address)
{
    var tcs = new TaskCompletionSource<string>();
    httpService.DownloadString(address, (result, exception) =>
    {
        if (exception != null)
            tcs.TrySetException(exception);
        else
            tcs.TrySetResult(result);
    });
    return tcs.Task;
}

Now why not just do it that way:

public static async Task<string> DownloadStringAsync(
    this IMyAsyncHttpService httpService, Uri address)
{
    var tcs = new TaskCompletionSource<string>();
    httpService.DownloadString(address, (result, exception) =>
    {
        if (exception != null)
            tcs.TrySetException(exception);
        else
            tcs.TrySetResult(result);
    });
    return await tcs.Task;
}

Is there a functional difference between the two? Is the second one not more natural?

Oh, how difficult it may be to read an unindented code. This is an additional `Task` which doesn't bring any improvement. You can just return a `Task`, but instead you create a state machine, which will just wait for this task and then return the same result. — Yeldar Kurmangaliyev, Feb 18 '19 at 10:53
This article should actually answer your question: https://stackoverflow.com/questions/19098143/what-is-the-purpose-of-return-await-in-c. — Yeldar Kurmangaliyev, Feb 18 '19 at 10:53
As others have said, it's more complex. It might be useful for you to describe why you think the "just do it that way" version is meant to be an improvement. Remember, `await` is a way of dealing with *existing* running `Task`s. It doesn't *start* any new work itself. — Damien_The_Unbeliever, Feb 18 '19 at 11:07
My thought process was, that since the original method was converted to a TAP construct, the user of the code will surely await the converted method (since it is TAP and awaiting running tasks at some point is how it is supposed to be used). By marking it async, the compiler will generate warnings, that it should be considered to await this method. So imo. this well have a more TAP look and feel from users perspective — J4ni, Feb 18 '19 at 11:24
You're still returning a `Task`. The caller of this method *always* (*1) has to `await` that `Task` if they need to know when the job is done. (*1 - or other moral equivalents) — Damien_The_Unbeliever, Feb 18 '19 at 11:27
Everything about using vs not using async all the way down has already been described by Stephen Cleary [here](https://blog.stephencleary.com/2016/12/eliding-async-await.html) — FCin, Feb 18 '19 at 11:40
@J4ni: It may be helpful to think of `async` as an implementation detail. Returning `Task` is part of the method signature; `async` is not (so the compiler warning - based on the method signature - works fine). `async` is *one* way to create a returned task. In the case of wrapping EAP, you're providing a low-level building block for TAP methods to build on; there's no need for `async` or `await` in that method. — Stephen Cleary, Feb 19 '19 at 15:49

score 2 · Accepted Answer · answered Feb 18 '19 at 11:40

By marking it async, the compiler will generate warnings, that it should be considered to await this method

You don't have to mark your own method as async in order to get the "Task not awaited" warning. The following code generates the same warning for the calls to both T and U:

static async Task Main(string[] args)
{
    Console.WriteLine("Done");
    T();
    U();
    Console.WriteLine("Hello");
}

public static Task T()
{
    return Task.CompletedTask;
}

public static async Task U()
{
    await Task.Yield();
    return;
}

Whenever you find yourself with a method only containing a single await and that being the last thing it does (except possibly returning the awaited value), you should ask yourself what value it's adding. Aside from some differences in exception handing, it's just adding an extra Task into the mix.

await is generally a way of indicating "I've got no useful work to do right now, but will have when this other Task is finished" which of course isn't true (you've got no other work to do later). So skip the await and just return what you would have awaited instead.

score 0 · Answer 2 · answered Feb 18 '19 at 11:03

0

Your version is strictly more complex -- instead of just returning the task, you make the method async, and await the task you could just be returning.

answered Feb 18 '19 at 11:03

Martijn

11,964
12
50
96

Its true, but is it a meaningful/measurable performance penalty, so that I should be aware not to mark straight serial "no real work done just parameter pass-through" functions in an Asynchronous intended function call chain as async? – J4ni Feb 18 '19 at 11:33
Yes, it's likely that you can measure the overhead. – Martijn Feb 18 '19 at 11:52

score 0 · Answer 3 · answered Feb 18 '19 at 11:08

0

There is one subtle practical difference (other than the version with await being slower to run).

In the first example, if DownloadString throws an exception (rather than calling the delegate you pass it with exception set), then that exception will bubble through your call to DownloadStringAsync.

In the second, the exception is packaged into the Task returned from DownloadStringAsync.

So, assuming that DownloadString throws this exception (and no other exceptions occur):

Task<string> task;
try
{
    task = httpService.DownloadStringAsync(...);
}
catch (Exception e)
{
    // Catches the exception ONLY in your first non-async example
}

try 
{
    await task;
}
catch (Exception e)
{
    // Catches the exception ONLY in your second async example
}

You probably don't care about the distinction - if you just write:

await httpService.DownloadStringAsync(...);

you won't notice the difference.

Again, this only happens if the DownloadString method itself throws. If it instead calls the delegate you give it with exception set to a value, then there is no observable difference between your two cases.

answered Feb 18 '19 at 11:08

canton7

37,633
3
64
77

I do not believe it to be true. Your first example where you try to catch a `task` will never fire. Your second example will fire in both cases. – FCin Feb 18 '19 at 11:37
@FCin why not try it? https://dotnetfiddle.net/5RHTGO . The same thing happens with methods containing `yield return` - any exceptions are thrown the first time that `MoveNext()` is called. – canton7 Feb 18 '19 at 11:45
@FCin no, you changed the example to do something different, which doesn't illustrate what I described in my answer. I said specifically "Again, this only happens if the DownloadString method itself throws". I'll update the example to be even clearer on this point, so there is no confusion. – canton7 Feb 18 '19 at 11:53
Your example (even the updated one) is _synchronous_. It doesn't do anything asynchronous! AsyncStateMachine is generated only at the first `await` so it never reaches `TaskAwaiter`. See [here](https://sharplab.io/#v2:EYLgtghgzgLgpgJwDQBMQGoA+ABADAAmwEYBuAWACg9CiBWciy7AZkICZ8BhfAb0vwGFW2ACz4AsgAoAlL36CAvvIHL8ABwQBLAG4R4NAGyEAHISMBRAB4QwagDZw2MygEg+FFy90J8MAMZQ+AC8+AB2cADuZpwA9rYOMJoxoQDKMQCuCH5wADwxwABWcH4wAHwyDJ4wABYIMVHhUVbZaonJkgBEAEIA8j3iHdKVLtgAnL4BAHTYBpVKFApAA===) – FCin Feb 18 '19 at 11:56
@FCin here, this maps exactly to your original question, so it should be easy to see what I'm trying to illustrate: https://dotnetfiddle.net/GNxsza . If you look at your sharplab, you'll see that `throw new Exception("BOOM");` happens inside a try/catch, and the catch block calls `<>t__builder.SetException(exception);`, which sets the Exception on the Task that's returned. The AsyncStateMachine is generated as soon as the `async` method is entered, and not at the first await. Yes `DownloadString` throws synchronously when it's called, but that's what I said very clearly twice in my answer. – canton7 Feb 18 '19 at 12:03
But your code is synchronous. Your lambda expression in `DownloadString` is never called. Consider scenario where `DownloadString` schedules a task and when the task ends it calls a callback. Unfortunately Fiddle doesn't have any web interfaces I could use... – FCin Feb 18 '19 at 12:27
@FCin indeed, that will be different. My answer however focussed specifically on the scenario where `DownloadString` synchronously throws an exception when it's called. I laid this out in my 2nd paragraph, and repeated it in the last. This can happen e.g. if `DownloadString` throws an `ArgumentNullException` if `address` is null. Yes if `DownloadString` calls its delegate with an exception then both of your examples are identical I agree, but that's not the subtle difference that my answer highlighted - it explicitly considered the case where `DownloadString` synchronously throws an exception. – canton7 Feb 18 '19 at 12:31
"In the first example, if DownloadString throws an exception (rather than calling the delegate you pass it with exception set)" and "Again, this only happens if the DownloadString method itself throws. If it instead calls the delegate you give it with exception set to a value, then there is no observable difference between your two cases." – canton7 Feb 18 '19 at 12:33
I think it should be stated much more explicitly in your answer since it can lead someone into thinking that this code will catch exceptions from running tasks, which is not true and may be difficult to debug. – FCin Feb 18 '19 at 12:33
I tried to state it very clearly, twice, because I was aware that it was a difference which only affected one small case, and might be easily misinterpreted by someone skimming over the answer. If you have any suggestions on improved wording, please make an edit or suggest in the comments. – canton7 Feb 18 '19 at 12:35

score 0 · Answer 4 · answered Feb 19 '19 at 13:36

Folks, thanks for the helpful comments.

In the meantime I have read the sources referenced here and also investigated the matter further: Influenced by https://blog.stephencleary.com/2016/12/eliding-async-await.html I have come to the conclusion, that its best practice to include async-await by default even in synchronous methods of the async function chain and only omit async await when the circumstances clearly indicate that the method will not potentially behave differently as expected from an async method en edge scenarios.
Such as: the synchronous method is short, simple and has no operations inside which could throw an exception. If the synchronous method throws an exception the caller will get an exception in the calling line instead of the line where the Task is awaited. This is clearly a change from expected behavior.

For example handing over call parameters to the next layer unchanged is a situation which imo permits omitting async-await.

score 0 · Answer 5 · answered Feb 19 '19 at 18:50

0

Read my answer why returning the task is not a good idea: What is the purpose of "return await" in C#?

Basically you break your call stack if you don't use await.

answered Feb 19 '19 at 18:50

haimb

381
3
4

Return with await when wrapping old async pattern into TaskCompletionSource?

5 Answers5