Java Concurrent Exception when using copy of lists [duplicate]

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
– BlakeP, Commented Nov 20, 2015 at 19:50
There's some modification to myList that you haven't shown us. — user2357112
– user2357112, Commented Nov 20, 2015 at 19:52

Sleiman Jneidi · Accepted Answer · 2015-11-20 19:52:53Z

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, 2015 at 19:52

Sleiman Jneidi

23.4k14 gold badges60 silver badges80 bronze badges

Sign up to request clarification or add additional context in comments.

Comments

geert3 · Accepted Answer · 2015-11-20 22:27:13Z

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, 2015 at 22:27

answered Nov 20, 2015 at 19:51

geert3

7,3771 gold badge36 silver badges54 bronze badges

4 Comments

Dmitry Spikhalsky Over a year ago

How will iteration.remove helps with removing from non thread safe collection in multithreaded environment? ConcurrentModificationException itself is mostly an issue with iterators.

Roddy of the Frozen Peas Over a year ago

Related SO question: stackoverflow.com/questions/223918/…

geert3 Over a year ago

@DmitrySpikhalskiy updated my answer. Synchronize helps with CME but not in this case.

Andrei F Over a year ago

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...

Dmitry Spikhalsky · Accepted Answer · 2015-11-20 22:30:43Z

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".

Collectives™ on Stack Overflow

Java Concurrent Exception when using copy of lists [duplicate]

3 Answers 3

Comments

4 Comments

Comments

Linked

Hot Network Questions