Java thread spontaneously waking up

Question

I've got a Java thread does something like this:

while (running) {
    synchronized (lock) {
        if (nextVal == null) {
            try {
                lock.wait();
            } catch (InterruptedException ie) {
                continue;
            }
        }

        val = nextVal;
        nextVal = null;
    }
    ...do stuff with 'val'...
}

Elsewhere I set the value like this:

if (val == null) {
    LOG.error("null value");
} else {
    synchronized (lock) {
        nextVal = newVal;
        lock.notify();
    }
}

Occasionally (literally once every couple of million times) nextVal will be set to null. I've tossed in logging messages and I can see that the order of execution looks like this:

1. thread1 sets nextVal to newVal
2. thread1 calls lock.notify()
3. thread2 wakes up from lock.wait()
4. thread2 sets val to nextVal
5. thread2 sets nextVal to null
6. thread2 does stuff with val
7. thread2 calls lock.wait()
8. thread2 wakes up from lock.wait()
   • no other thread has called lock.notify() and thread2 has not been interrupted
9. thread2 sets val to nextVal (which is null)
10. etc.

I've explicitly checked and the lock is waking up a second time, it's not being interrupted.

Am I doing something wrong here?

Answer 1

Yup, Threads spontaneously wake up. The this is explicitly stated in the Javadoc:

"A thread can also wake up without being notified, interrupted, or timing out, a so-called spurious wakeup."

You need to wait in a loop. This is also explicitly mentioned in the javadoc:

 synchronized (obj) {
     while (<condition does not hold>)
         obj.wait(timeout);
     ... // Perform action appropriate to condition
 }

In your case:

while (running) {
    synchronized (lock) {
        while (nextVal == null) {
            try {
                lock.wait();
            } catch (InterruptedException ie) {
                //oh well
            }
        }

        val = nextVal;
        nextVal = null;
    }
    ...do stuff with 'val'...
}

Answer 2

Spurious wake ups are fairly common and hence it's always advised to wait() on a condition within a loop.

Change your code as follows:

while (nextVal == null) {
    try {
        lock.wait();
    } catch (InterruptedException ignored) {
    }
}

Specific to the code you've shared: while also helps you avoid the unnecessary overhead of releasing and re-acquiring the same lock when your code hits continue;

References:
http://docs.oracle.com/javase/1.5.0/docs/api/java/lang/Object.html#wait()

From the public final void wait() documentation:
... interrupts and spurious wakeups are possible, and this method should always be used in a loop:

 synchronized (obj) {
     while (<condition does not hold>)
         obj.wait();
     ... // Perform action appropriate to condition
 }

Answer 3

This could be a spurious wakeup, but that is not the only possible cause. It is definitely a problem with your logic. You need to put the wait inside a loop that retests for the condition.

When a thread wakes up from the wait, it doesn't have the lock anymore. It released the lock when it started waiting, it needs to reacquire the lock before it can proceed. The just-awoken thread may typically be next in line due to thread affinity (which is probably why your code works most of the time), but there is still the chance it's not; another thread can come in and snag the lock, do its thing and leave the nextVal null, before the awoken thread can take the lock. That means the test for null that the thread made before waiting is no longer relevant. You have to go back and test again once you have the lock.

Change the code to use a loop, like:

synchronized(lock) {
    while (nextVal == null) {
        lock.wait();
    }
   ...

This way the test is made while the thread has the lock, and whatever happens in the block below the while loop can be certain nextVal really isn't null.