Yet another ConcurrentModificationException question

I am currently trying to learn how to properly handl开发者_StackOverflowe multi-threaded access to Collections, so I wrote the following Java application.

As you can see, I create a synchronized ArrayList which I try to access once from within a Thread and once without.

I iterate over the ArrayList using a for loop. In order to prevent multiple access on the List at the same time, I wrapped the loop into a synchronized block.

public class ThreadTest {

    Collection<Integer> data    = Collections.synchronizedList(new ArrayList<Integer>());
    final int           MAX     = 999;

    /**
     * Default constructor
     */
    public ThreadTest() {
        initData();
        startThread();
        startCollectionWork();
    }

    private int getRandom() {
        Random randomGenerator = new Random();
        return randomGenerator.nextInt(100);
    }

    private void initData() {
        for (int i = 0; i < MAX; i++) {
            data.add(getRandom());
        }
    }

    private void startCollectionWork() {
        System.out.println("\nStarting to work on data outside of thread");
        synchronized (data) {
            System.out.println("\nEntered synchronized block outside of thread");
            for (int value : data) { // ConcurrentModificationException here!
                if (value % 5 == 1) {
                    System.out.println(value);
                    data.remove(value);
                    data.add(value + 1);
                } else {
                    System.out.println("value % 5 = " + value % 5);
                }
            }
        }
        System.out.println("Done working on data outside of thread");
    }

    private void startThread() {
        Thread thread = new Thread() {
            @Override
            public void run() {
                System.out.println("\nStarting to work on data in a new thread");
                synchronized (data) {
                    System.out.println("\nEntered synchronized block in thread");
                    for (int value : data) { // ConcurrentModificationException
                        if (value % 5 == 1) {
                            System.out.println(value);
                            data.remove(value);
                            data.add(value + 1);
                        } else {
                            System.out.println("value % 5 = " + value % 5);
                        }
                    }
                }
                System.out.println("Done working on data in a new thread");
            }
        };
        thread.start();
    }
}

But everytime one of the for loops gets entered, I get a ConcurrentModificationException. This is my console output (which changes with every new run):

Starting to work on data outside of thread

Entered synchronized block outside of thread
51

Starting to work on data in a new thread

Entered synchronized block in thread
value % 5 = 2
value % 5 = 2
value % 5 = 4
value % 5 = 3
value % 5 = 2
value % 5 = 2
value % 5 = 0
21
Exception in thread "main" java.util.ConcurrentModificationException
    at java.util.AbstractList$Itr.checkForComodification(AbstractList.java:372)
    at java.util.AbstractList$Itr.next(AbstractList.java:343)
    at ThreadTest.startCollectionWork(ThreadTest.java:50)
    at ThreadTest.<init>(ThreadTest.java:32)
    at MultiThreadingTest.main(MultiThreadingTest.java:18)
Exception in thread "Thread-1" java.util.ConcurrentModificationException
    at java.util.AbstractList$Itr.checkForComodification(AbstractList.java:372)
    at java.util.AbstractList$Itr.next(AbstractList.java:343)
    at ThreadTest$1.run(ThreadTest.java:70)

What's wrong?

Ps: Please don't just post links to multi-threaded how-to's since I've already read enough about that. I am just curious why my application doesn't run as I want it to.

Update: I replaced the for(x : y) syntax with an explicit Iterator and a while loop. The problem remains though..

synchronized(data){
        Iterator<Integer> i = data.iterator();
        while (i.hasNext()) {
        int value = i.next(); // ConcurrentModificationException here!
        if (value % 5 == 1) {
            System.out.println(value);
            i.remove();
            data.add(value + 1);
        } else {
            System.out.println("value % 5 = " + value % 5);
        }
    }
}

Once you iterate over a collection, you have a contract between the iterating block of code and the collection as it exists at that moment in time. The contract basically states that you'll get each item in the collection once, in the order of the iteration.

The problem is that if you modify the Collection while something is iterating, you cannot maintain that contract. Deletions in a collection will remove the element from the collection, and that element might be required to be present for the initial iteration to satisfy the contract. Insertions in a Collection will likewise present issues if the element might be detected by the iteration that started prior to the element existing in the collection.

While it is easier to break this contract with multiple threads, you can break the contract with a single thread (if you choose to do so).

How this is typically implemented is the collection contains a "revision number", and prior to the iterator grabbing the "next" element in the collection, it checks to see if the collection's revision number is still the same as it was when the iterator started. This is just one way of implementing it, there are others.

So, if you want to iterate over something that you might want to change, an appropriate technique is to make a copy of the collection and iterate over that copy. That way you can modify the original collection and yet not alter the count, position, and presence of the items you were planning to process. Yes, there are other techniques, but conceptually they all fall into the "protect the copy you're iterating across while changing something else that the iterator doesn't access".

The ConcurrentModificationException appears, because you're modifing the list while iterating it... it has nothing to do with multiple threads in this case...

for (int value : data) { // ConcurrentModificationException here!
                if (value % 5 == 1) {
                    System.out.println(value);
                    data.remove(value); // you cannot do this
                    data.add(value + 1); // or that
                } else {
                    System.out.println("value % 5 = " + value % 5);
                }

The enhanced for loop

for (int value : data) { 

uses a Java Iterator under the covers. Iterators are fail-fast, so if the underlying Collection gets modified (i.e. by removing an element) while the Iterator is active you get the Exception. Your code here causes such a change to the underlying Collection:

data.remove(value);
data.add(value + 1);

Change your code to use java.util.Iterator explicitly and use its remove method. If you need to add elements to the Collection while iterating you may want to look at a suitable data structure from java.util.concurrent package e.g. BlockingQueue, where you can call its take method which will block until there is data present; but new Objects can be added via the offer method (very simplistic overview - Google for more)

The problem is not because of multithreading, you made if safe. But only because you modified collection while iterating it.

You are only allowed to remove items from the list through an iterator if you are iterating over the collection, so you can get ConcurrentModificationException with only one thread.

Updated reply after updated question: You aren't allowed to add elements to the list while you are iterating.

As has been pointed out, you are modifying the Collection while iterating over it, which causes your problem.

Change data to a List and then use a regular for loop to step over it. Then you will no longer have an Iterator to deal with, thus eliminating your problem.

List<Integer> data =...

for (int i=0; i<data.size(); i++) {
        int value = data.get(i);

        if (value % 5 == 1) {
            System.out.println(value);
            i.remove();
            data.add(value + 1);
        } else {
            System.out.println("value % 5 = " + value % 5);
        }
}

ConcurrentModificationException is not eliminated by using a synchronized block around the collection on which iteration is done. The exception occurs on the following sequence of steps :

1. Obtain an iterator from a collecion ( by calling its iterator method or by the for loop construct ).
2. Begin iterating ( by calling next() or in the for loop )
3. Have the collection modified ( by any means other than the iterator's methods ) ( either in the same thread or a different one : this is what is happening in your code ). Note that this modification may happen in a thread-safe manner ie sequentially or one after another - it does not matter- it will still lead to CME )
4. Continue iteration using the same iterator obtained earlier ( before modification in step 3 )

In order to avoid getting the exception , you must make sure that you do not modify the collection after you start your for loop in either of the threads, till the loop is finished.