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

Answer 1

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.