Is using Task.Run a bad practice?

Question

Generally, when implementing an asynchronous method on a class, I write something like this:

public Task<Guid> GetMyObjectIdAsync(string objectName)
{
    return Task.Run(() => GetMyObjectId(objectName));
}

private Guid GetMyObjectId(string objectName)
{
    using (var unitOfWork = _myUnitOfWorkFactory.CreateUnitOfWork())
    {
        var myObject = unitOfWork.MyObjects.Single(o => o.Name == objectName);
        return myObject.Id;
    }
}

This sort of pattern allows me to use the same logic synchronously and asynchronously, depending on the situation (most of my work is in an old code base, not a lot supports async calls yet), as I could expose the synchronous method publicly and get maximum compatibility if I need to.

Recently I've read several SO posts that suggest using Task.Run() is a bad idea, and should only be used under certain circumstances, but those circumstances did not seem very clear.

Is the pattern I've depicted above actually a bad idea? Am I losing some of the functionality/ intended purpose of async calls doing it this way? Or is this a legit implementation?

Answer 1

What you are doing is offloading a synchronous operation to another thread. If your thread is "special" then that's perfectly fine. One example of a "special" thread is a UI thread. In that case you may want to offload work off of it to keep the UI responsive (another example is some kind of listener).

In most cases however you're just moving work around from one thread to another. This doesn't add any value and does add unnecessary overhead.

So:

Is the pattern I've depicted above actually a bad idea?

Yes, it is. It's a bad idea to offload synchronous work to the ThreadPool and pretend as if it's asynchronous.

Am I losing some of the functionality/ intended purpose of async calls doing it this way?

There's actually nothing asynchronous about this operation to begin with. If your executing this on a remote machine and you can benefit from doing it asynchronously the operation itself needs to be truly asynchronous, meaning:

var myObject = await unitOfWork.MyObjects.SingleAsync(o => o.Name == objectName);

What you're currently doing is called "async over sync" and you probably shouldn't do it. More in Should I expose asynchronous wrappers for synchronous methods?