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;
}

As your own question says, the problem is not that two threads can enter the block at the same time. The problem is that the second thread will enter the second synchronized block after the first one has already polled the queue, and will thus find an empty queue and throw an IllegalStateException. Why don't you use a BlockingQueue, that would deal with this complex synchronization for you? — JB Nizet, Jan 19 '15 at 16:15
The answer you quoted is correct. If the first sync block finishes with a non-empty queue, there is no guarantee that another thread won't jump in and poll the queue before the second sync block. — khelwood, Jan 19 '15 at 16:16
What is the purpose of the first sync block? To block people from accessing the queue while you're waiting for someone to put values in it? That seems counterproductive. — JimW, Jan 19 '15 at 16:19
@JimW. `queue.wait()` releases the lock upon entry, and re-locks the lock before it returns. One thread (or two threads, or any number of threads) waiting in the`queue.wait()` call will _not_ prevent other threads from synchronizing on queue. — Solomon Slow, Jan 19 '15 at 16:57

Holger · Accepted Answer · 2015-01-19T17:02:41.337

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.

score 0 · Answer 2 · answered Jan 19 '15 at 16:25

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.

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

2 Answers2