SemaphoreSlim.WaitAsync before/after try block

Question

I know that in the sync world the first snippet is right, but what's about WaitAsync and async/await magic? Please give me some .net internals.

await _semaphore.WaitAsync();
try
{
    // todo
}
finally
{
    _semaphore.Release();
}

or

try
{
    await _semaphore.WaitAsync();
    // todo
}
finally
{
    _semaphore.Release();
}

Answer 1

According to MSDN, SemaphoreSlim.WaitAsync may throw:

1. ObjectDisposedException - If the semaphore has been disposed

2. ArgumentOutOfRangeException - if you choose the overload which accepts an int and it is a negative number (excluding -1)

In both cases, the SemaphoreSlim wont acquire the lock, which makes it unnessacery to release it in a finally block.

One thing to note is if the object is disposed or null in the second example, the finally block will execute and either trigger another exception or call Release which might have not acquired any locks to release in the first place.

To conclude, I would go with the former for consistency with non-async locks and avoiding exceptions in the finally block

Answer 2

If there's an exception inside WaitAsync the semaphore was not acquired, so a Release is unnecessary and should be avoided. You should go with the first snippet.

If you're worried about exceptions in the actual acquiring of the semaphore (which aren't likely, other than NullReferenceException) you could try-catch it independently:

try
{
    await _semaphore.WaitAsync();
}
catch
{
    // handle
}

try
{
    // todo
}
finally
{
    _semaphore.Release();
}

Answer 3

Both options are dangerous if we think about ThreadAbortException.

1. Consider Option 1 and ThreadAbortException happening between WaitAsync and try. In this case a semaphore lock would be acquired but never released. Eventually that would lead to a deadlock.

await _semaphore.WaitAsync();

// ThreadAbortException happens here

try
{
    // todo
}
finally
{
    _semaphore.Release();
}

2. Now in Option 2, if ThreadAbortException happens before a lock has been acquired, we'd still try to release somebody else's lock, or we'd get SemaphoreFullException if the semaphore is not locked.

try
{
    // ThreadAbortException happens here

    await _semaphore.WaitAsync();
    // todo
}
finally
{
    _semaphore.Release();
}

Theoretically, we can go with Option 2 and track whether a lock was actually acquired. For that we're going to put lock acquisition and tracking logic into another (inner) try-finally statement in a finally block. The reason is that ThreadAbortException doesn't interrupt finally block execution. So we'll have something like this:

var isTaken = false;

try
{
    try
    {           
    }
    finally
    {
        await _semaphore.WaitAsync();
        isTaken = true;
    }

    // todo
}
finally
{
    if (isTaken)
    {
        _semaphore.Release();
    }
}

Unfortunately, we're still not safe. The problem is that Thread.Abort locks the calling thread until the aborting thread leaves a protected region (the inner finally block in our scenario). That can lead to a deadlock. To avoid infinite or long-running semaphore awaiting we can interrupt it periodically and give ThreadAbortException a chance to interrupt execution. Now the logic feels safe.

var isTaken = false;

try
{
    do
    {
        try
        {
        }
        finally
        {
            isTaken = await _semaphore.WaitAsync(TimeSpan.FromSeconds(1));
        }
    }
    while(!isTaken);

    // todo
}
finally
{
    if (isTaken)
    {
        _semaphore.Release();
    }
}

Answer 4

Your first option is preferred to avoid calling release in the event that the Wait call threw. Though, with c#8.0 we can write things so that we don't have so much ugly nesting on each our methods requiring the use of a semaphore.

Usage:

public async Task YourMethod() 
{
  using await _semaphore.LockAsync();
  // todo
} //the using statement will auto-release the semaphore

Here's the extension method:

using System;
using System.Threading;
using System.Threading.Tasks;

namespace YourNamespace
{
  public static class SemaphorSlimExtensions
  {
    public static IDisposable LockSync(this SemaphoreSlim semaphore)
    {
      if (semaphore == null)
        throw new ArgumentNullException(nameof(semaphore));

      var wrapper = new AutoReleaseSemaphoreWrapper(semaphore);
      semaphore.Wait();
      return wrapper;
    }

    public static async Task<IDisposable> LockAsync(this SemaphoreSlim semaphore)
    {
      if (semaphore == null)
        throw new ArgumentNullException(nameof(semaphore));

      var wrapper = new AutoReleaseSemaphoreWrapper(semaphore);
      await semaphore.WaitAsync();
      return wrapper;
    }
  }
}

And the IDisposable wrapper:

using System;
using System.Threading;

namespace YourNamespace
{
  public class AutoReleaseSemaphoreWrapper : IDisposable
  {
    private readonly SemaphoreSlim _semaphore;

    public AutoReleaseSemaphoreWrapper(SemaphoreSlim semaphore )
    {
      _semaphore = semaphore;
    }

    public void Dispose()
    {
      try
      {
        _semaphore.Release();
      }
      catch { }
    }
  }
}