static volatile boolean - thread not getting terminated

Question

I wrote simple multithreaded application, just to play around with concurrency but I have a problem with boolean variable which controles the loop in thread. One of the functions should stop the thread if there's noelements left in queue and I guess that is my problem because If I add something in between braces to:

while (!queue.isEmpty()) {
}
isRunning = false;

So it becomes :

while (!queue.isEmpty()) {
    System.out.println("ASD");
}
isRunning = false;

It is working a bit better - the program terminates after executing turnOff method

Any Ideas?

Here is full code of my app:

package test;

public class xxx {
    public static void main(String[] args) {
        Foo instance = Foo.getInstance();
        Thread x = new Thread(instance);
        x.start();

        for (int count = 1; count < 100000; count++)
            instance.addToQueue(count + "");
        instance.turnOff();
    }
}

And:

package test;

import java.util.LinkedList;
import java.util.List;

public class Foo implements Runnable {
    private static Foo inner = null;
    private static List<String> queue = new LinkedList<String>();
    private volatile static boolean isRunning = false;

    private Foo() { }

    public static Foo getInstance() {
        if (inner == null) {
            inner = new Foo();
        }
        return inner;
    }

    public void addToQueue(String toPrint) {
        synchronized (queue) {
            queue.add(toPrint);
        }

    }

    public void removeFromQueue(String toRemove) {
        synchronized (queue) {
            queue.remove(toRemove);
        }
    }

    public void turnOff() {
        while (!queue.isEmpty()) {
        }
        System.out.println("end");
        isRunning = false;
    }

    @Override
    public void run() {
        isRunning = true;
        while (isRunning) {
            if (!queue.isEmpty()) {
                String string = queue.get(0);
                System.out.println(string);
                removeFromQueue(string);
            }

        }
    }
}

Answer 1

It is a race condition problem. Possibly the run method (the other thread) is executed after the turnOff in in the main thread so the flag isRunning is set as true again and the loop never ends.

That would explain why with a simple System.out.println("ASD") becomes better: the isRunning=false is delayed.

Answer 2

You have lots of problems in your code.

1. Busy loops in turnOff and wait
2. Unsynchronized access to queue in turnOff and run
3. Non-volatile, non-final access to inner
4. Needlessly static isRunning and queue variables
5. Race condition between turnOff and start invocations

Some of these are harmless in this specific instance (e.g. instance is always accessed from the main thread), but depending on your hardware configuration you are going to get bitten by some combination of the rest of them. The reason that adding the System.out "fixes" the problem is that it renders one of the busy loops less busy (fixes 1) and has an internal synchronization mechanism (fixes 2), but the others are still there.

I suggest getting rid of the isRunning variable and the test for queue.isEmpty() and replacing with a CountDownLatch.

package test;

import java.util.LinkedList;
import java.util.List; 
import java.util.concurrent.CountDownLatch;

public class Foo implements Runnable {
    private static final Foo inner = new Foo();
    private final List<String> queue = new LinkedList<String>();
    private final CountDownLatch latch = new CountDownLatch(1);

    private Foo() { }

    public static Foo getInstance() {
        return inner;
    }

    public void addToQueue(String toPrint) {
        synchronized (queue) {
            queue.add(toPrint);
        }
    }

    public void removeFromQueue(String toRemove) {
        synchronized (queue) {
            queue.remove(toRemove);
        }
    }

    public boolean isEmpty() {
        synchronized (queue) {
            return queue.isEmpty();
        }
    }

    public String getHead() {
        synchronized (queue) {
            return queue.get(0);
        }
    }

    public void turnOff() throws InterruptedException {
        latch.await();
        System.out.println("end");
    }

    @Override
    public void run() {
        while (!isEmpty()) {
            String string = getHead();
            System.out.println(string);
            removeFromQueue(string);
        }

        latch.countDown();
    }
}

And the runner

package test;

public class XXX {
    public static void main(String[] args) throws InterruptedException {
        Foo instance = Foo.getInstance();
        Thread x = new Thread(instance);

        for (int count = 1; count < 100000; count++)
            instance.addToQueue(count + "");

        x.start();
        instance.turnOff();
    }
}   

Answer 3

The main problem is the race condition between adding/removing elements and checking whether the queue is empty. In more words:

Wrapping add and remove calls in synchronized block provides you guarantees that all invocations of these methods will be performed sequentially. But, there is one more access to queue variable outside of synchronized block - it is queue.isEmpty(). It means there is a chance that some thread will get the result of this call and while it performs actions inside if block, other thread may add or remove elements.

This code also has some more concurrency problems, please let me know if you want them to be discussed (they are a little bit offtopic).

Answer 4

As Germann Arlington point, the value of queue.isEmpty() seems to be cached in the main thread. Try synchronize it:

while (true) {
    synchronized(queue) {
        if(queue.isEmpty())
            break;
    }
} 

Or just make the queue to be volatile:

private volatile static List<String> queue = new LinkedList<String>();

Answer 5

This will solve your problem.

Use volatile variable isRunning in turnOff() method's while loop also.

public void turnOff() {
    while (isRunning && !queue.isEmpty()) {
    }
    System.out.println("end");
    isRunning = false;
}