Is there really a race condition in this multi-threaded java code?

Question

I saw a snippet of code in this question which I could not understand (most probably due to the fact am a beginner in this area). The question talks about "an obvious race condition where sometimes the producer will finish, signal it, and the ConsumerWorkers will stop BEFORE consuming everything in the queue."

1. In my understanding, "isRunning" will be set on the consumers only after the producer decides not to add anymore items in the queue. So, if a consumer thread sees isRunning as FALSE AND then sees inputQueue is empty, then there is NO possibility of anything more getting added into the queue in the future. Obviosuly, I am wrong and missing something, as no one who responded to that question said the scenario of the question is impossible. So, Can someone pls explain what sequence of events causes this race condition ?

2. In fact, I see a problem with something else. For ex, if multiple consumer threads saw that the producer isRunning, and say the queue had ONE item, many threads could enter the blocked 'take'. If the producer STOPS now, while one thread would come out of the 'take', the other threads are blocked on the 'take' forever. Interestingly, no one who answered the question pointed out this problem as well. So, my understanding of this is also probably faulty ?!

I didnt want to add this as a comment there in that question, as it is an old question and my doubt may never get answered ! I am copy/placing the code from that question here for quick reference.

public class ConsumerWorker implements Runnable{

private BlockingQueue<Produced> inputQueue;
private volatile boolean isRunning = true;

public ConsumerWorker(BlockingQueue<Produced> inputQueue) {
    this.inputQueue = inputQueue;
}

@Override
public void run() {
    //worker loop keeps taking en element from the queue as long as the producer is still running or as 
    //long as the queue is not empty:
    while(isRunning || !inputQueue.isEmpty()) {
        System.out.println("Consumer "+Thread.currentThread().getName()+" START");
        try {
            Object queueElement = inputQueue.take();
            //process queueElement
        } catch (Exception e) {
            e.printStackTrace();
        }
    }
}

//this is used to signal from the main thread that he producer has finished adding stuff to the queue
public void setRunning(boolean isRunning) {
    this.isRunning = isRunning;
}

Centillion

Answer 1

I think OP of the original question probably meant

while(isRunning && !inputQueue.isEmpty()) 

rather than

while(isRunning || !inputQueue.isEmpty())

The former clearly produces the issue described by the original poster (*), while the later does indeed have the problem you described in your second point. A simple oversight there, but now we can note that both approaches are incorrect.

_{(*) and somehow assumes that the queue will never be empty.}

Ecology

I had the same thought. And I did check the 'edits' on that question to see if the OP changed the question and modified the operator. But, he hadnt. That operator was always ||. And no one in that qn, questioned this ! – Centillion

that's probably because it was not necessary, since both expressions are wrong, a solution was necessary. A simple oversight really. – Ecology

Answer 2

You are correct in both questions. Yes && is correct and || is not. As for the second question, answers was to use poison pill or timeout, both ways resolving the problem.

As for me, I would create new synchronization class which aggregates both the queue and isRunning variable, so that changing isRunning causes an exception in take() thus signalling the end of work.

Mcloughlin

I actually liked @PeterLawrey response the best. But thats me. – Centillion