Thread Locking doesn't seem to work consistently

Question

Update (Explanation of output)

I have multiple tasks running that all try to do a job however I only ever want one task to be doing or waiting to do that job.

In one scenario, if the job is in-progress, other tasks that attempt to do that same job will see that it's already in-progress and skip attempting to do it and move on immediately because they have no need to know if or when it actually finishes.

In another scenario, if the job is in-progress, other tasks should wait for the job to finish before continuing but then not attempt to do the job themselves because it was just completed by the task they were waiting on. Once the job is complete, all the tasks that were waiting may move on but none of them should attempt to do that job that was just done.

Original Question

My goal is to create an async Task extension method that will generate results similar to this: (not exactly because SKIP and WAIT-TO-SKIP won't be in exactly the same order every time)

When run with one thread:

[Task 1] LOCKED
[Task 1] WORK-START
[Task 1] WORK-END
[Task 1] UNLOCKED

When run with five threads:

[Task 1] LOCKED
[Task 1] WORK-START
[Task 2] WAIT-TO-SKIP
[Task 3] WAIT-TO-SKIP
[Task 4] WAIT-TO-SKIP
[Task 5] WAIT-TO-SKIP
[Task 1] WORK-END
[Task 1] UNLOCKED
[Task 2] SKIP
[Task 3] SKIP
[Task 4] SKIP
[Task 5] SKIP

It should also have a parameter that allows it to behave like this:

[Task 1] LOCKED
[Task 1] WORK-START
[Task 2] SKIP
[Task 3] SKIP
[Task 4] SKIP
[Task 5] SKIP
[Task 1] WORK-END
[Task 1] UNLOCKED

Here's the method I came up with:

public static async Task FirstThreadAsync(IFirstThread obj, Func<Task> action, TaskCompletionSource? waitTaskSource, string threadName = "")
{
    if (obj.Locked)
    {
        if (waitTaskSource != null && !waitTaskSource.Task.IsCompleted)
        {
            Log.Debug(Logger, $"[{threadName}] WAIT-TO-SKIP");
            await waitTaskSource.Task;
        }
        Log.Debug(Logger, $"[{threadName}] SKIP-1");
        return;
    }
    var lockWasTaken = false;
    var temp = obj;
    try
    {
        if (waitTaskSource == null || waitTaskSource.Task.IsCompleted == false)
        {
            Monitor.TryEnter(temp, ref lockWasTaken);
            if (lockWasTaken) obj.Locked = true;
        }
    }
    finally
    {
        if (lockWasTaken) Monitor.Exit(temp);
    }
    if (waitTaskSource?.Task.IsCompleted == true)
    {
        Log.Debug(Logger, $"[{threadName}] SKIP-3");
        return;
    }
    if (waitTaskSource != null && !lockWasTaken)
    {
        if (!waitTaskSource.Task.IsCompleted)
        {
            Log.Debug(Logger, $"[{threadName}] WAIT-TO-SKIP (LOCKED)");
            await waitTaskSource.Task;
        }
        Log.Debug(Logger, $"[{threadName}] SKIP-2");
        return;
    }
    Log.Debug(Logger, $"[{threadName}] LOCKED");
    try
    {
        Log.Debug(Logger, $"[{threadName}] WORK-START");
        await action.Invoke().ConfigureAwait(false);
        Log.Debug(Logger, $"[{threadName}] WORK-END");
    }
    catch (Exception ex)
    {
        waitTaskSource?.TrySetException(ex);
        throw;
    }
    finally
    {
        obj.Locked = false;
        Log.Debug(Logger, $"[{threadName}] UNLOCKED");
        waitTaskSource?.TrySetResult();
    }
}

Here's the interface and the Example class:

public interface IFirstThread
{
    bool Locked { get; set; }
}

public class Example : IFirstThread
{
    public bool Locked { get; set; }

    public async Task DoWorkAsync(string taskName)
    {
        for (var i = 0; i < 10; i++)
        {
            await Task.Delay(5);
        }
    }
}

Here's a unit test where the Task1 - Task5 are all trying to run at the same time and Task End runs after they all completed:

[TestMethod]
public async Task DoWorkOnce_AsyncX()
{
    var waitTaskSource = new TaskCompletionSource();
    var example = new Methods.Example();
    var tasks = new List<Task>
    {
        FirstThreadAsync(example, example.DoWorkAsync, waitTaskSource, "Task 1"),
        FirstThreadAsync(example, example.DoWorkAsync, waitTaskSource, "Task 2"),
        FirstThreadAsync(example, example.DoWorkAsync, waitTaskSource, "Task 3"),
        FirstThreadAsync(example, example.DoWorkAsync, waitTaskSource, "Task 4"),
        FirstThreadAsync(example, example.DoWorkAsync, waitTaskSource, "Task 5"),
    };

    await Task.WhenAll(tasks);
    await FirstThreadAsync(example, example.DoWorkAsync, waitTaskSource, "Task End"),

    //code to get and compare the output
}

The problem I'm having is that 99% of the time it works as expected but sometimes it allows two threads to run simultaneously, and I can't seem to figure out why or how to stop it. Here's an example of the unit test output from one the rare cases when it allows two threads to run simultaneously:

[Task 5] LOCKED,
[Task 4] WAIT-TO-SKIP,
[Task 2] LOCKED,
[Task 3] WAIT-TO-SKIP,
[Task 1] WAIT-TO-SKIP,
[Task 5] WORK-START,
[Task 2] WORK-START,
[Task 2] WORK-END,
[Task 5] WORK-END,
[Task 5] UNLOCKED,
[Task 2] UNLOCKED,
[Task 1] SKIP-1,
[Task 4] SKIP-1,
[Task 3] SKIP-1,
[Task End] LOCKED,
[Task End] WORK-START,
[Task End] WORK-END,
[Task End] UNLOCKED

As you can see Task 5 and Task 2 both get locked when only one of them should be getting locked. Task End is only part of the test to verify that class is left in an unlocked state when the simultaneous calls are done. Also, the threadName parameter is completely not necessary and only included so I can tell which task is which for testing purposes.

To much code for me but one thing i want to mention. Are you aware that Tasks and Thread aren't the same thing? A Task does not (or must not) map to its own Thread. — Ralf, Nov 04 '22 at 18:24
You're not doing any work while you own the lock on Example. That's why C# has a `lock` keyword so you can simply an reliably do this without messing with the Monitor directly. https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/statements/lock — David Browne - Microsoft, Nov 04 '22 at 18:37
@DavidBrowne-Microsoft he wants to execute an async method and await it. I think await is not allowed in a lock block. — Ralf, Nov 04 '22 at 18:56
@Ralf And there's good reason for it not being allowed. Using `Monitor` method explicitly to subvert that just results in it *not working properly* for exactly the same reason it C# doesn't allow you to use the `Lock` keyword. Monitor is thread affine, and async methods awaiting this are (potentially) running on different threads. — Servy, Nov 04 '22 at 19:17
@Servy I couldn't use lock and had to use monitor because I needed a way to check if the lock was taken without hitting the lock so that I could do the WAIT-TO-SKIP (LOCKED) or SKIP-2 — LorneCash, Nov 04 '22 at 19:23
@Servy Are you saying that's why I'm having this problem or were you just answering Ralf's comment. — LorneCash, Nov 04 '22 at 19:27
@LorneCash You haven't actually stated what you're actually trying to do, you've just posted some code and an expected output and expected people to try to figure out what it's supposed to *actually* do in all situations, so it's not really clear. Your code is clearly not properly using it's locking (as evidenced by merely the fact that it's behavior isn't adequately consistent). As David said, you're almost certainly overcomplicating your code and should be using more sophisticated tools to solve this problem, but how, specifically, would require you to state what your actual problem is. — Servy, Nov 04 '22 at 19:38
@LorneCash The problem is definetly there. You try to use Thread locking mechanisms for Task locking. That may randomly look like its working but it doesn't. — Ralf, Nov 04 '22 at 19:46
*"Once the job is complete, all the tasks that were waiting may move on."* -- This reminds me of this question: [How to make multiple concurrent calls to async function collapse into 1 task?](https://stackoverflow.com/questions/73438513/how-to-make-multiple-concurrent-calls-to-async-function-collapse-into-1-task) — Theodor Zoulias, Nov 04 '22 at 20:17
@Ralf From what you're saying (and the link NineBerry posted in his answer) it does seem that is the problem. By using monitor, I'm forcing unique threads but not unique tasks on the same thread and that's why I'm seeing this error. If I can adapt this logic to use a SemaphoreSlim as stated in the link in NineBerry's answer it sounds like that would fix my problem. — LorneCash, Nov 04 '22 at 20:18

NineBerry · Accepted Answer · 2022-11-04T21:09:40.590

You use Monitor.Exit much too early. When in one thread

Monitor.TryEnter(temp, ref lockWasTaken);

is executed after

if (lockWasTaken) Monitor.Exit(temp);

was executed in another thread, both can have lockWasTaken true.

You don't need the Locked property on the object you use as the protected resource. The Monitor class sets an internal marker in the object directly in an atomic matter. You can simplify your logic a lot by only relying on the Monitor functionality without having your own separate flags.

Also, as noted by another user, we don't see you creating explicit separate threads for the tasks. The tasks can run on the same thread and then Monitor.TryEnter will allow multiple tasks to enter at the same time when they run on the same thread.

See https://softwareengineering.stackexchange.com/questions/340414/when-is-it-safe-to-use-monitor-lock-with-task on why you should use SempahoreSlim instead of Monitor in async programming.

Here is some sample code solving the task with SemaphoreSlim. I don't see the deeper purpose though ;)

private async void button1_Click(object sender, EventArgs e)
{
    TaskCompletionSource? waitTaskSource = null; // or  new TaskCompletionSource();
    var example = new Example();
    var sempahore = new SemaphoreSlim(1);
    var tasks = new List<Task>
    {
        FirstThreadAsync(sempahore, example.DoWorkAsync, waitTaskSource, "Task 1"),
        FirstThreadAsync(sempahore, example.DoWorkAsync, waitTaskSource, "Task 2"),
        FirstThreadAsync(sempahore, example.DoWorkAsync, waitTaskSource, "Task 3"),
        FirstThreadAsync(sempahore, example.DoWorkAsync, waitTaskSource, "Task 4"),
        FirstThreadAsync(sempahore, example.DoWorkAsync, waitTaskSource, "Task 5"),
    };

    await Task.WhenAll(tasks);
    await FirstThreadAsync(sempahore, example.DoWorkAsync, waitTaskSource, "Task End");
}

public static async Task FirstThreadAsync(SemaphoreSlim semaphore, Func<Task> action, TaskCompletionSource? waitTaskSource, string threadName = "")
{
    // Try to acquire lock
    bool lockAcquired = await semaphore.WaitAsync(TimeSpan.Zero);

    if (lockAcquired)
    {
        // Lock acquired -> Do Work
        Debug.WriteLine($"[{threadName}] LOCKED");

        try
        {
            Debug.WriteLine($"[{threadName}] WORK-START");
            await action.Invoke();
            Debug.WriteLine($"[{threadName}] WORK-END");

            semaphore.Release();
            Debug.WriteLine($"[{threadName}] UNLOCKED");
        }
        catch (Exception ex)
        {
            waitTaskSource?.TrySetException(ex);
        }
        finally
        {
            waitTaskSource?.TrySetResult();
        }
    }
    else // No lock acquired
    {
        // When source is specified, await it.
        if(waitTaskSource != null)
        {
            Debug.WriteLine($"[{threadName}] WAIT-TO-SKIP");
            await waitTaskSource.Task;
        }

        Debug.WriteLine($"[{threadName}] SKIP");
    }
}



public class Example
{
    public async Task DoWorkAsync()
    {
        for (var i = 0; i < 10; i++)
        {
            await Task.Delay(5);
        }
    }
}

I have just seen your requirement that the work package should only be done once at all. To achieve that, you can just skip releasing the semaphore.

Fur further discussions we need to know more details on the actual requirements. Your current set of requirements seems a bit strange. Why have multiple tasks but only one work package?

`bool lockAcquired = await semaphore.WaitAsync(TimeSpan.Zero);` -- This is functionally the same with the simpler `bool lockAcquired = semaphore.Wait(TimeSpan.Zero);` — Theodor Zoulias, Nov 04 '22 at 21:16
@TheodorZoulias I've kept await to make clear that one could use a time value > 0 and then have an async wait until the resource is available if needed. — NineBerry, Nov 04 '22 at 21:19
Waiting for more than `TimeSpan.Zero` is not relevant with the scenario presented in the question. Waiting for exactly `TimeSpan.Zero` is a distinguished case. Essentially means not waiting, which contradicts the name of the method. And not waiting asynchronously makes even less sense. I wish that the `SemaphoreSlim` class had a separate API for the operation "either acquire it immediately or not acquire it". — Theodor Zoulias, Nov 04 '22 at 21:30

Thread Locking doesn't seem to work consistently

1 Answers1