2

I'm fairly sure there's a duplicate somewhere on here but I can't find it.

Quite simply, is it better do to this:

await MyMethod();

With:

public async Task MyMethod()
{
   Thread.Sleep(10000); // Simulates long running operation
}

or something involving Task.Run?

public async Task MyMethod()
{
   await Task.Run(() => Thread.Sleep(10000)); // Simulates long running operation
}

Actual long running operation is a blocking call that does not support async.

The first example obviously has visual studio complaining that it will run synchronously. The reason for making the method async is because I want people to be able to easily await so that they don't tie up their thread.

NibblyPig
  • 51,118
  • 72
  • 200
  • 356
  • I don't have the right answer for this and I think everybody would have different opinions. Mine is depends on the code, what you are trying to achieve. I can't think of a single case where this would be ok, but as I said, it is quite a generic question – Iria Jan 31 '20 at 10:19
  • 4
    If you don't have async code, then don't try to shoehorn it in. Let the callers deal with async if they really want to. – DavidG Jan 31 '20 at 10:20
  • 1
    The idea is that if you're writing an async program, and you want to call a blocking method, you'd have to roll your own Task.Run() or something every time you want to call it. So I thought it'd be nicer to have the method itself handle that. For example, with HttpClient you can use GetAsync, and you might use it all over your program. If it was just 'Get' and your program was async, you'd have to roll a solution every single time you called it if you wanted it to behave asynchronously. So it's to mitigate that. But perhaps it's wrong to mitigate it. – NibblyPig Jan 31 '20 at 10:26
  • 2
    @NibblyPig The caller could create their own wrapper if they need to call it often. – Johnathan Barclay Jan 31 '20 at 10:28
  • if you use `await Task.Delay` instead of `Thread.Sleep` then this can be `async`. The `Task.Run(() => Thread.Sleep())` defeats the object of async – Liam Jan 31 '20 at 10:32
  • Take a look at this: [Should I expose asynchronous wrappers for synchronous methods?](https://devblogs.microsoft.com/pfxteam/should-i-expose-asynchronous-wrappers-for-synchronous-methods/) or this: [Don't Use Task.Run in the Implementation](https://blog.stephencleary.com/2013/11/taskrun-etiquette-examples-dont-use.html). – Theodor Zoulias Jan 31 '20 at 13:29
  • How on earth is this "opinion based"? – Liam Feb 07 '20 at 08:20
  • 1
    @Liam it's not - the problems of fake async methods are well known. – Panagiotis Kanavos Feb 07 '20 at 10:16

3 Answers3

5

Using await with your first example is useless, because the Task will only return once the method completes, at which point the Task is also complete.

I wouldn't use the second example either; don't assume the caller wants the method to run on the thread pool; allow them to decide for themselves:

public void MyMethod()
{
   Thread.Sleep(10000); // Simulates long running operation
}

Caller:

await Task.Run(MyMethod);

I think the key point to make here is that, if a method is not truly asynchronous, it shouldn't claim to be so.

Somewhere down the line, a thread is being blocked, whether that be the current (UI) thread, or another thread on the thread pool.

Task.Run comes with overhead due to switching between threads, which may outweigh the benefits, depending on the consumer.

In an ASP.NET environment, it would likely be preferable to run the method synchronously; the HTTP response can only be sent once the operation has completed, so why introduce overhead?

In a desktop environment, it is key to keep the UI thread responsive, so the overhead created is worthwhile.

Johnathan Barclay
  • 18,599
  • 1
  • 22
  • 35
3

Tl;Dr

Should you use async or not depends on whether this code is blocking or not. If it is blocking and you can't make it unblocking then don't use async and you'll just have to deal with the blocking code. If you can make it non-blocking, using async then do so. Adding threads (using Task.Run() not Thread) depends on what this code is doing, is it IO or CPU bound?

If this code is blocking and you can't change that then the correct implementation would be:

public void MyMethod()
{
   // Long running operation
}

In fact if your code is blocking and contains no await calls, this is exactly what the pre-compiler will turn your method into (async or not).

Generally if your using async then always use the Task class (and it's methods) as this adds a layer of abstraction over the underlying threads and enables the async pattern. TBH use Task pretty much all the time, it's rare you need to interact with a thread directly since TPL. In code reviews I basically reject anyone using Thread now and tell them to use Task instead.

The correct way to simulate a delay in an async method is to use Task.Delay(), so:

public async Task MyMethod()
{
   await Task.Delay(10000).ConfigureAwait(false); // Simulates long running operation
}

This will release the thread back into the thread pool while the delay is happening. Your current method blocks the main thread so is less optimised than it could be. ConfigureAwait(false) just adds some more optimisation around context switching (this depends on your context here so read that link).

This method is the worst of both worlds:

public async Task MyMethod()
{
   await Task.Run(() => Thread.Sleep(10000)); // Simulates long running operation
}

this now releases the main thread (good I guess) but all it does is create a new one (with all the overhead of context switching) which then blocks. So you're still using one thread, this thread is still blocked but now you've just added a load of extra context switching into the mix to slow everything down. So this is very inefficient.

Liam
  • 27,717
  • 28
  • 128
  • 190
  • 3
    The `Thread.Sleep()` was to simulate a long running operation that could not be awaited. – NibblyPig Jan 31 '20 at 10:52
  • It's still relevant as it goes to the heart of the problem, don't use `Thread`, use `Task`. I've re-ordered the points to be more clear @NibblyPig – Liam Jan 31 '20 at 11:00
  • How do you use `Task.Delay` on a blocking function? If you replace Thread.Sleep(10000) with `DoBlockingFunction()`, a method that does not return? – NibblyPig Jan 31 '20 at 11:02
  • You can't. Like I said *"If it is blocking and you can't make it unblocking then don't use async"* – Liam Jan 31 '20 at 11:03
  • If I need to make it async anyway (in my 'catch' block which should never normally run, it logs it by awaiting an async log method), what will the result be of `await MyMethod();`? Will it block the thread and behave pretty much as if I hadn't used any async/await anywhere at all? – NibblyPig Jan 31 '20 at 11:09
  • If `MyMethod` does not `await` the pre-compiler will strip out all the `async`/`await` code out. So it does zip. Adding `async` does not make a method asynchronous, it just tells the compiler that this can be asynchronous. If it runs synchronously then it runs as though this was never there. That's essentially the abstraction that `Task` adds over `Thread` – Liam Jan 31 '20 at 11:10
  • If this was UI for example then, if we `await MyMethod()`, while it's executing the blocking code, the UI would lock up. Once the blocking code is finished, if we assume there is an `await Task.Delay(50000);` on the next line, then once it reaches that line the UI would not be frozen anymore. And then finally when the Delay was completed, the method would return and the code execution would continue. Does that sound right? – NibblyPig Jan 31 '20 at 11:19
  • Not my area of expertise I'm afraid, I'm a web developer. I think, no it would not lock up but you may have to use `Task.Run` to get a thread pool thread in a desktop environment. – Liam Jan 31 '20 at 11:21
  • 1
    Yes, it would lock up in the desktop environment in that scenario. You would need to call it with ``await Task.Run()`` The code in MyMethod will execute synchronously until it encounters the first await – Rowan Smith Jan 31 '20 at 11:41
0

The reason for making the method async is because I want people to be able to easily await so that they don't tie up their thread.

If you absolutely want people to be able to await this and you have completely synchronous code, which you can't change to make it return an awaitable and it's definitely going to block the calling thread and you don't want that, then your only choice is to use Task.Run() (or a derivative of it).

Be aware that this is considered an Anti-Pattern by many. The correct approach is to change your requirement so that it's not required to be awaitable.

public async Task MyMethod()
{
    await Task.Run(() => LongRunningBlockingSyncMethod());
}
Rowan Smith
  • 1,815
  • 15
  • 29