5

Suppose I have a method that awaits a Task. This method also returns a Task. For example:

public async virtual Task Save(String path)
{
    if (NewWords.Any())
    {
        await FileManager.WriteDictionary(path, NewWords, true);
    }
    else await Task.Run(() => { });
}

Is the

else await Task.Run(() => { });

necessary here or am I free to leave it? Is there any difference if it is present/absent? Maybe there is some other approach to this I should take?

svick
  • 236,525
  • 50
  • 385
  • 514
Balázs
  • 2,929
  • 2
  • 19
  • 34
  • 1
    Tell us why you inserted that code. It's not necessary and it's evidence that you must misunderstand something. Need to find out what it is. – usr Apr 08 '16 at 12:16
  • 2
    It's not necessary, and it's also harmful. – IS4 Apr 08 '16 at 12:21
  • DONT USE `Task.Run` IN IMPLEMENTATION! http://blog.stephencleary.com/2013/11/taskrun-etiquette-examples-dont-use.html – Aron Apr 19 '16 at 01:20

2 Answers2

6

It's worse than unnecessary, as you're spinning up a thread to do nothing and then waiting until after its finished doing nothing.

The simplest way to do nothing, is to do nothing. In an async method the method will still have returned a Task, but that Task will be completed already, so something awaiting it further up will get straight onto the next thing it needs to do:

public async virtual Task Save(String path)
{
    if (NewWords.Any())
    {
        await FileManager.WriteDictionary(path, NewWords, true);
    }
}

(Also, it would be more in line with convention if SaveAsync and WriteDictionaryAsync were the method names here). If not using async (and there's no need to here, but I understand it's an example) use Task.CompletedTask:

public virtual Task Save(String path)
{
    if (NewWords.Any())
    {
        return FileManager.WriteDictionary(path, NewWords, true);
    }
    return Task.CompletedTask;
}

If you are coding against an earlier framework than 4.6 and therefore don't have CompletedTask available, then Task.Delay(0) is useful as Delay special cases the value 0 to return a cached completed task (in fact, the same one that CompletedTask returns):

public virtual Task Save(String path)
{
    if (NewWords.Any())
    {
        return FileManager.WriteDictionary(path, NewWords, true);
    }
    return Task.Delay(0);
}

But the 4.6 way is clearer as to your intent, rather than depending on a quirk of implementation.

Jon Hanna
  • 110,372
  • 10
  • 146
  • 251
  • Thanks for the explaination, that helped me clean this up. I was a bit puzzled about the compiler allowing me not to return anything on some code path (despite being aware of that Task is the result of a background operation returning void). I guess the compiler treats it uniquely just like it does void, right? – Balázs Apr 08 '16 at 12:44
  • Pretty much. The point of `async` is to let us write methods that return tasks with potentially pretty complicated sequences of waiting on other tasks, much as we would write the equivalent synchronous method. So just as a synchronous method that does nothing can be empty (or empty in a given branch) so can an `async` that just returns a completed task. What actually happens is that a state machine is created and similar to how `yield` is turned into `MoveNext()` so this has a `MoveNext()` that is called into once at the beginning and then after every task you've `await`ed returns... – Jon Hanna Apr 08 '16 at 13:04
  • ... in the case of following a path that didn't hit an `await` then the first call into that `MoveNext()` indicates that is done. The `Task` that uses that state machine (which is what your method really returns) is then put into a completed state. Since the first `MoveNext()` happens before the `Task` is completed (and indeed any others if all the `await`s hit on completed tasks and don't have to actually wait) then the task is completed already when returned. – Jon Hanna Apr 08 '16 at 13:05
5

It's not neccesary. The async is only needed if at least one await is used. Everything inside the method is executed synchronously except for the await part.

AgentFire
  • 8,944
  • 8
  • 43
  • 90