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?

Solution 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.

Solution 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.

Solution 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 :)

Solution 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.

Solution 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

View more solutions

Comments

[M. Adeel Khalid]

Instead of using threads, why don't you try it using tasks?

[Stas BZ]

@MAdeelKhalid, I Use Unity3d engine, it supports only framework 2

[PJvG]

@StasBZ you can add tags Unity3D and the tag for the .NET version (e.g. .net-3.5) your using to avoid getting answers not applicable for your .NET version.

[Marc Gravell]

Note that threads are expensive (relatively speaking) starting a new thread each time is probably not ideal

[PJvG]

@StasBZ Perhaps you should also look into using thread pools.

[Programmer]

I see what you are doing. You simply want to execute something in the main Thread from another Thread. What you have now is a basic way of doing that but it shouldn't be used in a production code. You need a queue system. Take a took at this post for proper way to do this. I made a UnityThread class for that.

[Kroltan]

If you want to use multithreading, I'd recommend using a library. For example, UniRX provides a neat thread pool and tasks replacement for Unity, and also comes with all the Rx toolset (as the name implies, huh), which helps a lot in getting asynchronicity right.

[Nat]

Use the public SharedState GetFreshSharedState { get; } implementation shown in stackoverflow.com/questions/24677773/… with .MemoryBarrier(). Don't use volatile because it has a fencing issue. Don't use lock because it's unnecessarily clunky for this application. You could use .Interlocked() with an int if you're willing to check it against a const int TRUE_VALUE = 1, or something like that, to turn it into a bool.

[Nat]

See the "Can they be swapped?" table in Threading in C# for an explanation about why volatile won't fully solve this problem.

[Nat]

Related: stackoverflow.com/questions/3493931/…

[Stas BZ]

Can you explain why volatile does not guarantee thread-safe access to the field? Or share link.

[Stas BZ]

Could you answer position 4 that I added to my question, please?

[Nat]

@StasBZ See the "Can they be swapped?" table in Threading in C#. tl;dr- volatile doesn't prevent a read attempt from "reading" a cached value after a newer one was written, so if _done is set to TRUE, volatile won't prevent it from appearing to be FALSE. (I'm not endorsing the rest of this answer, but the poster was right about that particular point.)

[Nat]

Use of volatile wouldn't work here (Jon Skeet's explanation), while lock's excessive.

[IInspectable]

"Unless Unity's version of Mono really does something hugely and codebreakingly different from the regular .Net Framework [...], you'll not have different copies of _done in different threads in your scenario." - There is a world outside the framework, namely a CPU that runs the code. The CPU could store _done in a register, and with each thread maintaining a separate copy of the register file, you could wind up with two distinct copies of _done in two threads.

[Benjamin Gruenbaum]

This is the correct way to solve the problem. No need to do synchronization manually when it is provided by the platform. Writing multithreaded code is hard enough as it is.

[Peter]

Thanks. To prevent a potential misunderstanding: Unfortunately the solution isn't part of the official platform yet, UnityMainThreadDispatcher and UnityThreadPool are classes that every shop has to re-create on their own. Although I did provide a partial implementation for one and a link to the other, to ease the pain. I'm currently considering posting the complete but less than perfect versions of both as a community wiki answer.

[Shaggi]

The link explains reordering inside one thread, not across threads. Acquire/release semantics establish synchronization primitives between multiple threads. The author in the link is wrong (as is the assertion in this answer): Whether you're reading a fresh or stale value atomically is a question of cache-coherency, not memory ordering - also, most processors (including all platforms C# run on) guarantee cache coherency by default.

[Shaggi]

See my other comment, as well. "Older values" (referred to typically as fresh or stale) are effects of cache-coherency, not memory ordering. If your volatile read/write are of the atomic type (hint: volatile type requirements are identical), volatile indeed always guaranteed the most fresh value. See §10.5 of the spec for more information.

[creker]

@Shaggi, on ARM you don't have that guarantee and C# does run on it. Volatile doesn't guarantee cache-coherency, so you might end up reading stale value.

[creker]

@Shaggi, the spec doesn't say anything about preventing stale values. It only mentions memory reordering. So you still have problem with cache-coherency and it will get you in trouble on something like ARM

[creker]

I don't like this answer. Locks should be the first and pretty much the only thing you use in cases like this. Just because they work and don't require too much expertise. If you don't want lock for some reason, you can use interlocked. If you still thinking that you need something else, then think again. You should really know what you're doing and why, if you thinking about placing memory barriers by hand.

[Nat]

@creker Locks are a conceptually incorrect tool here. They'll do weird stuff like prevent two threads from reading the .IsDone at the same time. Plus you have to pay for the undesired feature by taking an unnecessary performance hit, both for merely acquiring the lock in the first place and a larger one if you frequently encounter reads block each other. Finally, locks can lead to thread starvation under heavy load, freezing the program.

[Shaggi]

@creker Reading up on this, it is clear that referring to "arm" as an architecture is meaningless, however they did change their cache coherence guarantees on all archs at some point to always have it. This 17-year old arch describes how you must (in software) update shared memory if you 'dirty' it... Not hard to see why everyone is moving towards cache coherence: infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0151‌​c/… A9 (at least, and forward) has cache coherence: infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0407‌​e/…

[Shaggi]

Let us continue this discussion in chat.

[rs232]

It is not a CPU who decides which variable should be stored in a register, but a compiler during optimization. I highly doubt that a compiler would ever move a class data member variable, which is used from withing multiple class methods, into a register.

[IInspectable]

I didn't imply, that the CPU were to make that decision (although it wasn't worded succinctly enough either). The point, however, is, that any variable must be loaded into a CPU register, if the CPU needs to work with it, creating at least a transient state, where two distinct copies exist.

[rs232]

You are now re-phrasing the second paragraph of my own answer.

[IInspectable]

Your first paragraph claims, that there never will be two copies in distinct memory locations, and your second paragraph never challenges this. My comments claim, that the variable can have two distinct copies in different locations (e.g. memory and CPU register). This contradicts your entire proposed answer, and frankly, I wouldn't know what to make of your second paragraph based on the assumption that you meant to have it agree with your first paragraph. Both cannot be right at the same time. Take 0 up-votes on a high-frequency Q&A as a hint.

[rs232]

The variable's value might be copied from memory to a register and back again as a part of a non-atomic operation, which is not something you might describe as "game thread and my _thread can have its own cached version of _done". When you consider these data operations as variable copies, you mix internal details of a realization of non-atomic operations on certain processors with language entities. From the user's code perspective, it absolutely does not matter what happens on hardware level.

[rs232]

The only important things are: a) both threads will access the same variable, no "cached versions" b) they should not access it at the same time, that's why critical sections required. I see you're frustrated being unable to separate different layers of abstraction; feel free to ask more questions if you still fail to understand what is the difference and why it is important.