Is this solution to ConcurrentModificationException safe?

Question

I have a 2nd Thread that i use to send messages using OSC. From the main Thread i add messages, I had a problem with ConcurrentModificationException.

What I did to fix it is I made a new List with messages to add. In the 2nd Thread i add those messages to the list i want to send.

At the moment it runs without any problems, but i'm wondering is that luck? In other words, could it be that the change to run into a ConcurrentModificationException is still there but is really small now or did i really fix the problem?

 public void run() {


    while (running) {

        toSend.addAll(newMessages);
        newMessages.clear();

        Iterator<OSCPriorityMessage> itr = toSend.iterator();

        while (itr.hasNext()) {
            OSCPriorityMessage msg = itr.next();
            oscP5.send(msg, netAddress);
            itr.remove();
        }


        try {
            sleep((long)(waitTime));
        }
        catch (Exception e) {

        }

    }

}

Here i add the messages to send:

public void send(OSCPriorityMessage msg) {


    newMessages.add(msg);
}

You're not going to get the exception any more because the only thread updating the list over which you are iterating is the same thread that is iterating the list. As long as you're synchronizing access to the "new messages" list, this pattern is fine. — dlev, Jun 24 '14 at 01:27
That said, you might consider using a `BlockingQueue`, which is the preferred was to implement the producer/consumer pattern (which is what you're trying to do). — dlev, Jun 24 '14 at 01:32

Thilo · Accepted Answer · 2014-06-24T01:36:45.083

You still need to synchronize on newMessages whereever you access it. The two places we see here are 1) adding to it and 2) copying it into toSend and then clearing it. Maybe there are more.

public void send(OSCPriorityMessage msg) {

    synchronized(newMessages){
       newMessages.add(msg);
    }
}

To make it clear that toSend is a temporary, local list just for use in the sending process, declare it as a local variable.

Instead of

toSend.addAll(newMessages);
newMessages.clear();

you can do

ArrayList<OSCPriorityMessage> toSend;
synchronized(newMessages){
   toSend = new ArrayList<>(newMessages);
   newMessages.clear();
}

Without this synchronization you might miss some messages (or get them twice, or maybe something completely weird) as they are being added concurrently.

Now that you have a fresh toSend in every loop iteration, you don't actually need to remove the elements anymore and can do away with the whole iterator, replacing it with a simple loop:

for (OSCPriorityMessage msg: toSend){
    oscP5.send(msg, netAddress);
}

And finally, as @dlev suggests, take a look at the existing thread-safe queue implementations in the java.util.concurrent package.

I could also use `toSend.addAll(newMessages)` in a synchronized block right? — clankill3r, Jun 24 '14 at 01:42
yes. Just keep it in the synchronized block. I like using the constructor to do that, but `addAll` is probably easier to read. — Thilo, Jun 24 '14 at 01:43

Is this solution to ConcurrentModificationException safe?

1 Answers1