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?

Any time the question is "do I need to use a lock", that's suspicious. An uncontested lock takes a dozen nanoseconds. Is your question "my program is already guaranteed to be correct, but it is a dozen nanoseconds too slow, this performance problem is attributable to a lock, and I know it will not be successful in the marketplace as a result, so can I remove this lock and preserve the correctness of my program?". Your code isn't even correct to begin with, and I highly doubt that the nanoseconds spent in the uncontested lock will cause your program to be considered too slow. — Eric Lippert, Jun 15 '15 at 15:24
A better question entirely would be "how can I eliminate my use of explicit thread management and shared memory by using the task parallel library to express the notions of success and failure of an asynchronously executed task?". The Task object already has the property that an exception thrown during the execution of the task can be safely cached and re-thrown on the execution context that is managing the task. — Eric Lippert, Jun 15 '15 at 15:26

score 10 · Accepted Answer · edited Jun 15 '15 at 12:00

10

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)

edited Jun 15 '15 at 12:00

Holf

5,605
3
42
63

answered Jun 15 '15 at 11:04

Marc Gravell

1,026,079
266
2,566
2,900

1

"Frankly, I discourage you from ever making a volatile field", Eric Lippert: (http://stackoverflow.com/a/17530556/7122). Please don't encourage its use. – David Arno Jun 15 '15 at 11:05
@DavidArno I'm not sure that adding `lock`, `Thread.VolatileRead` or some `Interlocked` is going to improve readability, however... – Marc Gravell Jun 15 '15 at 11:07
@DavidArno Nothing wrong with using volatile if you know what you're doing, thought that isn't common for people asking threading questions on SO. – Yuval Itzchakov Jun 15 '15 at 11:07
@YuvalItzchakov well, I *kinda* agree with his point in general; heck, I couldn't even give you the formal definition of `volatile` without double-checking - but in the real world, it is pretty common to make use of this side-effect *in some scenarios*... and I'd argue this is one of them – Marc Gravell Jun 15 '15 at 11:09
1

@MarcGravell There is no doubt going down that road maybe questionable, especially when you're using a *side effect*, and for sure using a `lock` would greatly save him from making mistakes down the road, but still, that doesn't mean it isn't a valid answer. – Yuval Itzchakov Jun 15 '15 at 11:11
@MarcGravell Did you mean to write 'var tmp = _exception' in your example? – Holf Jun 15 '15 at 11:13
3

I actually think that `Volatile.Read()` is better than making a variable `volatile`, since it is better targeted (you only do it where you need it) and makes it more obvious that a volatile read is being done. (Note that `Volatile.Read()` is NOT the same as `Thread.VolatileRead()` which [should not be used](http://stackoverflow.com/a/15052688/106159).) – Matthew Watson Jun 15 '15 at 11:27
I love this article: http://www.albahari.com/threading/part4.aspx. Cit: "the volatile keyword is even easier to get wrong". After reading it to the end, you'll never use volatile and stuff like this anymore and just use `lock`, which is easy and just works. – Stefan Steinegger Jun 15 '15 at 13:32
@StefanSteinegger: I agree, but I note that just because locks are *easier* to get right than volatile doesn't make code with locks "just work" -- you still have to worry about races, as Marc showed in this answer. The better solution is to *not write code that has multiple threads of control sharing memory* in the first place. – Eric Lippert Jun 15 '15 at 15:21
@EricLippert: I agree, multi threading is *always* easy to get wrong. I didn't say it gets easy when using locks. I mean that you should make it compiler and hardware independent that you have a little *chance* to make your code actually thread safe. Because you have a *chance* to see the possible problems within your code and do not have to think about compilers and CPUs and things you can't even know and things you shouldn't have to care about when trying to make your, lets say, web shop work. – Stefan Steinegger Jun 16 '15 at 05:53

score 1 · Answer 2 · answered Jun 15 '15 at 13:55

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.

Are checks for null thread-safe?

2 Answers2