Adding elements to a list with multiple threads: how to make it thread-safe?

Question

I am using multiple threads via an ExecutorService to add objects to a list with several for-loops. As the normal ArrayList is not thread-safe, I am using a CopyOnWriteList. However, adding the elements as in the code example below does not give me a list of (n1+n2) elements, so somehow there is a concurrency problem.

Where is the error in my thinking? Is CopyOnWriteList the correct choice for a thread-safe list?

    public class myThread{

         List<SomeObject> list;

         public myThread(List<someObject> list){
              this.list = list;
         }

         public void run(){
             SomeObject instance = new SomeObject();
             list.add(instance);
         }
     }

     public static void main(String[] args) {

         CopyOnWriteArrayList<someObject> syncList = new CopyOnWriteList<someObject>();

         ExecutorService executor = Executors.newFixedThreadPool(4);

         for(int i=0; i< n1; i++){
             executor.submit(new myThread(syncList))
         }

         for(int i=0; i< n2; i++){
            executor.submit(new myThread(syncList))
         }
      }

CopyOnWriteList is good when read operation dominates over write operation because on every write(add,set,etc.) a new fresh copy of underlying array is made — sol4me, Dec 15 '14 at 15:56
To understand how the CopyOnWriteList works: http://stackoverflow.com/questions/17853112/in-what-situations-is-the-copyonwritearraylist-suitable — Aaron Digulla, Dec 15 '14 at 15:59
The code example above doesn't work since you never pass `syncList` into `myThread`. — Aaron Digulla, Dec 15 '14 at 15:59
I forgot to write the list as a constructor argument, but programmed it in my code. My code compiled and had the incorrect number of elements as reported. Sorry for the confusion! — madison54, Dec 15 '14 at 17:27

score 3 · Answer 1 · edited Dec 15 '14 at 16:06

CopyOnWriteArrayList is a thread safe list.

public class myThread implements Runnable {

    List<String> list;

    public myThread(List<String> list){
        this.list = list;
    }

    public void run(){
        String instance = "";
        list.add(instance);
    }


    public static void main(String[] args) throws Exception {

        CopyOnWriteArrayList<String> syncList = new CopyOnWriteArrayList<String>();

        ExecutorService executor = Executors.newFixedThreadPool(4);

        for(int i=0; i< 10; i++){
            executor.submit(new myThread(syncList));
        }

        for(int i=0; i< 20; i++){
            executor.submit(new myThread(syncList));
        }

        executor.shutdown();

        while (executor.awaitTermination(1, TimeUnit.DAYS)) {
            break;
        }

        System.out.println(syncList.size());
    }
}

You try this. You may call before all the Thread will be completed.

score 1 · Answer 2 · answered Dec 15 '14 at 16:09

The code above doesn't work since you never pass the list into the thread (which isn't a thread since it doesn't extend Thread). Try

...new myThread(syncList)...

Then it will work but the performance might be slow if the list gets very big. In that case, it might be better to synchronize on the list:

synchronized(list) { list.add(instance); }

synchronized will work well here. Note that the elements might appear in an odd order in the list. That's because both threads add their elements at the "same" time, so you won't see n1+n2 but a mix of both n1 and n2 elements. The order of all elements from n1 will be the same but there will be elements from n2 mixed between them.

score 0 · Answer 3 · answered Dec 15 '14 at 16:14

Where is the error in my thinking?

You are not sharing the list among the threads. Your code doesn't even compile. To fix the problem pass the list to myThread constructor:

for(int i=0; i< n1; i++){
      executor.submit(new myThread(syncList))
}

Is CopyOnWriteList the correct choice for a thread-safe list?

CopyOnWriteList is better used when the number of reads is much greater than the number of writes. Your case seems to be the opposite, and thus it is better to use java.util.Collections.synchronizedList().

Note: From Java Concurency in Practice:

CopyOnWriteArrayList is a concurrent replacement for a synchronized
List that offers better concurrency in some common situations and 
eliminates the need to lock or copy the collection during iteration.

Adding elements to a list with multiple threads: how to make it thread-safe?

3 Answers3