Lockless Thread-safe state synchronization?

Question

First, I am aware of questions like this:

reference assignment is atomic so why is Interlocked.Exchange(ref Object, Object) needed?

... but I am still uncertain if I can avoid using lock(){} in my case.

In my case I have a class which represents some state and only a SINGLE thread that ever modifies that state from time to time. There are many threads that read the state though.

Do I need Interlocked.Exchange() in my case on the state object? Do I absolutely have to use lock(){}

Here is my example code reduced to a bare minimum:

using System;
using System.Collections.Generic;
using System.Threading;
using System.Threading.Tasks;

namespace MultiThreadingExample
{
class State
{
    public int X { get; set; }
    public string Str { get; set; }
    public DateTime Current { get; set; }
}

class Example
{
    State state;
    CancellationTokenSource cts = new CancellationTokenSource();

    Task updater;
    List<Task> readers = new List<Task>();

    public void Run()
    {
        updater = Task.Factory.StartNew(() =>
        {
            while (!cts.Token.IsCancellationRequested)
            {
                // wait until we have a new state from some source
                Thread.Sleep(1000);

                var newState = new State() { Current = DateTime.Now, X = DateTime.Now.Millisecond, Str = DateTime.Now.ToString() };

                // critical part
                state = newState;
            }
        }, cts.Token, TaskCreationOptions.LongRunning, TaskScheduler.Default);

        for (int i = 0; i < 10; i++)
        {
            readers.Add(Task.Factory.StartNew(() =>
            {
                while (!cts.Token.IsCancellationRequested)
                {
                    // critical part
                    var readState = state;

                    // use it
                    if (readState != null)
                    {
                        Console.WriteLine(readState.Current);
                        Console.WriteLine(readState.Str);
                        Console.WriteLine(readState.X);
                    }
                }
            }, cts.Token, TaskCreationOptions.LongRunning, TaskScheduler.Default));
        }
    }
}

class Program
{
    static void Main(string[] args)
    {
        new Example().Run();
        Console.ReadKey();
    }
}
}

Answer 1

If you only have a single thread that is updating and only a single thread reading I don't believe that you will run into any runtime errors. Your example however shows 10 reader threads. That being said I don't think that you should just assume that you don't need anything to make your application thread safe. You should introduce locking at a minimum to ensure that your threads will play nicely with one another. Because your State object is a complex object when you read the values in your reader thread, you may not be getting everything as you expect. Without locking one or two properties might be changed but not the third during the read operation. Here is a sample of the modifications that I am talking about.

class State
{
    public int X { get; set; }
    public string Str { get; set; }
    public DateTime Current { get; set; }
}

class Example
{
    State state;
    CancellationTokenSource cts = new CancellationTokenSource();
    Object syncObj = new Object();
    Task updater;
    List<Task> readers = new List<Task>();

    public void Run()
    {
        updater = Task.Factory.StartNew(() =>
        {
            while (!cts.Token.IsCancellationRequested)
            {
                // wait until we have a new state from some source
                Thread.Sleep(1000);

                var newState = new State() { Current = DateTime.Now, X = DateTime.Now.Millisecond, Str = DateTime.Now.ToString() };

                // critical part
                lock(syncObj) {
                    state = newState;
                }
            }
        }, cts.Token, TaskCreationOptions.LongRunning, TaskScheduler.Default);

        for (int i = 0; i < 10; i++)
        {
            readers.Add(Task.Factory.StartNew(() =>
            {
                while (!cts.Token.IsCancellationRequested)
                {
                    State readState = null;

                    // critical part
                    lock(syncObj) {
                        readState = state.Clone();
                    }

                    // use it
                    if (readState != null)
                    {
                        Console.WriteLine(readState.Current);
                        Console.WriteLine(readState.Str);
                        Console.WriteLine(readState.X);
                    }
                }
            }, cts.Token, TaskCreationOptions.LongRunning, TaskScheduler.Default));
        }
    }
}

class Program
{
    static void Main(string[] args)
    {
        new Example().Run();
        Console.ReadKey();
    }
}

It's a small change but it would ensure that you are thread safe with regard to the State object.

Answer 2

This is unsafe if cts.Token.IsCancellationRequested does not cause a memory barrier. If it does not then the compiler is free to only read once and cache state locally.

I don't think it's documented whether cts.Token.IsCancellationRequested causes a memory barrier. Normally, these concerns are not documented.

Answer 3

The way your code is written right now, a single "updater" task sets the state to a certain value, and then all readers start reading it and processing it as soon as they have the chance to do it. And they keep reading this state forever, or until it changes.

You probably don't want 10 threads doing the same thing, and then doing this same thing again in the next cycle, until state gets changed.

Correct way to implement a single producer/multiple consumer

I would at least set the state to null atomically in one of the readers:

// read and swap with null atomically
var readState = Interlocked.Exchange(ref state, null);

This will still leave your readers spinning the CPU like crazy, and this is likely not something you wanted.

A better solution would be to use a BlockingCollection, which solves most of your problems:

BlockingCollection<State> queue = new BlockingCollection<State>();

updater = Task.Factory.StartNew(() =>
{
    while (!cts.Token.IsCancellationRequested)
    {
        Thread.Sleep(1000);

        var newState = GetNewState();

        queue.Add(newState);
    }
}, cts.Token, TaskCreationOptions.LongRunning, TaskScheduler.Default);

for (int i = 0; i < 10; i++)
{
    var readerId = i.ToString();

    readers.Add(Task.Factory.StartNew(() =>
    {
        while (!cts.Token.IsCancellationRequested)
        {
            // get it
            var readState = queue.Take(cts.Token);

            // use it
            if (readState != null)
            {
                Console.WriteLine("Hello from reader #" + readerId);
                Console.WriteLine(readState.X);
            }
        }
    }, cts.Token, TaskCreationOptions.LongRunning, TaskScheduler.Default));
}

First of all, BlockingCollection<T>.Take will block all your readers until it's signalled from the writer (updater) thread. This means those threads are sitting doing nothing, and your CPU should be idle.

Also, it's neat that the method accepts a CancellationToken, meaning you don't have to worry about unblocking your readers when you're done.

Do you only need to share a read-only state?

If your intent is merely sharing some readonly state between threads (IMHO your example code doesn't express this intention clearly), then the correct way to prevent from shooting yourself in the foot would be:

1. Make the state field volatile to prevent compiler and CPU magically reordering instructions and caching it, and

2. Make all fields inside the State class readonly, to prevent yourself from modifying any of its fields once you assign it.

3. Ensure that all fields inside the State class are either primitive types, or immutable structs/classes.

Answer 4

When in Doubt use Locks

Unless you have actually noticed a performance issue or have a section that can be accessed by Async logic that may or may not be multithreaded and need to do waiting and signaling would recommend using locks for synchronization between threads.

That being said

Could use Thread.MemoryBarrier to do what you are asking

As long as you don't care about the accuracy of the data you are reading the issue you would end up hitting would have to do with compiler optimization that would reorder your instructions incorrectly because it is multithreaded.

A way you could avoid that is with the Thread.MemoryBarrier method.

            while (!cts.Token.IsCancellationRequested)
            {
                //No command before the barrier
                Thread.MemoryBarrier();
                //Can end up on this side of the barrier  

                // critical part
                var readState = state;

                // use it
                if (readState != null)
                {
                    Console.WriteLine(readState.Current);
                    Console.WriteLine(readState.Str);
                    Console.WriteLine(readState.X);
                }
            }

This is called a half fence, there is a lot more information here about fences explained much better than I ever could a free ebook, Threading in C# by Joseph Albahari