0

I am refactoring older synchronous C# code to use an async library. The current synchronous code makes liberal usage of locks. Outer methods often call inner methods, where both lock on the same objects. These are often "protected objects" defined in the base class and locked upon in base virtual methods and the overrides that call the base. For synchronous code, that's ok as the thread entering the outer/override method lock can also enter the inner/base method one. That is not the case for async / SemaphoreSlim(1,1)s.

I'm looking for a robust locking mechanism I can use in the async world that will allow subsequent downstream calls to the same locking object, to enter the lock, as per the behaviour in synchronous "lock {...}" syntax. The closest I have come is semaphore slim, but it is too restrictive for my needs. It restricts access not only to other threads, but to the same thread requesting entrance in the inner call too. Alternatively, is there a way to know that the thread is already "inside" the semaphore before calling the inner SemaphoreSlim.waitasync()?

Answers questioning the design structure of the inner/outer methods both locking on the same object are welcome (I question it myself!), but if so please propose alternative options. I have thought of only using private SemaphoreSlim(1,1)s, and having inheritors of the base class use their own private semaphores. But it gets tricky to manage quite quickly.

Sync Example: Because the same thread is requesting entrance to the lock in both inner and outer, it lets it in and the method can complete.

        private object LockObject = new object();
        
        public void Outer()
        {
            lock (LockObject)
            {
                foreach (var item in collection)
                {
                    Inner(item);
                }
            }

        }

        public void Inner(string item)
        {
            lock (LockObject)
            {
                DoWork(item);
            }
        }

Async Example: The semaphore doesn't work like that, it will get stuck at the first iteration of inner async because it's just a signal, it doesn't let another one pass until it is released, even if the same thread requests it


        protected SemaphoreSlim LockObjectAsync = new SemaphoreSlim(1,1);

        public async Task OuterAsync()
        {
            try
            {
                await LockObjectAsync.WaitAsync();
                foreach (var item in collection)
                {
                    await InnerAsync(item);
                }
            }
            finally
            {
                LockObjectAsync.Release();
            }

        }

        public async Task InnerAsync(string item)
        {
            try
            {
                await LockObjectAsync.WaitAsync();
                DoWork(item);
            }
            finally
            {
                LockObjectAsync.Release();
            }
        }
Adam Diament
  • 4,290
  • 3
  • 34
  • 55
  • 4
    Reentrancy doesn't really make sense in an async context. An asynchronous operation is likely to use many different threads. The point at which you're waiting on the lock again may well be on a different thread than the one that waited on it earlier in the asynchronous operation. Reentrancy like this should generally be avoided even in synchronous code (it usually makes it easier to make mistakes). – Servy Aug 27 '21 at 14:54
  • 2
    It would be *possible* write a recursive async lock -- you can't use thread-local state, but you could use an [AsyncLocal](https://learn.microsoft.com/en-us/dotnet/api/system.threading.asynclocal-1?view=net-5.0) -- but I'm not aware of any that do (maybe Stephen Cleary has written one?). There's often little point: locking is needed far less in async code than in threaded code (although there are still places it is needed), and use of recursive locks has been shown to be a nice source of bugs – canton7 Aug 27 '21 at 15:11
  • 3
    In your example, refactor Inner to have a non-public method which does the work, and a public method which applies the work. The method Output can call the non-public lock-less version. This pushes the use of locks towards the interface of your class, and away from the bowels of the implementation, which makes managing them easier – canton7 Aug 27 '21 at 15:13

1 Answers1

2

I am in full agreement with Servy here:

Reentrancy like this should generally be avoided even in synchronous code (it usually makes it easier to make mistakes).

Here's a blog post on the subject I wrote a while ago. Kinda long-winded; sorry.

I'm looking for a robust locking mechanism I can use in the async world that will allow subsequent downstream calls to the same locking object, to enter the lock, as per the behaviour in synchronous "lock {...}" syntax.

TL;DR: There isn't one.

Longer answer: An implementation exists, but I wouldn't use the word "robust".


My recommended solution is to refactor first so that the code no longer depends on lock re-entrancy. Make the existing code use SemaphoreSlim (with synchronous Waits) instead of lock.

This refactoring isn't extremely straightforward, but a pattern I like to use is to refactor the "inner" methods into private (or protected if necessary) implementation methods that are always executed under lock. I strongly recommend these inner methods follow a naming convention; I tend to use the ugly-but-in-your-face _UnderLock. Using your example code this would look like:

private object LockObject = new();
        
public void Outer()
{
  lock (LockObject)
  {
    foreach (var item in collection)
    {
      Inner_UnderLock(item);
    }
  }
}

public void Inner(string item)
{
  lock (LockObject)
  {
    Inner_UnderLock(item);
  }
}

private void Inner_UnderLock(string item)
{
  DoWork(item);
}

This gets more complex if there are multiple locks, but for simple cases this refactoring works well. Then you can replace the reentrant locks with non-reentrant SemaphoreSlims:

private SemaphoreSlim LockObject = new(1);
        
public void Outer()
{
  LockObject.Wait();
  try
  {
    foreach (var item in collection)
    {
      Inner_UnderLock(item);
    }
  }
  finally
  {
    LockObject.Release();
  }
}

public void Inner(string item)
{
  LockObject.Wait();
  try
  {
    Inner_UnderLock(item);
  }
  finally
  {
    LockObject.Release();
  }
}

private void Inner_UnderLock(string item)
{
  DoWork(item);
}

If you have many of these methods, look into writing a little extension method for SemaphoreSlim that returns IDisposable, and then you end up with using blocks that look more similar to the old lock blocks instead of having try/finally everywhere.


The not-recommended solution:

As canton7 suspected, an asynchronous recursive lock is possible, and I have written one. However, that code has never been published nor supported, nor will it ever be. It hasn't been proven in production or even fully tested. But it does, technically, exist.

Stephen Cleary
  • 437,863
  • 77
  • 675
  • 810
  • Thanks for the great answer Stephen. I didn't know that SemaphoreSlims could be used synchronously, that in itself solves quite a lot of problems. And nice tip on the IDisposable extension, I'll definitely look into that. – Adam Diament Aug 27 '21 at 17:28
  • 1
    Regarding how to `await` the `SemaphoreSlim` with the `using` statement, there are implementations [here](https://stackoverflow.com/questions/24139084/semaphoreslim-waitasync-before-after-try-block/60109694#60109694 "SemaphoreSlim.WaitAsync before/after try block") and [here](http://www.tomdupont.net/2016/03/how-to-release-semaphore-with-using.html "How to Release a Semaphore with a Using Block"). This convenience comes at a price though: an object has to be allocated on each `Wait`/`WaitAsync`, even when there is no contention for acquiring the semaphore. – Theodor Zoulias Aug 27 '21 at 19:14
  • @TheodorZoulias: I also have [an implementation](https://github.com/StephenCleary/AsyncEx/blob/8a73d0467d40ca41f9f9cf827c7a35702243abb8/src/Nito.AsyncEx.Tasks/SemaphoreSlimExtensions.cs) for both sync and async locking. – Stephen Cleary Aug 28 '21 at 12:42
  • Yeap, I realized it afterwards, after getting a compile error "The call is ambiguous between the following methods or properties", while trying to improve Bill Tarbell's [`LockAsync`](https://stackoverflow.com/questions/24139084/semaphoreslim-waitasync-before-after-try-block/60109694#60109694) method. There was a `using Nito.AsyncEx` in the same file too. :-) – Theodor Zoulias Aug 28 '21 at 14:00
  • @StephenCleary should I be able to access a semaphore using both .Wait() and .WaitAsync() from separate void/task methods or is that a recipe for disaster/an unsupported scenario? I ask because I am trying it, and am getting intermittent deadlocks. Refactoring all to async task with waitasync seems to fix the problem. I am wondering if the problem resides elsewhere in my code, or if it is just a problem to do what I am trying to do in the first place. I can add a new question if you prefer. – Adam Diament Mar 14 '22 at 11:41
  • 1
    @AdamDiament: It's supported, but releasing the semaphore for asynchronous waiters requires an available thread pool thread. So on a server where your thread pool is saturated, you can get deadlocks - or technically, delayed unlocks, which on a busy server can be delayed indefinitely and thus be observed as a deadlock. I recommend using `WaitAsync` everywhere if possible. Increasing your thread pool size or horizontal scaling is a temporary workaround. – Stephen Cleary Mar 14 '22 at 12:36
  • @StephenCleary understood! And thanks for the quick response – Adam Diament Mar 14 '22 at 13:02