3

While developing some asynchronous code my colleagues and I came across a strange problem with one portion of our code. A component was triggered at a continuous rate, but we wanted one of its dependencies that it called to never be executing more than one task at a time.

We decided to just cache the task, no problem. We threw a lock around the assignment of the cached task and had the singleton task remove itself once it completed.

Task onlyOneTask;

public Task TryToBeginSomeProcess()
{
    lock (someLock)
    {
        if (onlyOneTask == null)
        {
            onlyOneTask = DoSomethingButOnlyOneAtATime();
        }

        return onlyOneTask;
    }
}

public async Task DoSomethingButOnlyOneAtATime()
{
    // do some work

    await SomeWork();

    lock (someLock)
    {
        onlyOneTask = null;
    }
}

So, this actually seems to work... assuming that something in the SomeWork() call actually yields. However, if some work looks like this:

public Task SomeWork()
{
    return Task.FromResult(0);
}

It fails! Subtly so, however. The line that clears the onlyOneTask executes BEFORE the task is assigned! Afterwards there is always a task instance in onlyOneTask and it never re-executes the work.

You might think that this should deadlock, but threads may reacquire locks, and since asynchronous code may downgrade itself to fully synchronous code, the second lock is acquired on the same thread. No deadlock.

If SomeWork yields at some point in its execution then it seems to behave as we expect, but it does raise some questions about its exact behavior. I am unsure about the exact details of how threads and locks interact when asynchronous code executes synchronously versus asynchronously.

Another thing I'd like to point out is that this could seemingly be fixed by the usage of Task.Run to force the work to run on its own task. There's a major problem with this: This code was intended to run in a .NET service, and it's bad form to use Task.Run there.

One way to seemingly fix this is if there were some way to force a Task to always behave like it had an asynchronous implementation. I imagine that it doesn't exist, but I might as well ask.

So, onto the question asking:

  1. Can this scenario deadlock even if SomeWork() yields?
  2. Can the asynchronous case behave like the synchronous case (clear onlyOneTask before it is set) if the TaskScheduler continues on the same thread and SomeWork() is fast enough?
  3. Is there a way to force an existing Task to always behave as though it were asynchronous without compromising the thread pool?
  4. Is there a better alternative?

For what it's worth we found if we just inspected the status of the cached task to determine when it was appropriate to initiate SomeWork() again our issues disappeared, so this is mostly academic in nature.

Community
  • 1
  • 1
Bill Nadeau
  • 702
  • 1
  • 9
  • 15

2 Answers2

4

The problem is when the await hits your function returns with the incomplete task and exits the lock allowing more things to run the task. You normally would solve it by awaiting inside of the lock, but the following code will not compile

public async Task TryToBeginSomeProcess()
{
    lock (someLock)
    {
        if (onlyOneTask == null)
        {
            onlyOneTask = DoSomethingButOnlyOneAtATime();
        }
        //does not compile
        await onlyOneTask;
    }
}

The way we fix it is by switching from lock to SemaphoreSlim with a InitialCount of 1 and using its WaitAsync() method.

private SemaphoreSlim someLock = new SemaphoreSlim(1);

public async Task TryToBeginSomeProcess()
{
    try
    {
        await someLock.WaitAsync();

        await DoSomethingButOnlyOneAtATime().ConfigureAwait(false);
    }
    finally
    {
        someLock.Release();
    }
}

public async Task DoSomethingButOnlyOneAtATime()
{
    // do some work

    await SomeWork();

    // do some more work
}

UPDATE: The previous code did not replicate the old behavior, the original if you called the function 20 times it would return the single instance from the 20 calls and only run the code once. My code will run the code 20 times but only one at a time.

This solution really is just a async version of StriplingWarrior's answer, I also create a extension method to make adding in the Task.Yeild() easier.

SemaphoreSlim someLock = new SemaphoreSlim(1);
private Task onlyOneTask;

public async Task TryToBeginSomeProcess()
{
    Task localCopy;
    try
    {
        await someLock.WaitAsync();

        if (onlyOneTask == null)
        {
            onlyOneTask = DoSomethingButOnlyOneAtATime().YeildImmedeatly();
        }
        localCopy = onlyOneTask;
    }
    finally
    {
        someLock.Release();
    }

    await localCopy.ConfigureAwait(false);
}

public async Task DoSomethingButOnlyOneAtATime()
{
    //Do some synchronous work.

    await SomeWork().ConfigureAwait(false);

    try
    {
        await someLock.WaitAsync().ConfigureAwait(false);
        onlyOneTask = null;
    }
    finally
    {
        someLock.Release();
    }
}


//elsewhere
public static class ExtensionMethods
{
    public static async Task YeildImmedeatly(this Task @this)
    {
        await Task.Yield();
        await @this.ConfigureAwait(false);
    }
    public static async Task<T> YeildImmedeatly<T>(this Task<T> @this)
    {
        await Task.Yield();
        return await @this.ConfigureAwait(false);
    }
}
Community
  • 1
  • 1
Scott Chamberlain
  • 124,994
  • 33
  • 282
  • 431
  • This isn't quite the same functionality as the code sample above (if I'm reading it correctly). In the question's example, if I call TryToBeginSomeProcess 100 times, but all of them are called before DoSomethingButOnlyOneAtATime completes, I will only execute DoSomething once. In your example, I will always call DoSomething 100 times. Having said that, I wasn't aware of this usage of SemaphoreSlim, which seems like it would be able to sort out the issues. – Bill Nadeau Jun 17 '16 at 13:44
  • @BillNadeau You are correct, I totally did not re-create the behavior. Give me a minute and I might be able to tweak it. – Scott Chamberlain Jun 17 '16 at 13:46
  • @BillNadeau make sure you get the latest version, i had a version up for a while that had some potential race condition issues. – Scott Chamberlain Jun 17 '16 at 15:04
3

Scott's answer is probably the best way to do what you're trying to do. However, as a general answer to the question of how to invoke async methods and ensure that they do not return synchronously, there's a handy little method called Task.Yield():

public async Task DoSomethingButOnlyOneAtATime()
{
    // Make sure that the rest of this method runs after the
    // current synchronous context has been resolved.
    await Task.Yield();
    // do some work
    await SomeWork();

    lock (someLock)
    {
        onlyOneTask = null;
    }
}
StriplingWarrior
  • 151,543
  • 27
  • 246
  • 315