Why do I get an infinite while loop even if I modify the lock variable? [duplicate]

Question

public class GuardedBlock {

    private boolean guard = false;

    private static void threadMessage(String message) {
        System.out.println(Thread.currentThread().getName() + ": " + message);
    }

    public static void main(String[] args) {
        GuardedBlock guardedBlock = new GuardedBlock();

        Thread thread1 = new Thread(new Runnable() {

            @Override
            public void run() {
                try {
                    Thread.sleep(1000);
                    guardedBlock.guard = true;
                    threadMessage("Set guard=true");
                } catch (InterruptedException e) {
                    e.printStackTrace();
                }

            }
        });

        Thread thread2 = new Thread(new Runnable() {

            @Override
            public void run() {
                threadMessage("Start waiting");
                while (!guardedBlock.guard) {
                    //threadMessage("Still waiting...");
                }
                threadMessage("Finally!");
            }
        });

        thread1.start();
        thread2.start();
    }
}

I was learning concurrency through java essentials tutorial. Got to guarded blocks and tried to test it. There is one thing I cannot understand.

While loop is infinite, but if you uncomment threadMessage line everything works fine. Why?

Answer 1

Short answer

You forgot to declare guard as a volatile boolean.

---

If you ommit the declaration of your field as volatile, you are not telling the JVM that this field can be seen by multiple thread which is the case in your example.

In such cases, the value of guard will be read only once and will cause an infinite loop. It will be optimized to something like this (without the print) :

if(!guard)
{
    while(true)
    {
    }
}

Now why System.out.println change this behaviour ? Because the writes are synchronized which force the threads to not cache reads.

Here a paste of the code of one of the println method of PrintStream used by System.out.println :

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

and the write method :

private void write(String s) {
    try {
        synchronized (this) {
            ensureOpen();
            textOut.write(s);
            textOut.flushBuffer();
            charOut.flushBuffer();
            if (autoFlush && (s.indexOf('\n') >= 0))
                out.flush();
        }
    }
    catch (InterruptedIOException x) {
        Thread.currentThread().interrupt();
    }
    catch (IOException x) {
        trouble = true;
    }
}

notice the synchronization.

Answer 2

Jean-Francois's solution is the correct one: you absolutely must have some sort of synchronization when threads access a shared variable, whether it's via volatile, synchronized, etc.

I would also add that your while loop amounts to what is called busy waiting - that is, repeatedly testing a condition in a concurrent setting. The tight loop of the busy waiting in this code may hog the CPU. At the very least its effects on system resources will be unpredictable.

You may want to explore the condition variable approach to dealing with multiple threads being affected by a single shared condition. Java has many higher level tools for this in the java.util.concurrent library, but it's good to know the older lower-level API methods, especially since you are working directly with Thread instances already.

Every Object has wait() and notifyAll() methods. The Object represents the condition, or at least the monitor associated with it. The wait() method is called within a while loop that tests the condition, and blocks the calling thread until some other thread calls notifyAll(). Then all waiting threads will be awakened, and they will all have compete for the lock and the chance to test the condition again. If the condition just stays true at that point, then all the threads will proceed.

Here's what your code would look like using this approach:

public class GuardedBlock {

    private boolean guard = false;

    private static void threadMessage(String message) {
        System.out.println(Thread.currentThread().getName() + ": " + message);
    }

    public static void main(String[] args) throws Exception {
        GuardedBlock guardedBlock = new GuardedBlock();

        Thread thread1 = new Thread(new Runnable() {

            @Override
            public void run() {
                try {
                    Thread.sleep(1000);
                    synchronized (guardedBlock) {
                        guardedBlock.guard = true;
                        guardedBlock.notifyAll();
                    }
                    threadMessage("Set guard=true");
                } catch (InterruptedException e) {
                    e.printStackTrace();
                }

            }
        });

        Thread thread2 = new Thread(new Runnable() {

            @Override
            public void run() {
                threadMessage("Start waiting");
                while (!guardedBlock.guard) {
                    synchronized (guardedBlock) {
                        try {
                            guardedBlock.wait();
                        }
                        catch (InterruptedException e) {
                            e.printStackTrace();
                        }
                    }
                }
                threadMessage("Finally!");
            }
        });

        thread1.start();
        thread2.start();
        thread2.join();

        System.out.println("Done");
    }
}

Notice the following:

• Lock must be taken whenever condition is read or written (via synchronized keyword.)
• guard condition is tested inside a while loop, but that loop blocks during the wait() call. The only reason it is still a while loop is to handle situations where there are many threads and the condition changes many times. Then the condition should be re-tested when a thread is awakened, in case another thread changes the condition in the tiny gap between the awakening and the re-taking of the lock.
• Waiting threads are notified when guard condition is set to true (via notifyAll() calls.)
• Program blocks on the thread2 instance at the very end, so that we don't exit the main thread before all threads finish (via the join() call.)

If you look at the Object API, you'll see there's also a notify() method. It's simpler to always use notifyAll(), but if you want to understand the difference between these two methods, see this SO post.