7

I have a simple TestThreadClientMode class to test a race condition. I tried two attempts:

  1. When I run the following code with System.out.println(count); commented in the second thread, the output was:

OS: Windows 8.1 flag done set true ...

and the second thread was alive forever. Because the second thread never sees change of the done flag which was set true by Main thread.

  1. When I uncommented System.out.println(count); the output was:

    OS: Windows 8.1 0 ... 190785 190786 flag done set true Done! Thread-0 true

And the program stopped after 1 second.

How did System.out.println(count); make the second thread see the change in done?

Code

public class TestThreadClientMode {
    private static boolean done;
    public static void main(String[] args) throws InterruptedException {
        new Thread(new Runnable() {
            public void run() {
                int count = 0;
                while (!done) {
                    count ++;
                    //System.out.println(count);
                }
                System.out.println("Done! " + Thread.currentThread().getName() + "  " + done);
            }
        }).start();
        System.out.println("OS: " + System.getProperty("os.name"));

        Thread.sleep(1000);
        done = true;

        System.out.println("flag done set true ");
    }
}
Peter Mortensen
  • 30,738
  • 21
  • 105
  • 131
Humoyun Ahmad
  • 2,875
  • 4
  • 28
  • 46

3 Answers3

6

This is a brilliant example of memory consistency errors. Simply put, the variable is updated but the first thread does not always see the variable change. This issue can be solved by making done variable volatile by declaring it like so:

private static volatile boolean done;

In this case, changes to the variable are visible to all threads and the program always terminates after one second.

Update: It appears that using System.out.println does indeed solve the memory consistency issue - this is because the print function makes use of an underlying stream, which implements synchronization. Synchronization establishes a happens-before relationship as described in the tutorial I linked, which has the same effect as the volatile variable. (Details from this answer. Also credit to @Chris K for pointing out the side effect of the stream operation.)

Community
  • 1
  • 1
Parker Hoyes
  • 2,118
  • 1
  • 23
  • 38
  • Parker thank you for the answer, yes you are right, if I declare done as volatile it is guaranteed to be seen by second thread, because it causes main thread to cross the Memory Barrier, and as a result it is visible to other threads, but I want to know the underlying reason of this behavior – Humoyun Ahmad Aug 30 '15 at 07:45
  • @Humoyun I updated the answer. The effects of the `System.out.println` were likely only coincidence - in my testing there was no correlation. – Parker Hoyes Aug 30 '15 at 07:48
  • 1
    @Humoyun My bad - the print *does* actually establish a "happens-before" relationship because of synchronization used in the underlying stream. – Parker Hoyes Aug 30 '15 at 07:53
2

How did System.out.println(count); make the second thread see the change in done?

You are witnessing a side effect of println; your program is suffering from a concurrent race condition. When coordinating data between CPUs it is important to tell the Java program that you want to share the data between the CPUs, otherwise the CPUs are free to delay communication with each other.

There are a few ways to do this in Java. The main two are the keywords 'volatile' and 'synchronized' which both insert what hardware guys call 'memory barriers' into your code. Without inserting 'memory barriers' into the code, the behaviour of your concurrent program is not defined. That is, we do not know when 'done' will become visible to the other CPU, and it is thus a race condition.

Here is the implementation of System.out.println; notice the use of synchronized. The synchronized keyword is responsible for placing memory barriers in the generated assembler which is having the side effect of making the variable 'done' visible to the other CPU.

public void println(boolean x) {
    synchronized (this) {
        print(x);
        newLine();
    }
}

The correct fix for your program, is to place a read memory barrier when reading done and a write memory barrier on writing to it. Typically this is done by reading or writing to 'done' from within a synchronized block. In this case, marking the variable done as volatile will have the same net effect. You can also use an AtomicBoolean instead of boolean for the variable.

Peter Mortensen
  • 30,738
  • 21
  • 105
  • 131
Chris K
  • 11,622
  • 1
  • 36
  • 49
  • And yes, using synchronization would establish a "happens-before" relationship with the read on the flag, essentially having the same effect as a volatile variable. – Parker Hoyes Aug 30 '15 at 07:46
  • @ParkerHoyes yes, that is correct. the use of println is a red herring/side effect. The synchronized block within it does add memory barriers to the code but they are in the wrong place to guarantee correct behaviour. – Chris K Aug 30 '15 at 07:58
  • @ChrisK Does that actually matter? Given that the monitor exit from `println` after `done = true` produced sync edge A, and calling `println` within the loop produces sync edge B, would actions prior to A not be *happens-before* B? Granted, it only affects the next recursion and given that the sync edge prevents loop hoisting, the code could be sufficiently piggybacking `println`. Thoughts? –  Aug 30 '15 at 08:12
  • @xTrollxDudex the println memory barrier only appears in the writing thread (the read thread does not call println until after it has already seen the data change)... so we still do not know when the data will become visible to the other CPU.. remember, we need a barrier both when writing to a variable AND when reading. – Chris K Aug 30 '15 at 08:14
  • @ChrisK I was referring to if OP uncommented the `println` line within the `while` loop. That *should* produce the needed fences if I understand correctly. –  Aug 30 '15 at 08:17
  • 1
    @xTrollxDudex yes, that is correct.. – Chris K Aug 30 '15 at 08:18
  • I like how you explain it using "memory barriers". Oddly, an operation that happens-before another isn't always technically considered a "happens-before" relationship, that's why you need to synchronize it or declare it volatile. And nice catch on how the `println` call has the same effect due to it's synchronization. – Parker Hoyes Aug 30 '15 at 08:23
1

The println() implementation contains explicit memory barrier:

public void println(String x) {
    synchronized (this) {
        print(x);
        newLine();
    }
}

Which causes the invoking thread to refresh all variables.

The following code will have the same behavior as yours:

    public void run() {
        int count = 0;
        while (!done) {
            count++;
            synchronized (this) {
            }
        }
        System.out.println("Done! " + Thread.currentThread().getName() + "  " + done);
    }

In fact any object can be used for monitor, following will also work:

synchronized ("".intern()) {
}

Another way to create explicit memory barrier is using volatile, so the following will work:

new Thread() {
    private volatile int explicitMemoryBarrier;
    public void run() {
        int count = 0;
        while (!done) {
            count++;
            explicitMemoryBarrier = 0;
        }
        System.out.println("Done! " + Thread.currentThread().getName() + "  " + done);
    }
}.start();
Peter Mortensen
  • 30,738
  • 21
  • 105
  • 131
outdev
  • 5,249
  • 3
  • 21
  • 38
  • You've synchronised on the wrong lock within your loop. –  Aug 30 '15 at 08:19
  • @xTrollxDudex the weird thing here is that which object that we use as the lock does not matter, if we were using the synchronized keyword for mutual exclusion then you would be correct. However we are after the side effect of having a memory barrier inserted, and in that case it does not matter which object is being used as the lock. The one thing that I am unsure about, that may affect this is when will Hotspot choose to strip the synchronzied block out of the code.. if it was to ever do that, we would loose the memory barrier. – Chris K Aug 30 '15 at 08:22
  • Either way, using a synchronized block for its side effect either explicitly as written here or hidden within println is asking for bugs. It is much better to be explicit and make the variable volatile, as discussed in Parker's answer. But ultracoms does explain why the OP saw this behaviour well. – Chris K Aug 30 '15 at 08:24
  • @ChrisK I am getting confused here - the fences should only take affect if they refer to the same state, right? –  Aug 30 '15 at 08:25
  • @xTrollxDudex alas, no.. – Chris K Aug 30 '15 at 08:26
  • 1
    @xTrollxDudex we are getting into some real low level detail here, detail that starts to become CPU centric. The Java Memory Model was updated in Java 5, it would be worth a read as it was purposefully changed to communicate that data outside of a locked monitor are to be included within the fence. That includes the behaviour volatile (and its affects against other fields), that does not even refer to a object monitor. – Chris K Aug 30 '15 at 08:28
  • @ChrisK Strange, I will need to review the JSR133 Cookbook again. On last note: memory barriers stay in their place even if the respective code was Dead Code Eliminated. –  Aug 30 '15 at 08:29
  • @xTrollxDudex cool, that is good to know.. thanks. – Chris K Aug 30 '15 at 08:30
  • @xTrollxDudex Check the forth table in the cookbook, it talks about memory barriers for 'normal reads' when both volatile and synchronized are used (http://g.oswego.edu/dl/jmm/cookbook.html) – Chris K Aug 30 '15 at 08:34
  • After that, the behaviour is undefined.. so anything could happen. To my knowledge once the memory barrier has been executed the CPU will not differentiate data.. but that all depends on the hardware. Intel works via the load and store memory reordering buffers which do not take that into account. – Chris K Aug 30 '15 at 08:37
  • @ChrisK I think I understand now. How does instruction reordering and JMM rules come into play regarding the edges generated by 2 different state variables? –  Aug 30 '15 at 08:58
  • @xTrollxDudex the way that I think of a memory barrier is as a token within a queue. The queue contains load or store operations (intel have one queue for loads and another for stores, other hardware may and do do it differently). A load or store operation cannot be reordered past the special memory barrier. However when there are two loads before the memory barrier, then they can still be reordered between themselves. – Chris K Aug 30 '15 at 09:05
  • Let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/88292/discussion-between-xtrollxdudex-and-chris-k). –  Aug 30 '15 at 09:08