Calling an async method using a Task.Run seems wrong?

Question

I recently came across this code written by a contractor we had working for us. It's either devilishly clever or silly (I think the latter but I wanted a second opinion). I'm not massively up to speed on async await.

Basically it worked like this:

public bool Send(TemplatedMessageDto message)
{
    return Task.Run(() => SendAsync(message))
        .GetAwaiter()
        .GetResult();
}

public async Task<bool> SendAsync(TemplatedMessageDto message)
{
    //code doing stuff
    var results = await _externalresource.DothingsExternally();
    //code doing stuff
}

Now as I understand it that first Task.Run() is pointless and inefficient? and should really be:

public bool Send(TemplatedMessageDto message)
{
    return SendAsync(message))
    .GetAwaiter()
    .GetResult();
}

public async Task<bool> SendAsync(TemplatedMessageDto message)
{
    //code doing stuff
    var results = await _externalresource.DothingsExternally();
    //code doing stuff
}

I'm also not convinced this is really an async method because it will still wait, right? I think it's only advantage (even re-written) is to free up the main worker thread.

Can someone confirm that this first Task shouldn't be there?

Spiroid

Why the GetAwaiter().GetResult() call instead of .Result? And why wait at all instead of writing public async Task<bool> Send ... await Task.Run(); ? – Fausta

If the "other stuff" is heavy enough Task.Run isn't inefficient - async doesn't make anything run asynchronously. Everything up to await will run on the calling thread. If you need it to run concurrently, you should use Task.Run, or better yet, extract it to its own method that will be called with Task.Run – Fausta

@PanagiotisKanavos, well, yes, this did cross my mind. I don't think he really understood what he was doing. – Spiroid

Do you actually need the synchronous version? – Centesimo

Answer 1

I'm also not convinced this is really an async method because it will still wait, right?

It isn't, as Yuval explained. You shouldn't use sync over async.

Now as I understand it that first Task.Run() is pointless and inefficient?

Not really, there is value in using Task.Run in such a way.

Since you're blocking on an async method (which you shouldn't do) there is a chance you'll deadlock. That happens in UI apps and asp.net where you have a SynchronizationContext.

Using Task.Run clears that SynchronizationContext since it offloads the work to a ThreadPool thread and removes the risk for a deadlock.

So, blocking is bad, but if you end up doing it using Task.Run is safer.

Glimp

Could you clarify the blocking on an async method (which you shouldn't do) bit. I'm a bit confused now. The _externalresource is a third party library that return Task<List<value>> and this method (my Send() method) just needs to confirm that that list contains values. I was going to just change this to a syncohonous method and use the .Result, but I think your saying that's a bad idea? – Spiroid

@Spiroid Blocking on asynchronous methods is usually a bad idea. It hurts performance and can lead to deadlocks. It's much preferable to make your entire flow async. – Glimp

@Spiroid If you must have a synchronous option it's better if you use synchronous calls all the way (i.e. var results = _externalresource.DothingsExternally()) but if this doesn't exist then sync over async is inevitable. – Glimp

Maybe it's the library that's silly then. I think I may post a follow on question. Thanks for your help. – Spiroid

I have posted a follow on question if your interested? – Spiroid

Answer 2

I'm also not convinced this is really an async method because it will still wait, right?

What your contractor did is use the sync over async anti-pattern. He probably did so to save himself from creating an additional method which does its work synchronously. He is unnecessarly invoking Task.Run and immediately blocking on it using GetResult.

Using GetAwaiter().GetResult() will propogate the inner exception, if such happens, instead of the wrapped AggregateException.

I think it's only advantage (even re-written) is to free up the main worker thread.

Both your version and his will block the main thread while executing, while his will also do so by using a threadpool thread. As Bar mentioned, this can help avoid deadlocks with issues regarding to synchronization context marshaling. I would recommend creating a synchronous equivalent if needed.

Tuneberg