Non volatile status flag in multi-threaded code

Question

I am a novice when it comes to concurrency and unsure of myself when spotting issues, I was looking through a fairly established code base and found the following code (edited for brevity) which I believe to be susceptible to data races:

public class Example extends Thread {
    boolean condition = false;

    public void run () {
        while (true) {
            synchronized (this) {
                try {
                    while( condition ) wait();
                }
                catch (InterruptedException e) { /*for brevity*/  }
            }

            // non-blocking computation
        }
    }

    public void setTrue () { condition = true; }

    public void setFalse () {
        synchronized (this) {
            condition = false;
            this.notifyAll();
        }
    }
}

As far as I understand condition must be volatile since even with the synchronized block, the compiler will not issue any memory barriers; if it were a volatile store to condition in setTrue the compiler would issue StoreEnter.

Am I right to believe the above is susceptible to data races? And if so how can I witness the data race through an example (as opposed to simply knowing the guarantees provided by the JMM). A simple test with threads randomly invoking setTrue in a loop doesn't uncover the data race.

Also, I believe the use of notifyAll is overkill here since there is one condition to check and only one thread will ever be waiting on it, right?

Thank you.

Don't extend the `Thread` class unless you really have reason to. It is better to implement the Runnable interface and supply an instance of your class to the Thread constructor. — gparyani, Aug 18 '13 at 02:32

Stephen C · Accepted Answer · 2013-08-18T03:35:15.663

As far as I understand condition must be volatile since even with the synchronized block, the compiler will not issue any memory barriers; if it were a volatile store to condition in setTrue the compiler would issue StoreEnter.

That is not correct. When you use a shared variable within a synchronized block, your code will be thread-safe with respect to other threads using the same variable with the same lock. If memory barriers are required, then they will be used.

However, the code you have shown us is is incorrect because the setTrue() method is updating the flag outside of a synchronized block.

Am I right to believe the above is susceptible to data races?

Yea ... sort of. The scenario is as follows:

The condition is false.
Some other thread calls setTrue which sets the condition variable to true in its cache. But since the setTrue method doesn't use synchronized, there is no write barrier, and no flushing to main memory.
The "example" thread fetches the latest committed value from main memory (which is still false), and doesn't wait as it is supposed to do.

Also, I believe the use of notifyAll is overkill here since there is one condition to check and only one thread will ever be waiting on it, right?

It could be replaced with a notify() ... if that is what you mean. But to be honest, it makes no real difference which flavour of notify you use.

You commented:

I meant that the compiler would not consider it necessary to submit a memory barrier in this situation.

Maybe. But the "monitorenter" and "monitorexit" instructions implicitly involve memory barriers.

And:

Wouldn't it also be correct if condition were volatile?

If you are talking about using volatile AND synchronized, then yes it would be correct ... though the volatile would be redundant (assuming that the setTrue bug is fixed.)

If you are talking about volatile only, then no. You can't implement an efficient "wait on a condition variable" with just volatile. The problem is that neither the "read/test/wait" or "write/notify" sequences can be performed atomically; i.e. without the possibility of race-conditions.

And besides, you can't do the equivalent of wait/notify without using a primitive object mutex, or a Lock object.

I meant that the compiler would not consider it necessary to submit a memory barrier in this situation. Wouldn't it also be correct if condition were volatile? — bluth, Aug 18 '13 at 02:44

score 0 · Answer 2 · answered Aug 18 '13 at 03:03

Am I right to believe the above is susceptible to data races?

Don't think so. condition is unimportant, it only permits the method to avoid waiting. The way it is set is also not important. It doesn't need to be volatile, as it's use is local to one object.

Also, I believe the use of notifyAll is overkill here since there is one condition to check and only one thread will ever be waiting on it, right?

NotifyAll is fine, while there is only one thread waiting in the method there may be many other threads waiting on, or waiting for, the thread.

Non volatile status flag in multi-threaded code

2 Answers2