Concurrency, object visibility

Question

I'm trying to figure out if the code below suffers from any potential concurrency issues. Specifically, the issue of visibility related to volatile variables. Volatile is defined as: The value of this variable will never be cached thread-locally: all reads and writes will go straight to "main memory"

public static void main(String [] args)
{
    Test test = new Test();

    // This will always single threaded
    ExecutorService ex = Executors.newSingleThreadExecutor();

    for (int i=0; i<10; ++i)
        ex.execute(test);
}

private static class Test implements Runnable {
    // non volatile variable in question
    private int state = 0;

    @Override
    public void run() {
        // will we always see updated state value? Will updating state value
        // guarantee future run's see the value?
        if (this.state != -1)
            this.state++;
    }
}

For the above single threaded executor:

Is it okay to make test.state non volatile? In other words, will every successive Test.run() (which will occur sequentially and not concurrently because again executor is single threaded), always see the updated test.state value? If not, doesn't exiting of Test.run() ensure any changes made thread locally get written back to main memory? Otherwise when does changes made thread locally get written back to main memory if not upon exiting of the thread?

Answer 1

As long as it's only a single thread there is no need to make it volatile. If you're going to use multiple threads, you should not only use volatile but synchronize too. Incrementing a number is not an atomic operation - that's a common misconception.

public void run() {
    synchronize (this) {
        if (this.state != -1)
            this.state++;
    }
}

Instead of using synchronization, you could also use AtomicInteger#getAndIncrement() (if you won't need an if before).

private AtomicInteger state = new AtomicInteger();

public void run() {
    state.getAndIncrement()
}

Answer 2

Originally, I was thinking this way:

If the task were always executed by the same thread, there would be no problem. But Excecutor produced by newSingleThreadExecutor() may create new threads to replace a those that are killed for any reason. There is no guarantee about when the replacement thread will be created or which thread will create it.

If a thread performs some writes, then calls start() on a new thread, those writes will be visible to the new thread. But there is no guarantee that that rule applies in this case.

But irreputable is right: creating a correct ExecutorService without sufficient barriers to ensure visibility is practically impossible. I was forgetting that detecting the death of another thread is a synchronizes-with relationship. The blocking mechanism used to idle worker threads would also require a barrier.

Answer 3

Yes it is safe, even if the executor replaced its thread in the middle. Thread start/terminate are also synchronization points.

http://java.sun.com/docs/books/jls/third_edition/html/memory.html#17.4.4

A simple example:

static int state;
static public void main(String... args) {
    state = 0;                   // (1)
    Thread t = new Thread() {
        public void run() {
            state = state + 1;   // (2) 
        }
    };
    t.start();
    t.join();
    System.out.println(state);  // (3)
}

It is guaranteed that (1), (2), (3) are well ordered and behave as expected.

For the single thread executor, "Tasks are guaranteed to execute sequentially", it must somehow detect the finish of one task before starting the next one, which necessarily properly synchronizes the different run()'s