Is the following code that retrieves an integer value from some queue implementation correct

Question

I was reading something from the internet. Here is a question about the code below. Is the following code that retrieves an integer value from some queue implementation correct? And the answer is like this:

Although the code above uses the queue as object monitor, it does not behave correctly in a multi-threaded environment. The reason for this is that it has two separate synchronized blocks. When two threads are woken up in line 6 by another thread that calls notifyAll(), both threads enter one after the other the second synchronized block. It this second block the queue has now only one new value, hence the second thread will poll on an empty queue and get null as return value.

I was thinking the synchronized block will prevent different threads to access the this resource at the same time, is it not? Thank you.

public Integer getNextInt() {

    Integer retVal = null;

    synchronized (queue) {
        try {
            while (queue.isEmpty()) {
                queue.wait();
           }
        } catch (InterruptedException e) {
            e.printStackTrace();
        }

    }

    synchronized (queue) {
        retVal = queue.poll();
        if (retVal == null) {
            System.err.println("retVal is null");
            throw new IllegalStateException();
        }
    }

    return retVal;
}

Answer 1

The problem is much broader than described in your question. It doesn’t even need two notified threads (or a notifyAll) to get into trouble. The problem is that thread scheduling is indeterminate.

Consider the following scenario:

• Thread A enters the first synchronized block, finds an empty queue and waits
• Another thread puts one item into the queue and notifies one thread.
• Thread A wakes up and leaves the first synchronized block
• Thread B enter the first synchronized block and finds a non-empty queue and doesn’t wait!
• Thread B leaves the first synchronized block

So now, two threads, A and B, are about to enter the second synchronized block and the order doesn’t matter as at this time it’s clear that only one thread can consume an item and the other one will fail (unless another thread happens to put something into the queue in-between).

Actually, it doesn’t even need any notification, e.g. if two threads invoke this method when the queue holds exactly one element. They both may execute the first block finding the queue to be non-empty, before both attempt to execute the second block.

---

The bottom line is that if you have some sort of condition, the code performing the wait for the condition (implies checking the condition) and the code relying on the condition have to be in one synchronized block or, in the case of other kind of locks, must hold the lock throughout the entire code.

Answer 2

Ah, I understand this now.. So if 2 thread were notified at the same time, one enter the block and polled the queue, the second was waiting because of the synchronized block. Then when the second thread enters, the queue already empty... Thank you so much. This was an example I found on the internet.