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

If your ExecutorService is single threaded then there is no shared state, so I don't see how there could be any issues around that.

However wouldn't it make more sense to pass a new instance of your Test class to each call to execute()? i.e.

for (int i=0; i<10; ++i)
    ex.execute(new Test());

This way there will not be any shared state.

Answer 2

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 3

Your code, specifically this bit

            if (this.state != -1)
                    this.state++;

would require the atomic test of the state value and then an increment to the state in a concurrent context. So even if your variable was volatile and more than one thread was involved, you would have concurrency issues.

But your design is based on asserting that there will always only be one instance of Test, and, that single instance is only granted to a single (same) thread. (But note that the single instance is in fact a shared state between the main thread and the executor thread.)

I think you need to make these assumptions more explicit (in the code, for example, use the ThreadLocal and ThreadLocal.get()). This is to guard both against future bugs (when some other developer may carelessly violate the design assumptions), and, guard against making assumptions about the internal implementation of the Executor method you are using which may in some implementations simply provide single threaded executor (i.e. sequential and not necessarily the same thread in each invocation of execute(runnable).

Answer 4

It is perfectly fine for state to be non-volatile in this specific code, because there is only one thread, and only that thread accesses the field. Disabling caching the value of this field within the only thread you have will just give a performance hit.

However if you wish to use the value of state in the main thread which is running the loop, you have to make the field volatile:

    for (int i=0; i<10; ++i) {
            ex.execute(test);
            System.out.println(test.getState());
    }

However, even this might not work correctly with volatile, because there is no synchronization between the threads.

Since the field is private, there is only an issue if the main thread executes a method that can access this field.

Answer 5

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 6

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