Synchronize methods to prevent ConcurrentModificationException

Question

I have a Java 1.8 class that holds two collections:

Map<Key,Object>
Set<Object>

My class five methods:

addObjectToMap()
removeObjectFromMap()
addObjectToSet()
removeObjectFromSet()

loopOverEverything(){
    for(Object o : mySet){
        for(Object o2 : myMap.getKeySet()){
            doSomething(o,o2);
        }
    } 
}

The point of the class is to implement the observer pattern but be very flexible in both observers and observees. The problem I face is that the last method can easily throw a ConcurrentModificationException when a thread calls the add/remove methods while the looping is going on. I was thinking about synchronizing all methods on "this":

set.add(object);

would become

synchronized(this){
    set.add(object);
}

and I would add a similar synchronize statement to the other 4 methods, including the loop method.

Will this work? I know synchronizing methods can cause bottlenecks. Even though there is currently no performance issue, I would like to rationalize this design and hear about possible alternatives that have less of a performance hit.

Answer 1

Because locks are re-entrant, synchronizing access to the collection won't stop the same thread iterating over a collection, and then modifying the collection itself. Despite the name, ConcurrentModificationException is not always raised due to concurrent modification by another thread. Registering or un-registering other observers in a notification callback is a prime culprit for such concurrent modification.

A common strategy for firing events is to take a "snapshot" of the listeners to be notified before delivering any events. Because the order in which observers are notified is generally unspecified, it's valid for all observers that were registered when an event is generated to receive it, even if another observer de-registers it as a result of that notification.

To make a snapshot, you can copy the observers to a temporary collection or array:

Collection<?> observers = new ArrayList<>(mySet);

or

Object[] observers = mySet.toArray(new Object[mySet.size()]);

Then iterate over that copy, leaving the original available for update:

for (Object o : observers) {
  ...
    doSomething(o, ...);
  ...
}

Some concurrent collections like ConcurrentSkipListSet won't raise an exception, but they only guarantee "weakly consistent" iteration. This could result in some (newly added) observers unexpectedly receiving the current notification. CopyOnWriteArraySet uses a snapshotting technique internally. If you rarely modify the set, it will be more efficient, because it only copies the array when necessary.