Second thread enters lock before first thread releases it

Question

I'm playing with some threading constructs in C#, and ran into something that defies my understanding of how locking works. I have a helper function that accepts an asynchronous task, and uses a TaskCompletionSource member to try and synchronize access when called multiple times.

public static void Main(string[] args)
{
    var test = new TestClass();
    var task1 = test.Execute("First Task", async () => await Task.Delay(1000));
    var task2 = test.Execute("Second Task", async () => await Task.Delay(1000));

    task1.Wait();
    task2.Wait();

    Console.ReadLine();
}

class TestClass : IDisposable
{
    private readonly object _lockObject = new object();
    private TaskCompletionSource<bool> _activeTaskCompletionSource;

    public async Task Execute(string source, Func<Task> actionToExecute)
    {
        Task activeTask = null;
        lock (_lockObject)
        {
            if (_activeTaskCompletionSource != null)
            {
                activeTask = _activeTaskCompletionSource.Task;
            }
            else
            {
                _activeTaskCompletionSource = new TaskCompletionSource<bool>();
            }
        }

        while (activeTask != null)
        {
            await activeTask;
            lock (_lockObject)
            {
                if (_activeTaskCompletionSource != null)
                {
                    activeTask = _activeTaskCompletionSource.Task;
                }
                else
                {
                    activeTask = null;
                }
            }
        }

        await actionToExecute();
        lock (_lockObject)
        {
            _activeTaskCompletionSource.SetResult(true);
            _activeTaskCompletionSource = null;
        }
    }
}

This always ends up falling into an infinite loop for the second task. I put some code to log each step as it happens, and it always produces something like this (I've manually inserted comments after #s):

[First Task] Waiting for lock (setup)
[First Task] Entered lock (setup)
[First Task] Grabbing '_activeTaskCompletionSource' (setup)
[First Task] Lock released (setup)
[First Task] RUNNING ...
[Second Task] Waiting for lock (setup)
[Second Task] Entered lock (setup)
[Second Task] Assigning 'activeTask' (setup)
[Second Task] Lock released (setup)
[Second Task] Waiting for task to complete ...
[First Task] COMPLETED!
[First Task] Waiting for lock (cleanup)
[First Task] Entered lock (cleanup)
[First Task] Setting _activeTaskCompletionSource result ...
    # Never gets to '_activeTaskCompletionSource = null'
    # Never gets to 'Releasing lock (cleanup)' for first task
[Second Task] Awaited task completed!
[Second Task] Waiting for lock (loop)
    # Immediately enters lock after 'await' is complete
    # Does not wait for 'First Task' to finish its lock!
[Second Task] Entered lock (loop)
[Second Task] Assigning 'activeTask' (loop)
[Second Task] Lock released (loop)
[Second Task] Waiting for task to complete ...
[Second Task] Awaited task completed!

This ultimately sends the second task into an infinite loop, because _activeTaskCompletionSource is never set back to null.

I was under the impression that no other thread could ever enter a lock until all previous threads had exited it, but here, my First Task thread never finishes and releases its cleanup lock before the Second Task thread grabs hold of it.

Does this have anything to do with mixing locks and async/await?

It is definately not recommended to make a task.Wait() or task.Result to an async task, since it could easily end up in a deadlock case. — Osman Esen, Dec 15 '15 at 17:48
@OsmanEsen The `task.Wait()` calls in this example are only used because it's running in a console application, where it is _impossible_ to use async/await in the main function. I've avoided using blocking calls in all of the async functions themselves, and all throughout the `TestClass` implementation. — KChaloux, Dec 15 '15 at 18:23

score 3 · Accepted Answer · answered Dec 15 '15 at 17:05

3

Calling TaskCompletionSource.SetResult can inline continuations causing unexpected and arbitrary code to run under the lock. await uses continuations, too.

This nasty behavior is a design bug in the TPL. If you care about this there is a GitHub issue. Leave a comment there.

answered Dec 15 '15 at 17:05

boot4life

4,966
7
25
47

Specifically if you are on .NET 4.6 you can work around it by changing your creation of the completion source to `_activeTaskCompletionSource = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously);` – Scott Chamberlain Dec 15 '15 at 17:18
Ouch! That's painful. I assumed it was something I had done wrong, not something bugged in the framework itself. Thanks for the info. – KChaloux Dec 15 '15 at 18:04

score 2 · Answer 2 · edited May 23 '17 at 10:27

As boot4Life points out, this is a design bug with the framework that allows continuations from SetResult to run on the same thread.

To work around it if you are on .NET 4.6 change your creation of the completion source to

_activeTaskCompletionSource = new TaskCompletionSource<bool>(TaskCreationOptions.RunContinuationsAsynchronously);

This stops it from happening. If you are only on 4.5.x you must run the completion on a new thread to prevent it from being picked up.

    lock (_lockObject)
    {
        var completionSource = _activeTaskCompletionSource;
        _activeTaskCompletionSource = null;
        Task.Run(() => completionSource.SetResult(true));            
    }

Unfortunately I am stuck on 4.5, so it looks like your workaround for running it on its own thread is going to be required. — KChaloux, Dec 15 '15 at 18:04

Christian Findlay · Answer 3 · 2018-09-20T00:52:24.903

Try SemaphorSlim. The lock statement is not the right tool for the job with the task based async scenario.

Also, there is a temptation to await tasks that are already going on in a class externally, but my understanding is that if you do that, they will not be awaited unless you use ConfigureAwait correctly. Semaphore slim solves this problem because as far as I know, it will await the lock no matter what.

    private readonly SemaphoreSlim _RefreshLock = new SemaphoreSlim(1);

    public virtual async Task RefreshAsync()
    {
        try
        {
            await _RefreshLock.WaitAsync();

            //Your work here

        }
        finally
        {
            _RefreshLock.Release();
        }
    }

Thanks, I wasn't aware this existed. I'll have to check it out for future async/await stuff. — KChaloux, Sep 19 '18 at 13:40

Second thread enters lock before first thread releases it

3 Answers3