Already synchronized, but got ConcurrentModificationException

Question

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?

The solution and logic for showing list entry after each change is not very cool, so why don't you show the content just after the insertion in `insert()` method?! and also it may have a bug when you have one (or more) object(s) in the list, so the `run()` is going to lock and showing the entry forever. — , Oct 13 '13 at 14:20
Seems like you are try to solve 'producer-consumer problem'. If so you may want to look at BlockingQueue in java which helps you do that neatly. — KumarM, Oct 13 '13 at 14:27
See http://stackoverflow.com/questions/1655362/concurrentmodificationexception-despite-using-synchronized — Raedwald, Jan 22 '16 at 16:00

score 4 · Accepted Answer · answered Oct 13 '13 at 14:14

4

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);
}

answered Oct 13 '13 at 14:14

T.J. Crowder

1,031,962
187
1,923
1,875

That's it. Very helpful. I've got a lot to learn. Thanks :) – qweruiop Oct 13 '13 at 15:11
@Crowder, and could you explain the difference between `for(:)` and `Iterator`? – qweruiop Oct 13 '13 at 15:29
@ColinZ: The [enhanced `for` statement](http://docs.oracle.com/javase/specs/jls/se7/html/jls-14.html#jls-14.14.2) is just a shorthand way of using an iterator behind the scenes (see that link for details). But with an enhanced `for` statement, you can't use the iterator directly, so you can't access its `remove` method, which you need if you want to remove things during the iteration. – T.J. Crowder Oct 13 '13 at 15:42
@Crowder Your saying **you can't use the iterator directly** means that `PacketQueue.remove(msg)` actually calls the `remove()` method of `iterator` but not the `List.remove()`? – qweruiop Oct 14 '13 at 01:36
@ColinZ: No, `PacketQueue.remove()` is `List.remove`. I'm saying if you use the enhanced `for` statement, you don't have access to the underlying `Iterator` it uses, and so you can't remove things using that iterator (which is the correct way to remove them). So if you want to remove something in the loop, you can't use the enhanced `for` statement; you have to use an `Iterator` explicitly, as shown above, so you have access to the `Iterator`'s `remove`. – T.J. Crowder Oct 14 '13 at 07:13

Sergey Kalinichenko · Answer 2 · 2013-10-13T15:37:07.620

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.

score 0 · Answer 3 · answered Oct 13 '13 at 14:24

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.

Already synchronized, but got ConcurrentModificationException

3 Answers3