How to wrap a legacy synchronous I/O bound operation with an asynchronous method?

Question

I'm still learning async/await concepts so please bear with me. Suppose there is this legacy code that defines a contract for getting data from a data source (local database in the real case):

public interface IRepository<T> {
    IList<T> GetAll();
}

In new code I have the following interface:

public interface IMyObjectRepository {
    Task<IEnumerable<MyObject>> GetAllAsync();
}

The MyObjectRepository class depends on IRepository<T> like this:

public class MyObjectRepository : IMyObjectRepository
{
    private readonly IRepository<Some.Other.Namespace.MyObject> repo_;

    private readonly IDataMapper<Some.Other.Namespace.MyObject, MyObject> mapper_;

    public BannerRepository(IRepository<Some.Other.Namespace.MyObject> repo, IDataMapper<Some.Other.Namespace.MyObject, MyObject> mapper)
    {
        repo_ = repo;
        mapper_ = mapper;
    }
}

How do I implement the IMyObjectRepository.GetAllAsync() method in such a way that it makes sense in the context of asynchronous programming? Getting data from some local data source is an I/O bound operation, so the way I did it is:

public async Task<IEnumerable<MyObject>> GetAllAsync()
{
    var tcs = new TaskCompletionSource<IEnumerable<MyObject>>();

    try
    {
        GetAll(objects =>
        {
            var result = new List<MyObject>();

            foreach (var o in objects)
            {
                result.Add(mapper_.Map(o));
            }

            tcs.SetResult(result);
        });
    }
    catch (Exception e)
    {
        tcs.SetException(e);
    }

    return await tcs.Task;
}

private void GetAll(Action<IEnumerable<Some.Other.Namespace.MyObject>> handle)
{
    IEnumerable<Some.Other.Namespace.MyObject> objects = repo_.GetAll<Some.Other.Namespace.MyObject>();

    if (objects is null)
    {
        objects = Enumerable.Empty<Some.Other.Namespace.MyObject>();
    }

    handle(objects);
}

Does this make any sense? I did not want to use Task.Run() because, the way I understand it, that wastes a thread for nothing in the case of an I/O bound operation which makes sense.

Do you mean `Task.FromResult()`? I thought about that, what is the difference with using `TaskCompletionSource`? — vladek, Jun 17 '21 at 09:28
```Task.FromResult()``` is just a shortcut for creating a ```TaskCompletionSource ``` and executing ```SetResult```. — devsmn, Jun 17 '21 at 09:30
@mjwills So you're saying my `GetAllAsync()` method will be synchronous, meaning it will block the thread it's being called on? If I have: `_ = repo.GetAllAsync()` that will block the thread right? — vladek, Jun 17 '21 at 11:09
Because your `repo_.GetAll` inside your `GetAll` method is synchronous that's why you are not taking advantage of async I/O operations. `Task.FromResult` just wraps your result into a `Task`. — Peter Csala, Jun 17 '21 at 12:19

score 4 · Answer 1 · answered Jun 21 '21 at 00:47

Do not implement a synchronous method with an asynchronous implementation. Also do not implement a fake-asynchronous method (an asynchronous method that just runs synchronous code on a thread pool thread). Instead, first take a step back...

How do I implement the IMyObjectRepository.GetAllAsync() method in such a way that it makes sense in the context of asynchronous programming?

Wrong question.

Instead of approaching async by saying "how do I make this async", approach it from the other end. Start with things that should be async. As you noted, I/O is perfect for async code. So start with the code that actually does the I/O - the lowest-level method. This could be IRepository<T>.GetAll - whatever actually does the communication with the database. Change that to call the async equivalent with await, and allow async to grow from the lowest-level method out to the rest of your code.

This is what I suspected, there's no way around it, it needs to be done at the lowest level. Thank you — vladek, Jun 22 '21 at 11:12

score 1 · Accepted Answer · answered Jun 17 '21 at 12:46

The approach in the example is a valid one. I would however be careful to only use it when absolutely needed, since users might get very confused when calling the "async" method, only for it to block. In that sense it might be preferable to use Task.Run() since that will not block. But it does require the called code to be threadsafe.

In your example there is no point in using async, just remove that and the await should give the exact same result, since you are returning a task anyway.

Using Task.FromResult is another easy alternative. But it will not be quite identical if exception occurs. Using a TaskCompletionSource and .SetException will make exceptions occur as a property of the Task.Exception rather than raised from the method call itself. If the task is awaited it should not matter, since both cases should be handled by a try/catch. But there are other ways to handle exceptions for tasks, so you need to be aware how it is used, and provide appropriate documentation/comments to describe the behavior.

Right, for some reason I had this misconception that if it is `async` it can't block. But in my case it truly is a synchronous method, just marked as `async` and it doesn't take advantage at all of asynchronous I/O - in this case, as you said, I should probably use `Task.Run()` (and yes the code is thread safe). — vladek, Jun 17 '21 at 13:47
If this is a server application (ASP.NET) using `Task.Run` might be just the worst thing you can do. You'll be using 2 thread pool threads instead of 1. — Paulo Morgado, Jun 17 '21 at 17:07

How to wrap a legacy synchronous I/O bound operation with an asynchronous method?

2 Answers2