SemaphoreSlim.WaitAsync before/after try block
Solution 1
According to MSDN, SemaphoreSlim.WaitAsync
may throw:
ObjectDisposedException
- If the semaphore has been disposedArgumentOutOfRangeException
- if you choose the overload which accepts anint
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
Solution 2
Both options are dangerous if we think about ThreadAbortException
.
- Consider Option 1 and
ThreadAbortException
happening betweenWaitAsync
andtry
. 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();
}
- 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 getSemaphoreFullException
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();
}
}
Solution 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();
}
Solution 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 { }
}
}
}
Related videos on Youtube
sh1ng
Updated on August 28, 2021Comments
-
sh1ng over 2 years
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(); }
-
noseratio almost 10 years
async/await
doesn't change how exceptions behave insideasync
method. It only changes how exceptions are propagated outsideasync
method. So, what was right for "sync word" is still right here. -
Niklas Peter about 6 yearsThis article explains that the same question was considered when implementing
lock() {}
: blogs.msdn.microsoft.com/ericlippert/2009/03/06/…
-
-
i3arnon almost 10 yearsWouldn't a
Release
where aWaitAsync
failed still exit the semaphore once? -
Yuval Itzchakov almost 10 yearsDefinitely would cause a redundant call. Fixed my answer
-
Mooh about 3 yearsWhy would we at all consider handling ThreadAbortException if we are sure that our code does not call Thread.Abort?
-
AndreyCh about 3 yearsYour 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. -
Theodor Zoulias over 2 yearsWhy are you suppressing a possible exception of the
_semaphore.Release();
call? ASemaphoreFullException
occurrence is certainly something that at least should be logged, in order to receive the developer's attention. -
Bill Tarbell over 2 yearsBecause 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.
-
Theodor Zoulias over 2 yearsBill 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. -
Mike Bruno over 2 yearsJust curious; instead of an empty
try
block with work infinally
, why not callWaitAsync()
in thetry
block and then use an emptycatch
? -
AndreyCh over 2 yearsWe wanted to ensure the
isTaken
variable is always updated with the result ofWaitAsync()
invocation. AThreadAbortException
may happen after semaphore lock is acquired, but before the variable gets updated. That's why we put these operations intofinally
block guaranteeing they will not be interrupted. Unlikely to thefinally
block, thetry-catch
can be interrupted by theThreadAbortException
. Hope this answers your question.