Search code examples
c#multithreadinglockingthread-synchronizationinterlocked

Proper way to synchronize a property's value in a multi-threaded application


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:

  1. 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?
  2. 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); } }
  1. 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:

  1. 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.
  2. 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;
    }

Solution

    1. Yes, the 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.
    2. Replacing the 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);
    }
    
    1. Omitting the barrier in either the 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!