async await for a single task at a time

Question

During my job interview, I was given a task to create an asynchronous wrapper over some long running method, processing some data, but to create it so that only a single task could be running at a time. I was not very familiar with async/await pattern, so I did my best and wrote some mixup between task-style and event-style, so that my wrapper was holding a task currently being executed, and exposing a public method and a public event. Method took data to process as an argument, and if there was no task running, started one, if there was a task, it enqueued the data. Task was raising the public event upon completion, which was sending process results to subscribers and starting a new task if there is any enqueued.

So, as you could probably guess by that point, I failed an interview, but now that I did some research, I am trying to figure out how to properly do it (it should have also been thread-safe, but I was too busy worrying about that). So my question is, if I have

public class SynchronousProcessor
{
    public string Process(string arg)
    {
        Thread.Sleep(1500); //Work imitation
        return someRandomString;
    }
}

public class AsynchronousWrapper
{
    SynchronousProcessor proc = new SynchronousProcessor();

    public async Task<string> ProcessAsync(string arg)
    {
        return Task.Run(() => proc.Process(arg));
    } 
}

, or something like this, how do I properly handle calls to ProcessAsync(string) if there is already a task executing?

Answer 1

Many job interview questions are asked for a purpose other than to see you write the code. Usually, questions are a bit vague specifically to see what clarifying questions you ask - your questions determine how well you do. Writing code on a whiteboard is secondary at best.

I was given a task to create an asynchronous wrapper over some long running method, processing some data

First question: is this long-running method asynchronous? If so, then there would not be a need for Task.Run. But if not...

Followup question: if it's not asynchronous, should it be? I.e., is it I/O-based? If so, then we could invest the time to make it properly asynchronous. But if not...

Followup question: if we need a task wrapper (around CPU-based code or around blocking I/O code), is the environment agreeable to a wrapper? I.e., is this a desktop/mobile app and not code that would be used in ASP.NET?

create it so that only a single task could be running at a time.

Clarifying questions: if a second request comes in when one is already running, does the second request "queue up"? Or would it "merge" with an existing request? If merging, do they need to "key" off of the input data - or some subset of the input data?

Every one of these questions change how the answer is structured.

exposing a public method and a public event.

This could be what threw it. Between Task<T> / IProgress<T> and Rx, events are seldom needed. They really only should be used if you're on a team that won't learn Rx.

Oh, and don't worry about "failing" an interview. I've "failed" over 2/3 of my interviews over the course of my career. I just don't interview well.

Answer 2

As @MickyD already said, you need to know the Best Practices in Asynchronous Programming to solve such problems right way. Your solution has a code smell as it provide async wrapper with Task.Run for a synchronous code. As you were asked about the library development, it will be quite impacting your library consumers.

You have to understand that asynchronous isn't multithreading, as it can be done with one thread. It's like waiting for a mail - you don't hire a worker to wait by the mailbox.

Other solutions here aren't, well, async, because break other rule for async code: do not block async action, so you should avoid the lock construct.

So, back to your problem: if you face a task which states

only a single task could be running at a time

It is not about the lock (Monitor), it is about Semaphore(Slim). If for some reason in future you'll need to improve your code so more than one task can be executed simultaneously, you'll have to rewrite your code. In case of Semaphore usage you'll need to change only one constant. Also it has an async wrappers for waiting methods

So your code can be like this (note that the Task.Run is removed, as it is a client responsibility to provide an awaitable):

public class AsynchronousWrapper
{
    private static SemaphoreSlim _mutex = new SemaphoreSlim(1);

    public async Task<T> ProcessAsync<T>(Task<T> arg)
    {
        await _mutex.WaitAsync().ConfigureAwait(false);

        try
        {
            return await arg;
        }
        finally
        {
            _mutex.Release();
        }
    }
}