SemaphoreSlim extension method for safely handling cancellation and disposal

Question

I have often found myself using a try {semaphore.Wait()} finally {semaphore.Release()} pattern when using a semaphore, so decided I wanted to try and write an extension method to do this instead.

This stems from the problem where I wanted to dispose of my class containing the SemaphoreSlim instance, and was unsure how to safely and elegantly deal with the SemaphoreSlim instance as well. I'm still not sure if this is the right approach:

public static class SemaphoreSlimExtension
{
    public static async Task RunAsync(this System.Threading.SemaphoreSlim semaphore, Func<Task> action, System.Threading.CancellationToken cancellationToken)
    {
        try
        {
            cancellationToken.ThrowIfCancellationRequested();

            try
            {
                await semaphore.WaitAsync(cancellationToken);
                await action();
            }
            finally
            {
                semaphore.Release();
            }
        }
        catch (OperationCanceledException ex)
        {
            if (cancellationToken != ex.CancellationToken || !cancellationToken.IsCancellationRequested)
            {
                throw;
            }
        }
        catch (ObjectDisposedException ex)
        {
            if (!cancellationToken.IsCancellationRequested)
            {
                throw;
            }
        }
    }
}

Example usage:

public class ClassA : IDisposable
{
    private CancellationTokenSource WorkSemaphoreCancellationTokenSource { get; } = new CancellationTokenSource();
    private SemaphoreSlim WorkSemaphore { get; } = new SemaphoreSlim(1, 1);

    public async Task DoWork()
    {
        await WorkSemaphore.RunAsync(() => Task.Delay(5000), WorkSemaphoreCancellationTokenSource.Token);
    }

    public void Dispose()
    {
        WorkSemaphoreCancellationTokenSource.Cancel();
        WorkSemaphore.Dispose();
        WorkSemaphoreCancellationTokenSource.Dispose();
    }
}

Does this seem like a good approach? Is there a better way to safely manage the cancellation and disposal of SemaphoreSlim instances?

Answer 1

Review

• Is there a better way to safely manage the cancellation and disposal of SemaphoreSlim instances?

  You've made a pattern to safely manage completing / aborting a task that is acquiring or has acquired a lock on a semaphore, not to safely manage the cancellation and disposal of SemaphoreSlim instances.

• You should only release the semaphore if you are sure you have acquired a lock. So take await semaphore.WaitAsync(cancellationToken); out the try block (as suggested in the comments).

• You should also provide overloads or equivalent checks that return Task<T> and/or accept a CancellationToken as argument for the task.

• In your exception handlers you might get false negatives, in that it's possible an OperationCanceledException or ObjectDisposedException was raised while condition cancellationToken.IsCancellationRequested was met, but coming from a different source than the cancellationToken. I don't know whether this is a big issue, since cancellation was requested anyway. But you should definately document in which scenarios exceptions could be propagated to the consumer.

• Rather than the catch - if - throw blocks, you could use catch when (condition) blocks. For instance, catch (OperationCanceledException ex) when (cancellationToken == ex.CancellationToken). This is more compact, doesn't require you rethrow yourself and allows you to focus on silently capturing these exceptions on certain conditions. You might want to log something on such errors.

• I'm not convinced this extension method should take the semaphore as source object. To me, the semaphore is more of a utility used to execute these tasks. Maybe a static Helper class would have been the better choice.