Assigning a object to a field defined outside a synchronized block - is it thread safe?

Question

Is there anything wrong with the thread safety of this java code? Threads 1-10 add numbers via sample.add(), and Threads 11-20 call removeAndDouble() and print the results to stdout. I recall from the back of my mind that someone said that assigning item in same way as I've got in removeAndDouble() using it outside of the synchronized block may not be thread safe. That the compiler may optimize the instructions away so they occur out of sequence. Is that the case here? Is my removeAndDouble() method unsafe?

Is there anything else wrong from a concurrency perspective with this code? I am trying to get a better understanding of concurrency and the memory model with java (1.6 upwards).

import java.util.*;
import java.util.concurrent.*;

public class Sample {

    private final List<Integer> list = new ArrayList<Integer>();

    public void add(Integer o) {
        synchronized (list) {
            list.add(o);
            list.notify();
        }
    }

    public void waitUntilEmpty() {
        synchronized (list) {
            while (!list.isEmpty()) {
                try { 
                    list.wait(10000);  
                 } catch (InterruptedException ex) { }
            }
        }
    }

    public void waitUntilNotEmpty() {
        synchronized (list) {
            while (list.isEmpty()) {
                try { 
                    list.wait(10000);  
                 } catch (InterruptedException ex) { }
            }
        }
    }

    public Integer removeAndDouble() {
        // item declared outside synchronized block
        Integer item; 
        synchronized (list) { 
            waitUntilNotEmpty();
            item = list.remove(0);
        }
        // Would this ever be anything but that from list.remove(0)?
        return Integer.valueOf(item.intValue() * 2);
    }

    public static void main(String[] args) {
        final Sample sample = new Sample();

        for (int i = 0; i < 10; i++) {
            Thread t = new Thread() {
                public void run() {
                    while (true) {
                        System.out.println(getName()+" Found: " + sample.removeAndDouble());
                    }
                }
            };
            t.setName("Consumer-"+i);
            t.setDaemon(true);
            t.start();
        }

        final ExecutorService producers = Executors.newFixedThreadPool(10);
        for (int i = 0; i < 10; i++) {
            final int j = i * 10000;
            Thread t = new Thread() {
                public void run() {
                    for (int c = 0; c < 1000; c++) {
                        sample.add(j + c);
                    }
                }
            };
            t.setName("Producer-"+i);
            t.setDaemon(false);
            producers.execute(t);
        }

        producers.shutdown();
        try {
            producers.awaitTermination(600, TimeUnit.SECONDS);
        } catch (InterruptedException e) {
            e.printStackTrace();
        }

        sample.waitUntilEmpty();        
        System.out.println("Done.");
    }
}

Answer 1

It looks thread safe to me. Here is my reasoning.

Everytime you access list you do it synchronized. This is great. Even though you pull out a part of the list in item, that item is not accessed by multiple threads.

As long as you only access list while synchronized, you should be good (in your current design.)

Answer 2

Your code as-is is in fact thread safe. The reasoning behind this is two part.

The first is mutual exclusion. Your synchronization correctly ensures that only one thread at a time will modify the collections.

The second has to do with your concern about compiler reordering. Youre worried that the compile can in fact re order the assigning in which it wouldnt be thread safe. You dont have to worry about it in this case. Synchronizing on the list creates a happens-before relationship. All removes from the list happens-before the write to Integer item. This tells the compiler that it cannot re order the write to item in that method.

Answer 3

Your synchronization is fine, and will not result in any out-of-order execution problems.

However, I do notice a few issues.

First, your waitUntilEmpty method would be much more timely if you add a list.notifyAll() after the list.remove(0) in removeAndDouble. This will eliminate an up-to 10 second delay in your wait(10000).

Second, your list.notify in add(Integer) should be a notifyAll, because notify only wakes one thread, and it may wake a thread that is waiting inside waitUntilEmpty instead of waitUntilNotEmpty.

Third, none of the above is terminal to your application's liveness, because you used bounded waits, but if you make the two above changes, your application will have better threaded performance (waitUntilEmpty) and the bounded waits become unnecessary and can become plain old no-arg waits.

Answer 4

Your code is thread-safe, but not concurrent (as in parallel). As everything is accessed under a single mutual exclusion lock, you are serialising all access, in effect access to the structure is single-threaded.

If you require the functionality as described in your production code, the java.util.concurrent package already provides a BlockingQueue with (fixed size) array and (growable) linked list based implementations. These are very interesting to study for implementation ideas at the very least.