How to write thread-safe C# code for Unity3D?

Question

I'd like to understand how to write thread safe code.

For example I have this code in my game:

bool _done = false;
Thread _thread;

// main game update loop
Update()
{
    // if computation done handle it then start again
    if(_done)
    {
        // .. handle it ...
        _done = false;
        _thread = new Thread(Work);
        _thread.Start();
    }
}

void Work()
{
     // ... massive computation

     _done = true;
}

If I understand it correctly, it may happened that main game thread and my _thread can have its own cached version of _done, and one thread may never see that _done changed in another thread?

And if it may, how to solve it?

1. Is it possible to solve by, only applying volatile keyword.

2. Or is it possible to read and write value through Interlocked's methods like Exchange and Read?

3. If I surround _done read and write operation with lock (_someObject), need I use Interlocked or something to prevent caching?

Edit 1

4. If I define _done as volatile and call Update method from multiple threads. Is it possible that 2 threads will enter if statement before I assign _done to false?

Answer 1

1. yes, but technically that's not what the volatile keyword does; it has that result as a side-effect, though - and most uses of volatile are for that side-effect; actually the MSDN documentation of volatile now only lists this side-effect scenario (link) - I guess the actual original wording (about reordering instructions) was just too confusing? so maybe this is now the official usage?

2. there are no bool methods for Interlocked; you'd need to use an int with values like 0/1, but that's pretty much what a bool is anyway - note that Thread.VolatileRead would also work

3. lock has a full fence; you don't need any additional constructs there, the lock by itself is enough for the JIT to understand what you need

Personally, I'd use the volatile. You've conveniently listed your 1/2/3 in increasing overhead order. volatile will be the cheapest option here.

Answer 2

Although you might use volatile keyword for your bool flag, it does not always guarantee a thread-safe access to the field.

In your case I'd probably create a separate class Worker and use events to notify when background task completes execution:

// Change this class to contain whatever data you need

public class MyEventArgs 
{
    public string Data { get; set; }
}

public class Worker
{
    public event EventHandler<MyEventArgs> WorkComplete = delegate { };
    private readonly object _locker = new object();

    public void Start()
    {
        new Thread(DoWork).Start();
    }

    void DoWork()
    {
        // add a 'lock' here if this shouldn't be run in parallel 
        Thread.Sleep(5000); // ... massive computation
        WorkComplete(this, null); // pass the result of computations with MyEventArgs
    }
}

class MyClass
{
    private readonly Worker _worker = new Worker();

    public MyClass()
    {
        _worker.WorkComplete += OnWorkComplete;
    }

    private void OnWorkComplete(object sender, MyEventArgs eventArgs)
    {
        // Do something with result here
    }

    private void Update()
    {
        _worker.Start();
    }
}

Feel free to change the code according to your needs

P.S. Volatile is good performance-wise, and in your scenario it should work as it looks like you get your reads and writes in the right order. Possibly the memory barrier is achieved precisely by reading/writing freshly - but there is no guarantee by MSDN specifications. It's up to you to decide whether to take the risk of using volatile or not.

Answer 3

Maybe you don't even need your _done variable, as you could achieve the same behaviour if you use the thread's IsAlive()-method. (given that you only have 1 background thread)

Like this:

if(_thread == null || !_thread.IsAlive())
{
    _thread = new Thread(Work);
    _thread.Start();
}

I didn't test this btw .. this is just a suggestion :)

Answer 4

Use MemoryBarrier()

System.Threading.Thread.MemoryBarrier() is the right tool here. The code may look clunky, but it's faster than the other viable alternatives.

bool _isDone = false;
public bool IsDone
{
    get
    {
        System.Threading.Thread.MemoryBarrier();
        var toReturn = this._isDone;
        System.Threading.Thread.MemoryBarrier();

        return toReturn;
    }
    private set
    {
        System.Threading.Thread.MemoryBarrier();
        this._isDone = value;
        System.Threading.Thread.MemoryBarrier();
    }
}

Don't use volatile

volatile doesn't prevent an older value from being read, so it doesn't meet the design objective here. See Jon Skeet's explanation or Threading in C# for more.

Note that volatile may appear to work in many cases due to undefined behavior, specifically a strong memory model on many common systems. However, reliance on undefined behavior can cause bugs to appear when you're running your code on other systems. A practical example of this would be if you're running this code on a Raspberry Pi (now possible due to .NET Core!).

Edit: After discussing the claim that "volatile won't work here", it's unclear exactly what the C# spec guarantees; arguably, volatile might be guaranteed to work, albeit it with a larger delay. MemoryBarrier() is still the better solution since it ensures a faster commit. This behavior is explained in an example from "C# 4 in a Nutshell", discussed in "Why do I need a memory barrier?".

Don't use locks

Locks are a heavier mechanism meant for stronger process control. They're unnecessarily clunky in an application like this.

The performance hit is small enough that you probably won't notice it with light use, but it's still sub-optimal. Also, it can contribute (even if slightly) toward larger problems like thread starvation and deadlocking.

Details about why volatile doesn't work

To demonstrate the issue, here's the .NET source code from Microsoft (via ReferenceSource):

public static class Volatile
{
    public static bool Read(ref bool location)
    {
        var value = location;
        Thread.MemoryBarrier();
        return value;
    }

    public static void Write(ref byte location, byte value)
    {
        Thread.MemoryBarrier();
        location = value;
    }
}

So, say that one thread sets _done = true;, then another reads _done to check if it's true. What does that look like if we inline it?

void WhatHappensIfWeUseVolatile()
{
    // Thread #1:  Volatile write
    Thread.MemoryBarrier();
    this._done = true;           // "location = value;"

    // Thread #2:  Volatile read
    var _done = this._done;      // "var value = location;"
    Thread.MemoryBarrier();

    // Check if Thread #2 got the new value from Thread #1
    if (_done == true)
    {
        //  This MIGHT happen, or might not.
        //  
        //  There was no MemoryBarrier between Thread #1's set and
        //  Thread #2's read, so we're not guaranteed that Thread #2
        //  got Thread #1's set.
    }
}

In short, the problem with volatile is that, while it does insert MemoryBarrier()'s, it doesn't insert them where we need them in this case.

Answer 5

Novices can create thread-safe code in Unity by doing the following:

• Copy the data they want a worker thread to work on.
• Tell a worker thread to work on the copy.
• From the worker thread, when the work is done, dispatch a call to the main thread to apply the changes.

This way you don't need locks and volatiles in your code, just two dispatchers (which hide all the locks and volatiles).

Now this is the simple and safe variant, the one novices should use. You're probably wondering what experts are doing: They do the exact same thing.

Here's some code from the Update method in one of my projects, which solves the very same problem you're trying to solve:

Helpers.UnityThreadPool.Instance.Enqueue(() => {
    // This work is done by a worker thread:
    SimpleTexture t = Assets.Geometry.CubeSphere.CreateTexture(block, (int)Scramble(ID));
    Helpers.UnityMainThreadDispatcher.Instance.Enqueue(() => {
        // This work is done by the Unity main thread:
        obj.GetComponent<MeshRenderer>().material.mainTexture = t.ToUnityTexture();
    });
});

Notice the only thing we have to do to make the above thread safe is to not edit blockor ID after calling enqueue. No volatile or explicit lock involved.

Here are the relevant methods from the UnityMainThreadDispatcher:

List<Action> mExecutionQueue;
List<Action> mUpdateQueue;

public void Update()
{
    lock (mExecutionQueue)
    {
        mUpdateQueue.AddRange(mExecutionQueue);
        mExecutionQueue.Clear();
    }
    foreach (var action in mUpdateQueue) // todo: time limit, only perform ~10ms of actions per frame
    {
        try {
            action();
        }
        catch (System.Exception e) {
            UnityEngine.Debug.LogError("Exception in UnityMainThreadDispatcher: " + e.ToString());
        }
    }
    mUpdateQueue.Clear();
}

public void Enqueue(Action action)
{
    lock (mExecutionQueue)
        mExecutionQueue.Add(action);
}

And here's a link to a thread pool implementation you can use until Unity finally supports the .NET ThreadPool: https://stackoverflow.com/a/436552/1612743

Answer 6

I do not think you need to add that much to your code. Unless Unity's version of Mono really does something hugely and codebreakingly different from the regular .Net Framework (which I am certain it does not), you'll not have different copies of _done in different threads in your scenario. So, no need for Interlocked, no need for volatile, no black magic.

What you really may encounter with a pretty high probability is a situation when your main thread sets _done to false in the very same moment when your background thread sets it to true, which is definitely a bad thing. That's why you need to surround those operations with 'lock's (don't forget to lock them on the SAME sync object).

As long as you use _done as you described in your code example, you can just 'lock' it every time you write your variable and everything will be fine.