I've recently started revisiting some of my old multi-threaded code and wondering if it's all safe and correct (No issues in production yet...). In particular am I handling object references correctly? I've read a ton of examples using simple primitives like integers, but not a lot pertaining to references and any possible nuances.
First, I recently learned that object reference assignments are atomic, at least on a 64 bit machine which is all I'm focused on for this particular application. Previously, I was locking class properties' get/sets to avoid corrupting the reference as I didn't realize reference assignments were atomic. For example:
// Immutable collection of options for a Contact
public class ContactOptions
{
public string Email { get; }
public string PhoneNumber { get; }
}
// Sample class that implements the Options
public class Contact
{
private readonly object OptionsLock = new object();
private ContactOptions _Options;
public ContactOptions Options { get { lock(OptionsLock) { return _Options; } }
set { lock(OptionsLock) { _Options = value; } } };
}
Now that I know that a reference assignment is atomic, I thought "great, time to remove these ugly and unnecessary locks!" Then I read further and learned of synchronization of memory between threads. Now I'm back to keeping the locks to ensure the data doesn't go stale when accessing it. For example, if I access a Contact's Options, I want to ensure I'm always receiving the latest set of Options assigned.
Questions:
private ContactOptions _Options;
public ContactOptions Options {
get { return Interlocked.CompareExchange(ref _Options, null, null); }
set { Interlocked.Exchange(ref _Options, value); } }
private ContactOptions _Options;
public ContactOptions Options {
get { return Interlocked.CompareExchange(ref _Options, null, null); }
set { _Options = value; } }
Side Notes:
public class SomeInteger
{
private readonly object ValueLock = new object();
private int _Value;
public int Value { get { lock(ValueLock) { return _Value; } }
private set { lock(ValueLock) { _Value = value; } } };
// WRONG
public void manipulateBad()
{
Value++;
}
// OK
public void manipulateOk()
{
lock (ValueLock)
{
Value++;
// Or, even better: _Value++; // And remove the lock around the setter
}
}
}
Point being, I'm really only focused on the memory synchronization issue.
SOLUTION: I went with the Volatile.Read and Volatile.Write methods as they do make the code more explicit, they're cleaner than Interlocked and lock, and they're faster than that aforementioned.
// Sample class that implements the Options
public class Contact
{
public ContactOptions Options { get { return Volatile.Read(ref _Options); } set { Volatile.Write(ref _Options, value); } }
private ContactOptions _Options;
}
lock (OptionsLock)
ensures that all threads will see the latest value of the Options
, because memory barriers are inserted when entering and exiting the lock
.lock
with methods of the Interlocked
or the Volatile
class would serve equally well the latest-value-visibility goal. Memory barriers are inserted by these methods as well. I think that using the Volatile
communicates better the intentions of the code:public ContactOptions Options
{
get => Volatile.Read(ref _Options);
set => Volatile.Write(ref _Options, value);
}
get
or the set
accessor puts you automatically in the big black forest of memory models, cache coherency protocols and CPU architectures. In order to know if it's safe to omit it, intricate knowledge of the targeted hardware/OS configuration is required. You will need either an expert's advice, or to become an expert yourself. If you prefer to stay in the realm of software development, don't omit the barrier!