Threads and Concurrent Modification Exception working with a list

Question

I know this is a daft questions but cant work out how to fix this, I have not had much experience with using threads before.

Below should create first of all a timer which will execute the command output.write(mylist) every 10 seconds which will simply output the contents of mylist.

Secondly it loops through about 3 lists I have and for each them creates a thread which will continue to loop through getting the next word in the list. Please note: this is stripped down version and not complete so please don't comment on the arraylist / list but rather the error itself.

There is a concurrent modification exception happening frequently but not all the time when it tries to do output.write(). I am guessing this is because one of the other threads are currently saving something to mylist? How would I go about fixing this?

    Runnable timer = new Runnable() {
        public void run() {
            try {
                while (true) {
                    Thread.sleep(10000);
                    output.write(mylist);
                }
            } catch (InterruptedException iex) {}
        }
    };
    Thread timerThread = new Thread(timer);
    timerThread.start();

    for (final ValueList item : list) {

        Runnable r = new Runnable() {
            public void run() {
                try {
                    while (true) {

                        char curr = item.getNext();

                         mylist.addWord(curr);
                    }
                } catch (InterruptedException iex) {}
            }
        };

        Thread thr = new Thread(r);
        thr.start();
    }
}

Answer 1

There is a concurrent modification exception happening frequently but not all the time when it tries to do output.write...

The problem is (I assume) that the output.write(...) method is iterating through your myList at the same time as another thread is calling myList.addWord(curr);. This is not allowed unless myList is a concurrent collection.

How would I go about fixing this?

You will need to synchronize on myList every time you are accessing it -- in this case when you are outputting it or adding a word to it.

  synchronized (myList) {
      output.write(mylist);
  }
  ...

  synchronized (myList) {
      myList.addWord(curr);
 }

In this case, because output.write(mylist) is probably iterating through the list, you cannot use the Collections.synchronizedList(...) method because the iterators need to be synchronized by the caller.

If this is some high-performance method that is called tons of times then you could also use a ConcurrentLinkedQueue but that is a queue, not a list obviously. ConcurrentSkipList is another option although that is a heavier data structure.

Answer 2

Just like you said, this is happening because Java iterators are fail-fast, and will fail if the collection is concurrently modified. One easy solution is to protect accesses to your list with synchronized, but this will cause your threads to stop, waiting for the output to complete.

One maybe smarter solution is to use some concurrent implementation of List. Since your list is often modified, CopyOnWriteArrayList is not the solution you want. If it doesn't require too many modifications, you might use a ConcurrentLinkedDeque or a LinkedBlockingDeque, which are backed by a linked list, and do not implement List, so they don't have a get method (which you don't seem to be using). Anyway, the iterators returned by these collections are not fail-fast, but weakly-consistent (which means they will 'see' the collection how it was when the iterator was created, or reflect some of the later modifications).

Another solution may be to use a concurrent collection (ConcurrentLinkedDeque) and a ReadWriteLock, but in a more original way. Your writer threads would use the read lock (thus being able to write concurrently). You printer thread would acquire the write lock (thus temporarily blocking the other threads), make a copy of the collection into a non-concurrent collection, release the write lock and finally print its local copy.

A definitely smarter (and probably faster) choice would be to assign a different non-concurrent list to each thread. So you would have a list of non-concurrent lists, each assigned to a single thread. When all the threads are done, you can join all your lists into a single one. Moreover, each of the lists should be protected by a lock. When your printer thread wants to print, it iterates through the lists, locking them one at a time, making a copy of it, releasing the lock, and then printing it.

The reason why I insist in making copies is that it's faster, much faster than printing I/O. If your worker threads will have to wait for the printer thread to print the list, all your computation will slow down.

PS: even if the only wrong thing you can see is the exception, ArrayLists are not thread safe. If you add elements to the same list from multiple threads, you will lose some elements (unless some weirder exception occurs).

Answer 3

Or just add synchronized to the deceleration of run(). Alternatively enclose the contents of the method in a synchronized{...} block, as long as the sleeping/waiting is in the 'synchronized zone', i.e. where only one thread is allowed to operate at a time.

Anyone reading this for something they're stuck on shoud read through:
http://docs.oracle.com/javase/tutorial/essential/concurrency/sync.html
It will be sound investment in time;)