Is making long-running calls async this simple?

Question

I've just, for a rough draft, converted a whole lot of data access methods to async using the following pattern, and it looks too simple to be good enough for later iterations. How safe is it, what is missing, and how should I be doing it?

The service that provides long-running calls:

private class UserService
{
    public IdentityUser GetById(int id)
    {
        ...
    }
}

private UserService _userService = new UserService();

The original synchronous method:

public IdentityUser GetById(int id)
{
    return _userService.GetById(id);
}

My fantastic new async method:

public async Task<IdentityUser> GetByIdAsync(int id)
{
    await Task.Run(() => _userService.GetById(id));
}

Answer 1

You should not make "fake async" methods like this:

public async Task<IdentityUser> GetByIdAsync(int id)
{
    await Task.Run(() => _userService.GetById(id));
}

The reason I call it "fake async" is because there is nothing intrinsically async about the operation. In this case, you should have only the synchronous method. If a caller wants to make it async by using Task.Run, he can do that.

When is something intrinsically async? When you make a request to a web service or a database, for example, between sending the request and receiving the response there is a waiting period - the request is an intrinsically async operation. To avoid blocking the calling thread, use async-await.

Answer 2

Technically, that works, but it works by creating a new thread to perform a synchronous operation, which itself is wrapping and blocking on an inherently asynchronous operation. That means you're not getting some of the biggest benefits of going async in the first place.

The right way is to go asynchronous all the way. Whereas right now you probably have something like this:

private class UserService
{
    public IdentityUser GetById(int id)
    {
        return mContext.Users.Single(u => u.Id == id);
    }
}

... you should now create an async version:

private class UserService
{
    public async Task<IdentityUser> GetByIdAsync(int id)
    {
        return await mContext.Users.SingleAsync(u => u.Id == id);
    }
}

Usage:

public async Task<IdentityUser> GetByIdAsync(int id)
{
    return await _userService.GetByIdAsync(id);
}

Supposing, of course, that your underlying framework supports asynchronous methods like SingleAsync() for inherently asynchronous operations, this will allow the system to release the current thread while you wait for the database operation to complete. The thread can be reused elsewhere, and when the operation is done, you can use whatever thread happens to be available at that time.

It's probably also worth reading about and adopting these Best Practices. You'll probably want to use .ConfigureAwait(false) anywhere that you're not accessing contextual information like sessions and requests, for example.

This answer assumes, of course, that GetById is inherently asynchronous: you're retrieving it from a hard drive or network location or something. If it's calculating the user's ID using a long-running CPU operation, then Task.Run() is a good way to go, and you'll probably want to additionally specify that it's a long-running task in the arguments to Task.Run().

Answer 3

Task.Run () should only be used for CPU-bound work. Don't quite remember why though. Try to make a GetByIdAsync () method instead, that ultimately calls an async resource.