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();
}

Curitiba

async/await doesn't change how exceptions behave inside async method. It only changes how exceptions are propagated outside async method. So, what was right for "sync word" is still right here. – Hawkeyed

This article explains that the same question was considered when implementing lock() {}: blogs.msdn.microsoft.com/ericlippert/2009/03/06/… – Sharlasharleen

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

Vagarious

Wouldn't a Release where a WaitAsync failed still exit the semaphore once? – Bruges

Definitely would cause a redundant call. Fixed my answer – Vagarious

Answer 2

Both options are dangerous if we think about ThreadAbortException, an exception that can happen in older .NET Framework code although it's important to note that it won't occur in newer .NET Core code, as Microsoft say: "Even though this type exists in .NET Core and .NET 5+, since Abort is not supported, the common language runtime won't ever throw 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();
    }
}

Averroism

Why would we at all consider handling ThreadAbortException if we are sure that our code does not call Thread.Abort? – Cachucha

Your question is very valid, especially in terms of the changes introduced in .NET Core, where Thread.Abort was superseded with a more safe approach based on CancellationToken. There is a nice thread regarding that: github.com/dotnet/runtime/issues/11369 As for .NET Framework projects, I'd say, considering ThreadAbortException is still valid, since it's hard to foresee all ways where it can be thrown - that includes 3rd party libraries as well as consumers. – Averroism

Just curious; instead of an empty try block with work in finally, why not call WaitAsync() in the try block and then use an empty catch? – Canopy

We wanted to ensure the isTaken variable is always updated with the result of WaitAsync() invocation. A ThreadAbortException may happen after semaphore lock is acquired, but before the variable gets updated. That's why we put these operations into finally block guaranteeing they will not be interrupted. Unlikely to the finally block, the try-catch can be interrupted by the ThreadAbortException. Hope this answers your question. – Averroism

Answer 3

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();
}

Bruges

Answer 4

This is an attempted improvement of Bill Tarbell's LockSync extension method for the SemaphoreSlim class. By using a value-type IDisposable wrapper and a ValueTask return type, it is possible to reduce significantly the additional allocations beyond what the SemaphoreSlim class allocates by itself.

public static ReleaseToken Lock(this SemaphoreSlim semaphore,
    CancellationToken cancellationToken = default)
{
    semaphore.Wait(cancellationToken);
    return new ReleaseToken(semaphore);
}

public static async ValueTask<ReleaseToken> LockAsync(this SemaphoreSlim semaphore,
    CancellationToken cancellationToken = default)
{
    await semaphore.WaitAsync(cancellationToken).ConfigureAwait(false);
    return new ReleaseToken(semaphore);
}

public readonly struct ReleaseToken : IDisposable
{
    private readonly SemaphoreSlim _semaphore;
    public ReleaseToken(SemaphoreSlim semaphore) => _semaphore = semaphore;
    public void Dispose() => _semaphore?.Release();
}

Usage example (sync/async):

using (semaphore.Lock())
{
    DoStuff();
}

using (await semaphore.LockAsync())
{
    await DoStuffAsync();
}

The synchronous Lock is always allocation-free, regardless of whether the semaphore is acquired immediately or after a blocking wait. The asynchronous LockAsync is also allocation-free, but only when the semaphore is acquired synchronously (when it's CurrentCount happens to be positive at the time). When there is contention and the LockAsync must complete asynchronously, 144 bytes are allocated additionally to the standard SemaphoreSlim.WaitAsync allocations (which are 88 bytes without CancellationToken, and 497 bytes with cancelable CancellationToken as of .NET 5 on a 64 bit machine).

From the docs:

The use of the ValueTask<TResult> type is supported starting with C# 7.0, and is not supported by any version of Visual Basic.

readonly structs are available beginning with C# 7.2.

Also here is explained why the IDisposable ReleaseToken struct is not boxed by the using statement.

Note: Personally I am not a fan of (mis)using the using statement for purposes other than releasing unmanaged resources.

Devisor

Answer 5

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 { }
    }
  }
}

Hassett

Why are you suppressing a possible exception of the _semaphore.Release(); call? A SemaphoreFullException occurrence is certainly something that at least should be logged, in order to receive the developer's attention. – Devisor

Because there's not enough context in the sample to meaningfully handle the exception. The reader may want to simply log it, destroy all instances that made use of the semaphore, or perhaps fully terminate their app. The choice is theirs - this is only a succinct sample to demonstrate the concept. – Hassett

Bill my understand is that the AutoReleaseSemaphoreWrapper is library-worthy code. It's not a piece of application code, intended to be customized to the special needs of the application at hand. So IMHO the general rules about handling exceptions apply: If you don't know how to handle it, let the exception propagate. – Devisor

Answer 6

This is a mix of an answer and a question.

From an article about lock(){} implementation:

The problem here is that if the compiler generates a no-op instruction between the monitor enter and the try-protected region then it is possible for the runtime to throw a thread abort exception after the monitor enter but before the try. In that scenario, the finally never runs so the lock leaks, probably eventually deadlocking the program. It would be nice if this were impossible in unoptimized and optimized builds. (https://blogs.msdn.microsoft.com/ericlippert/2009/03/06/locks-and-exceptions-do-not-mix/)

Of course, lock is not the same, but from this note we could conclude, that it might also be better to put the SemaphoreSlim.WaitAsync() inside the try block, if it also offered a way to determine if the lock was acquired successfully (as Monitor.Enter as described in the article does). However, SemaphoreSlim does not offer such a mechanism.

This article about the implementation of using says:

using (Font font1 = new Font("Arial", 10.0f)) 
{
    byte charset = font1.GdiCharSet;
}

is transformed to:

{
  Font font1 = new Font("Arial", 10.0f);
  try
  {
    byte charset = font1.GdiCharSet;
  }
  finally
  {
    if (font1 != null)
      ((IDisposable)font1).Dispose();
  }
}

If a noop can be generated between a Monitor.Enter() and its immediately following try, wouldn't the same problem apply to the transformed using code, either?

Maybe this implementation of AsyncSemaphore https://github.com/Microsoft/vs-threading/blob/81db9bbc559e641c2b2baf2b811d959f0c0adf24/src/Microsoft.VisualStudio.Threading/AsyncSemaphore.cs

and extensions for SemaphoreSlim https://github.com/StephenCleary/AsyncEx/blob/02341dbaf3df62e97c4bbaeb6d6606d345f9cda5/src/Nito.AsyncEx.Coordination/SemaphoreSlimExtensions.cs

are also interesting.

Sharlasharleen