ConcurrentModificationException: .add() vs .addAll()

Question

Why does the following occur? Shouldn't both work?

List<String> items = data;
for( String id : items ) {
    List<String> otherItems = otherData;        

    // 1.   addAll()
    //Causes ConcurrentModificationException
    items.addAll(otherItems);

    // 2.   .add()
    //Doesn't cause exceptions
    for( String otherId : otherItems ) {
        items.add(otherId);
    }
}

Is it because add() adds to the collection Items, but addAll() creates a new collection thus modifying Items to be a different instance of List?

Edit items and otherItems are of concrete type ArrayList<String>.

Answer 1

Neither operation is proper, since it modifies the collection while iterating it.

Examining the implementation of ArrayList shows that calling either add or addAll should successfully throw the ConcurrentModificationException on the next loop iteration. The fact that it isn't doing that for add means that either there is an obscure bug in the ArrayList class for the particular version of Java you have; or (more likely) otherItems is empty, so in the second case, you aren't actually calling add at all.

I'm sure otherItems must be empty, because if adding to the Items list "worked" in the way you want, then it would grow every time around the loop, causing it to loop indefinitely until dying with an OutOfMemoryError.

Answer 2

The for loop you have here

for( String id : Items ) {

is logically the same as:

for(Iterator<String> it = Items.iterator(); it.hasNext();) {
    String id = it.next();

    ....
}

Now, if you modify the list an iterator iterates over, in the middle of iterating, you get the ConcurrentModificationException. From the Javadoc for ArrayList:

The iterators returned by this class's iterator and listIterator methods are fail-fast: if the list is structurally modified at any time after the iterator is created, in any way except through the iterator's own remove or add methods, the iterator will throw a ConcurrentModificationException.

So, the addAll() is modifying Items, and causing the iterator to fail.

Your only solution is to either:

1. don't do the addAll()

2. change to the loop with the iterator, and do:

   for (String other : otherItems) {
       it.add(other);
   }
   

   In other words, do the add via the iterator, and avoid the ConcurrentModification....

Now, about why add() works, and addAll() does not? I believe you are probably just seeing something like the add version when there are no other items in the iterator, or the values being added are emtpry, Perhaps a bug in the implementation of Items. It should be throwing a CME, and the fact that it is not throwing one, means there is a bug, not in your code, but in the collection.

They should both fail!

BUT: You have subsequently found that the addAll() was adding an empty collection. An empty addAll() should not be causeing a CME... and, as @Boann points out, this is a bug in the implementation of ArrayList.

I have put together the following test to prove things out:

private static List<String> buildData() {
    return new ArrayList<>(Arrays.asList("Hello", "World"));
}

public static void testThings(List<String> data, List<String> addall, List<String> add) {
    System.out.printf("Using %s addAll %s and add %s%n", data, addall, add);

    try {
        for (String s : data) {
            if (addall != null) {
                data.addAll(addall);
            }
            if (add != null) {
                for (String a : add) {
                    data.add(a);
                }
            }
        }
        System.out.println("OK: " + data);
    } catch (Exception e) {
        System.out.println("Fail: " + e.getClass() + " -> " + e.getMessage());
    }
}

public static void main(String[] args) {

    String[] hw = {"Hello", "World"};

    testThings(buildData(), Arrays.asList(hw), null);
    testThings(buildData(), null, Arrays.asList(hw));
    testThings(new ArrayList<>(), Arrays.asList(hw), null);
    testThings(new ArrayList<>(), null, Arrays.asList(hw));
    testThings(buildData(), new ArrayList<>(), null);
    testThings(buildData(), null, new ArrayList<>());
    testThings(new ArrayList<>(), new ArrayList<>(), null);
    testThings(new ArrayList<>(), null, new ArrayList<>());
}

This produces the results:

Using [Hello, World] addAll [Hello, World] and add null
Fail: class java.util.ConcurrentModificationException -> null
Using [Hello, World] addAll null and add [Hello, World]
Fail: class java.util.ConcurrentModificationException -> null
Using [] addAll [Hello, World] and add null
OK: []
Using [] addAll null and add [Hello, World]
OK: []
Using [Hello, World] addAll [] and add null
Fail: class java.util.ConcurrentModificationException -> null
Using [Hello, World] addAll null and add []
OK: [Hello, World]
Using [] addAll [] and add null
OK: []
Using [] addAll null and add []
OK: []

Note the two lines:

Using [Hello, World] addAll [] and add null
Fail: class java.util.ConcurrentModificationException -> null
Using [Hello, World] addAll null and add []
OK: [Hello, World]

Adding an empty addAll causes a CME, yet that does not structurally modify the list. This is a bug in ArrayList.

Answer 3

I agree with rolfl -> in both cases CME should be thrown...

Most probable answer:

addAll() is a first invocation, it throws Exception, and second piece of code is simply never reached -> you have forgotten to comment first part out and therefore think that add()-part is exception-free code ;)