Already synchronized, but got ConcurrentModificationException [duplicate]

Question

This question already has answers here:

How to debug ConcurrentModificationException? [duplicate] (6 answers)

Closed 8 years ago.

Here is my class with two methods modifying the List PacketQueue. These two methods are execute in two thread, so synchronize is tagged.

public class MessageHandler implements nuctrl.interfaces.MessageHandler, Runnable {
    private static final List<GatewayMsg> PacketQueue = new LinkedList<GatewayMsg>();

    @Override
    public void insert(GatewayMsg msg) {
        synchronized (PacketQueue){
            PacketQueue.add(msg);
            PacketQueue.notify();
        }
        log.debug("insert " + msg.toString());
    }

    @Override
    public void run() {
        while(running){
            synchronized (PacketQueue){
                try {
                    while(PacketQueue.size() == 0){
                        PacketQueue.wait();
                    }
                } catch (InterruptedException e) {
                    e.printStackTrace();
                    break;
                }
                for (GatewayMsg msg : PacketQueue){
                    PacketQueue.remove(msg);
                    packetHandler.onPacket(msg);//method call
                }
            }
        }
    }
}

run() is for thread-4 and insert() is for another thread(I/O Worker #1). Synchronized has been added, and everything seems ok, but I still kept getting ConcurrentModificationException.

DEBUG [New I/O worker #1] (MessageHandler.java:47)| insert GatewayMsg<>
Exception in thread "Thread-4" java.util.ConcurrentModificationException
    at java.util.LinkedList$ListItr.checkForComodification(LinkedList.java:761)
    at java.util.LinkedList$ListItr.next(LinkedList.java:696)
    at nuctrl.core.MessageHandler.run(MessageHandler.java:67)
    at java.lang.Thread.run(Thread.java:680)

It drives me crazy now! Can anyone help find the fault? Or other ways to do the same thing?

Answer 1

Synchronizing doesn't do anything to prevent the ConcurrentModificationException if the synchronized code modifies the collection during iteration, which you do here:

for (GatewayMsg msg : PacketQueue){
    PacketQueue.remove(msg);     // <== Not allowed during iteration
    packetHandler.onPacket(msg);
}

During an iteration, you may only remove elements via an Iterator, e.g.:

Iterator<GatewayMsg> it = PacketQueue.iterator();
while (it.hasNext()) {
    GatewayMsg msg = it.next();
    it.remove();                 // <== This is allowed, provided the collection supports it
    packetHandler.onPacket(msg);
}

Answer 2

This has nothing to do with synchronizing - code like that would trigger the exception even in a single thread:

for (GatewayMsg msg : PacketQueue){
    PacketQueue.remove(msg);
    packetHandler.onPacket(msg);
}

This is because you are modifying the collection that you are iterating.

To fix this problem, use a list iterator in a loop, and call iterator's remove. Better yet, process all items in a loop, and then clear PacketQueue all at once, like this:

for (GatewayMsg msg : PacketQueue){
    packetHandler.onPacket(msg);
}
PacketQueue.clear();

This will work fine, because the access to PacketQueue is synchronized: other threads will not see PacketQueue in a state where part of its messages are processed, but they still remain in the queue.

Answer 3

The reason you get a CME is because you are modifying while iterating over it. The library can't tell the difference between your thread and another thread modifying it.

---

The simplest solution is to not write this queue/thread handling code yourself. I would write it using a ExecutorService

public class MessageHandler implements nuctrl.interfaces.MessageHandler {
    private static final ExecutorService EXEC = Executors.newSingleThreadExecutor();

    @Override
    public void insert(final GatewayMsg msg) {
        EXEC.submit(new Runnable() {
            @Override
            public void run() {
                packetHandler.onPacket(msg);//method call
            }
        });
        if(log.isDebugEnabled())
            log.debug("submitted " + msg);
    }

    public static void stop() {
        EXEC.shutdown();
    }
}

Note: this does not need to be wrapped in a Thread.

I check if logging is enabled as generating Strings you don't need can be amazingly slower and often the simplest way to speed up an application is to avoid them.