Java Concurrent Exception when using copy of lists

Question

I have the following code in my class:

private static LinkedList<MyObject> myList = 
                new LinkedList<MyObject>();

public static void doEventStuff(String user, String event){
        LinkedList<MyObject> copy;
        synchronized (myList) {
            copy = new LinkedList<>(myList);
        }
        for (MyObject o : copy) {
             ... do something with objects o
        }

}

public static void removeObject(MyObject o) {
        synchronized (myList) {
            myList.remove(o);
        }
        o.doCleanup();
    }

public static void terminate() {
        synchronized (myList) {
            for (MyObject o : myList) {
                o.doCleanup();
            }

            myList.clear();
        }

    }

public static List<MyObject> getMyObjectsCopy() {
        synchronized (myList) {
            return new LinkedList<>(myList);
        }
    }

My problem is a ConcurrentModificationException when calling terminate() , specifically when iterating "for (MyObject o : myList) ".

The list myList is not passed around and can only be accessed through the static methods. Also: the method MyObject.doCleanup() ca trigger events where the method "removeObject(MyObject)" can be called, when doing the iteration inside terminate() mthod , but since all the methods synchronize on "myList", I didn't believe a concurrency exception can happen.

Can anyone help me with this issue?

If it is possible for an object to be removed from the list, use an iterator instead of the for loop. Removing an object during a for loop like that can cause that exception. — BlakeP, Nov 20 '15 at 19:50
There's some modification to `myList` that you haven't shown us. — user2357112, Nov 20 '15 at 19:52

geert3 · Answer 1 · 2015-11-20T22:27:13.840

2

ConcurrentModificationException also happens if the list was modified while iterating over it using a 'foreach' loop. synchronize will help avoid other threads from accessing your list, but your issue is not due to thread-concurrency. If you want to delete (from the same thread) while iterating over the list, you must use an iterator and call iterator.remove().

edited Nov 20 '15 at 22:27

answered Nov 20 '15 at 19:51

geert3

7,086
1
33
49

How will iteration.remove helps with removing from non thread safe collection in multithreaded environment? ConcurrentModificationException itself is mostly an issue with iterators. – Dmitry Spikhalskiy Nov 20 '15 at 20:11
Related SO question: http://stackoverflow.com/questions/223918/iterating-through-a-list-avoiding-concurrentmodificationexception-when-removing – Roddy of the Frozen Peas Nov 20 '15 at 20:27
@DmitrySpikhalskiy updated my answer. Synchronize helps with CME but not in this case. – geert3 Nov 20 '15 at 22:28
I believe this is the correct answer... The events that triggered in the doCleanup() method did not arrive (and did not call methods from my external callbacks) on a separate thread, do synchronizing in the removeObject(...) method didn't actually do anything... – Andrei F Nov 20 '15 at 22:32

score 2 · Answer 2 · answered Nov 20 '15 at 19:52

2

This is not multi-threading issue per se, if you remove an object from the list in a foreach loop you will get ConcurrentModificationException. And by the way, you can use CopyOnWriteArrayList instead

answered Nov 20 '15 at 19:52

Sleiman Jneidi

22,907
14
56
77

Dmitry Spikhalskiy · Answer 3 · 2015-11-20T22:30:43.317

In this code:

for (MyObject o : myList) {
    o.doCleanup(o);
}

You call code, which internally calls removeObject() method. In this call we make myList.remove(o), which will change a list, as a result, it works like:

for (MyObject o : myList) {
    myList.remove();
}

So, it's not a concurrency issue, it's just modification of collection in forEach loop over this collection. I think the best solution for this situation is to avoid removing from myList in doCleanup() code, it looks like lack of design. Other possible solution - another doCleanup() method version which doesn't throw an event which cause removal from collection - you already do myList.clear(). Or rewrite removeObject() method like:

public static void removeObject(MyObject o) {
    synchronized (myList) {
        for (Iterator<MyObject> it = myList.iterator(); it.hasNext(); ) {
            MyObject o1 = it.next();
            if (o1.equals(o)) {
               it.remove();
            }
        }            
    }
    o.doCleanup();
}

like @geert3 recommends in his answer as far as I understand, but motivation in this answer is not fully clear for me.

But I don't like last solution - it looks like a hack for design problem because in this global collection maintaining code we call doCleanup() on deleted object which should call one more removeObject() inside event handler - I think it will be better to remove this "recursion".

Java Concurrent Exception when using copy of lists

3 Answers3