Are checks for null thread-safe?

Question

I have some code where exceptions are thrown on a new Thread which I need to acknowledge and deal with on the Main Thread. To achieve this I am sharing state between threads by using a field which holds the thrown exception.

My question is do I need to use locks when checking for null as I am doing in the following code sample?

public class MyClass
{
    readonly object _exceptionLock = new object();
    Exception _exception;

    public MyClass()
    {
        Task.Run(() =>
        {
            while (CheckIsExceptionNull())
            {
                // This conditional will return true if 'something has gone wrong'.
                if(CheckIfMyCodeHasGoneWrong())
                {
                    lock(_exceptionLock)
                    {
                        _exception = new GoneWrongException();
                    }
                }
            }
        });
    }

    bool CheckIsExceptionNull() // Is this method actually necessary?
    {
        lock (_exceptionLock)
        {
            return _exception == null;
        }
    }

    // This method gets fired periodically on the Main Thread.
    void RethrowExceptionsOnMainThread()
    {
        if (!CheckIsExceptionNull())
        {
            lock (_exceptionLock)
            {
                throw _exception; // Does this throw need to be in a lock?
            }
        }
    }
}

Additionally, do I need to use a lock when throwing the exception on the Main Thread?

Answer 1

The first thing to note is that your code is not thread safe because you have a thread race: you check CheckIsExceptionNull in a different locked region to where you throw, but: the value can change between the test and the throw.

Field access is not guaranteed to be thread safe. In particular, while references cannot be torn (that much is guaranteed - reads and writes of references are atomic), it is not guaranteed that different threads will see the latest values, due to CPU caching etc. It is very unlikely to actually bite you, but that's the problem with threading issues in the general case ;p

Personally, I'd probably just make the field volatile, and use a local. For example:

var tmp = _exception;
if(tmp != null) throw tmp;

The above has no thread race. Adding:

volatile Exception _exception;

ensures the value isn't cached in a register (although that is technically a side-effect, not the intended / documented effect of volatile)

Answer 2

There might be lot of interesting and useful information about questions like this. You can spend hours on reading blog posts and books about it. Even the people who actually know it best disagree. (As you can see in the post from Marc Gravell and Eric Lippert).

So there is not an answer to this question from me. Just a suggestion: Always use locks. As soon as more then one thread is accessing a field. No risk, no guessing, no discussing about compiler and CPU technology. Just use locks and be save and make it work not only in most cases on your machine, but in every case on every machine.

Further reading:

• http://www.albahari.com/threading/part4.aspx
• http://blogs.msdn.com/b/ericlippert/archive/2011/06/16/atomicity-volatility-and-immutability-are-different-part-three.aspx