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:
- Correct me if I'm wrong here, but the above code does ensure that I'm achieving the goal of getting the latest value of Options when I get it in a thread safe manner? Any other issues using this method?
- I believe there is some overhead with the lock (Converts to Monitor.Enter/Exit). I thought I could use Interlocked for a nominal performance gain, but more importantly to me, a cleaner set of code. Would the following work to achieve synchronization?
private ContactOptions _Options;
public ContactOptions Options {
get { return Interlocked.CompareExchange(ref _Options, null, null); }
set { Interlocked.Exchange(ref _Options, value); } }
- Since a reference assignment is atomic, is the synchronization (using either lock or Interlocked) necessary when assigning the reference? If I omit the set logic and only maintain the get, will I still maintain atomicity and synchronization? My hopeful thinking is that the lock/Interlock usage in the get would provide the synchronization I'm looking for. I've tried writing sample programs to force stale value scenarios, but I couldn't get it done reliably.
private ContactOptions _Options;
public ContactOptions Options {
get { return Interlocked.CompareExchange(ref _Options, null, null); }
set { _Options = value; } }
Side Notes:
- The ContactOptions class is deliberately immutable as I don't want to have to synchronize or worry about atomicity within the options themselves. They may contain any kind of data type, so I think it's a lot cleaner/safer to assign a new set of Options when a change is necessary.
- I'm familiar of the non-atomic implications of getting a value, working with that value, then setting the value. Consider the following snippet:
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;
}
