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?

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.

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.