Summary
I have a class that uses lock to provide thread-safe access to a private field. However, for reasons detailed below I'm considering switching to using SemaphoreSlim to implement thread-safety. I know that if I surround all access to this field with lock (_lock) (as I currently do), I'm guaranteed that writes to the field will be atomic and that all threads will see the most recently-written value (source). I'm wondering if I get both of these guarantees with SemaphoreSlim, or only the guarantee about atomic writes.
Details
I have an interface I wrote a while back to represent services that can be used for communicating with external devices:
public interface ICommunicationService : IDisposable
{
event EventHandler<ReceivedMessageEventArgs> MessageReceived;
void SendMessage(GenericMessage message);
void Start();
}
I've recently come across a situation where I'd prefer to have an async implementation of these methods, and apparently when you start doing async you need to sort of go the whole hog with it to avoid deadlocks, etc., so I'm planning on updating the interface and all existing implementations that I've written to support asynchronous operation:
public interface ICommunicationService : IDisposable
{
event EventHandler<ReceivedMessageEventArgs> MessageReceived;
Task SendMessage(GenericMessage message);
Task Start();
}
One of my classes that implemented this interface did a bunch of locking in its implementations of SendMessage() and Start():
public virtual void SendMessage(GenericMessage message)
{
CheckDisposed();
lock (_lock)
{
if (CommunicationState != CommunicationState.Up)
{
throw new InvalidOperationException("Message cannot be sent as communication is not established.");
}
try
{
_currentDelegate.SendMessage(message);
}
catch (Exception)
{
TerminateCommunication();
}
}
}
Trying to switch this code to an async implementation, I noticed I couldn't await a task within a lock statement. According to async guru Stephen Cleary's blog post on the subject, the async way of doing this is to use SemaphoreSlim.
Based on that post, I've changed my SendMessage implementation to this:
public virtual async Task SendMessage(GenericMessage message)
{
CheckDisposed();
if (CommunicationState != CommunicationState.Up)
{
throw new InvalidOperationException("Message cannot be sent as communication is not established.");
}
// _mutex = new SemaphoreSlim(0, 1)
await _mutex.WaitAsync().ConfigureAwait(false);
try
{
await _currentDelegate.SendMessage(message);
}
catch (Exception)
{
TerminateCommunication();
}
finally
{
_mutex.Release();
}
}
What I'm wondering though is whether I'm guaranteed that any given thread will see the most recent value of _currentDelegate when it executes await _currentDelegate.SendMessage(message), or whether I need to use another construct to ensure writes are immediately visible to other threads.
In particular, this class has another method TryRestartCommunication:
private bool TryRestartCommunication(bool initialStart)
{
// ...
lock (_lock)
{
// ...
try
{
_currentDelegate = _factory();
_currentDelegate.Start();
_currentDelegate.MessageReceived += MessageReceived;
CommunicationState = CommunicationState.Up;
return true;
}
// ...
}
}
If I re-implement this method to lock on the semaphore and have some Thread A call TryRestartCommunication() and immediately after Thread B call SendMessage(), am I guaranteed that Thread B will see the new value of _currentDelegate set by Thread A?
If not, would a decent solution be to just make _currentDelegate volatile, or to use Interlocked to update its value?
Edit
Got a close vote because apparently this question isn't clear enough. Let me try to make it as clear as possible: if I switch from protecting my critical regions with lock to SemaphoreSlim, do I then need to mark the shared fields as volatile (or something similar) to ensure the same level of thread safety?
