Code with notify and notifyAll / synchronization (Help to understand)

Question

I'm studying for an exam from a Book given by my professor and there is this code working with Threads and Synchronization: We want to be notified everytime the state changes (without missing a state change).

public class C {

    private int state = 0; // the state
    private boolean modified = false; // to show if the state was changed since actualization

    public synchronized void printNewState() {
        while (true) {
            if (!modified) {
                wait();
            }
            System.out.println(state);
            modified = false;
            notify();
        }
    }

    public synchronized void setValue(int v) {
        if (modified) {
            wait();
        }
        state = v;
        notify();
        modified = true;
        System.out.println("value set");
    }
}

And then it's writen:
However, it is not guaranteed that notify() in the method SetValue(int) wakes up the printNewState Thread! In Java we solve this problem with the help of notifyAll() and take a little busy waiting:

public synchronized void printNewState() {
    while (true) {
        while (!modified) {
            wait();
        }
        System.out.println(state);
        modified = false;
        **notify();** \\(?)
    }
}

public synchronized void setValue(int v) {
    while (modified) {
        wait();
    }
    state = v;
    notifyAll();
    modified = true;
    System.out.println("value set");
}

I don't understand why the notify wasn't also changed to notifyAll()? It might not be guaranteed that this notify goes to a Thread of setValue(int) ???

Thank you

Answer 1

The notify() method wakes up a single waiting thread, whereas the notifyAll() method wakes up all the waiting threads. The problem is that with notify(), the thread that is woken up is effectively random.

If some other thread is accidentally or maliciously wait()ing on the same object, it could receive the notification instead of the thread you expect to wake up.

Take a look at this answer for some more information. The comments on that answer are also quite interesting.

EDIT

In the code sample you posted, the first notify() within printNewState() can handle only one update at a time, so it doesn't make sense to notify all waiting threads to post their updates. The code assumes, however, that only threads invoking setValue(int) are waiting.

Since public synchronized void setValue(int) is essentially the same as having synchronized(this) as the first line of the method, that isn't actually guaranteed. Any code that has a reference to an instance of C class can wait on it and screw up the code.

The synchronization and wait/notify actions should happen on an object lock/monitor. private final Object monitor = new Object(), synchronized(this.monitor), this.monitor.wait(), this.monitor.notifyAll(), etc.

I would also like to note that modified = true needs to be placed before notifyAll() in setValue(int), otherwise other waiting update threads will proceed without printNewState() noticing the update.

Also, private boolean modified should really be private volatile boolean modified, and the same for private int state, though an AtomicInteger may be a better option.