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.

Answer 1

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:

1. The condition is false.
2. 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.
3. 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.