Two Tasks run on the same thread which invalidates lock

Question

Edit

I find Building Async Coordination Primitives, Part 1: AsyncManualResetEvent might be related to my topic.

In the case of TaskCompletionSource, that means that synchronous continuations can happen as part of a call to {Try}Set*, which means in our AsyncManualResetEvent example, those continuations could execute as part of the Set method. Depending on your needs (and whether callers of Set may be ok with a potentially longer-running Set call as all synchronous continuations execute), this may or may not be what you want.

Many thanks to all of the answers, thank you for your knowledge and patience!

---

Original Question

I know that Task.Run runs on a threadpool thread and threads can have re-entrancy. But I never knew that 2 tasks can run on the same thread when they are both alive!

My Question is: is that reasonable by design? Does that mean lock inside an async method is meaningless (or say, lock cannot be trusted in async method block, if I'd like a method that doesn't allow reentrancy)?

Code:

namespace TaskHijacking
{
    class Program
    {
        static TaskCompletionSource<bool> tcs = new TaskCompletionSource<bool>();
        static object methodLock = new object();

        static void MethodNotAllowReetrance(string callerName)
        {
            lock(methodLock)
            {
                Console.WriteLine($"Enter MethodNotAllowReetrance, caller: {callerName}, on thread: {Thread.CurrentThread.ManagedThreadId}");
                if (callerName == "task1")
                {
                    tcs.SetException(new Exception("Terminate tcs"));
                }
                Thread.Sleep(1000);
                Console.WriteLine($"Exit MethodNotAllowReetrance, caller: {callerName}, on thread: {Thread.CurrentThread.ManagedThreadId}");
            }
        }

        static void Main(string[] args)
        {
            var task1 = Task.Run(async () =>
            {
                await Task.Delay(1000);
                MethodNotAllowReetrance("task1");
            });

            var task2 = Task.Run(async () =>
            {
                try
                {
                    await tcs.Task;  // await here until task SetException on tcs
                }
                catch
                {
                    // Omit the exception
                }
                MethodNotAllowReetrance("task2");
            });

            Task.WaitAll(task1, task2);

            Console.ReadKey();
        }
    }
}

Output:

Enter MethodNotAllowReetrance, caller: task1, on thread: 6
Enter MethodNotAllowReetrance, caller: task2, on thread: 6
Exit MethodNotAllowReetrance, caller: task2, on thread: 6
Exit MethodNotAllowReetrance, caller: task1, on thread: 6

The control flow of the thread 6 is shown in the figure:

Answer 1

Use SemaphoreSlim instead of lock, since, as the documentation says:

The SemaphoreSlim class doesn't enforce thread or task identity

In your case, it would look something like this:

// Semaphore only allows one request to enter at a time
private static readonly SemaphoreSlim _semaphoreSlim = new SemaphoreSlim(1, 1);

void SyncMethod() {
  _semaphoreSlim.Wait();
  try {
    // Do some sync work
  } finally {
    _semaphoreSlim.Release();
  }
}

The try/finally block is optional, but it makes sure that the semaphore is released even if an exception is thrown somewhere in your code.

Note that SemaphoreSlim also has a WaitAsync() method, if you want to wait asynchronously to enter the semaphore.

Answer 2

So your method is basically like this:

static void MethodNotAllowReetrance()
{
    lock (methodLock) tcs.SetResult();
}

...and the tcs.Task has a continuation attached that invokes the MethodNotAllowReetrance. What happens then is the same thing that would happen if your method was like this instead:

static void MethodNotAllowReetrance()
{
    lock (methodLock) MethodNotAllowReetrance();
}

The moral lesson is that you must be very careful every time you invoke any method inside a lock-protected region. In this particular case you have a couple of options:

1. Don't complete the TaskCompletionSource while holding the lock. Defer its completion until after you have exited the protected region:

static void MethodNotAllowReetrance()
{
    bool doComplete = false;
    lock (methodLock) doComplete = true;
    if (doComplete) tcs.SetResult();
}

2. Configure the TaskCompletionSource so that it invokes its continuations asynchronously, by passing the TaskCreationOptions.RunContinuationsAsynchronously in its constructor. This is an option that you don't have very often. For example when you cancel a CancellationTokenSource, you don't have the option to invoke asynchronously the callbacks registered to its associated CancellationToken.

3. Refactor the MethodNotAllowReetrance method in a way that it can handle reentrancy.

Answer 3

You already have several solutions. I just want to describe the problem a bit more. There are several factors at play here that combine to cause the observed re-entrancy.

First, lock is re-entrant. lock is strictly about mutual exclusion of threads, which is not the same as mutual exclusion of code. I think re-entrant locks are a bad idea in the 99% case (as described on my blog), since developers generally want mutual exclusion of code and not threads. SemaphoreSlim, since it is not re-entrant, mutually excludes code. IMO re-entrant locks are a holdover from decades ago, when they were introduced as an OS concept, and the OS is just concerned about managing threads.

Next, TaskCompletionSource<T> by default invokes task continuations synchronously.

Also, await will schedule its method continuation as a synchronous task continuation (as described on my blog).

Task continuations will sometimes run asynchronously even if scheduled synchronously, but in this scenario they will run synchronously. The context captured by await is the thread pool context, and the completing thread (the one calling TCS.TrySet*) is a thread pool thread, and in that case the continuation will almost always run synchronously.

So, you end up with a thread that takes a lock, completes a TCS, thus executing the continuations of that task, which includes continuing another method, which is then able to take that same lock.

To repeat the existing solutions in other answers, to solve this you need to break that chain at some point:

• (OK) Use a non-reentrant lock. SemaphoreSlim.WaitAsync will still execute the continuations while holding the lock (not a good idea), but since SemaphoreSlim isn't re-entrant, the method continuation will (asynchronously) wait for the lock to be available.
• (Best) Use TaskCompletionSource.RunContinuationsAsynchronously, which will force task continuations onto a (different) thread pool thread. This is a better solution because your code is no longer invoking arbitrary code while holding a lock (i.e., the task continuations).

You can also break the chain by using a non-thread-pool context for the method awaiting the TCS. E.g., if that method had to resume on a UI thread, then it could not be run synchronously from a thread pool thread.

From a broader perspective, if you're mixing locks and TaskCompletionSource instances, it sounds like you may be building (or may need) an asynchronous coordination primitive. I have an open-source library that implements a bunch of them, if that helps.

Answer 4

A task is an abstraction over some amount of work. Usually this means that the work is split into parts, where the execution can be paused and resumed between parts. When resuming it may very well run on another thread. But the pausing/resuming may only be done at the await statements. Notably, while the task is 'paused', for example because it is waiting for IO, it does not consume any thread at all, it will only use a thread while it is actually running.

My Question is: is that reasonable by design? Does that mean lock inside an async method is meaningless?

locks inside a async method is far from meaningless since it allows you to ensure a section of code is only run by one thread at a time.

In your first example there can be only one thread that has the lock at a time. While the lock is held that task cannot be paused/resumed since await is not legal while in a lock body. So a single thread will execute the entire lock body, and that thread cannot do anything else until it completes the lock body. So there is no risk of re-entrancy unless you invoke some code that can call back to the same method.

In your updated example the problem occurs due to TaskCompletionSource.SetException, this is allowed to reuse the current thread to run any continuation of the task immediately. To avoid this, and many other issues, make sure you only hold the lock while running a limited amount of code. Any method calls that may run arbitrary code risks causing deadlocks, reentrancy, and many other problems.

You can solve the specific problem by using a ManualResetEvent(Slim) to do the signaling between threads instead of using a TaskCompletionSource.