java.util.ConcurrentModificationException upon LinkedHashSet iteration

Question

Please, help me understand the error I am getting:

private void replayHistory() {
    synchronized (alarmsHistory) {
        for (AlarmEvent alarmEvent : alarmsHistory) {
            LOG.error("replayHistory " + alarmEvent.type + " " + alarmEvent.source);
            sendNotification(alarmEvent.type, alarmEvent.source, alarmEvent.description, 
                    alarmEvent.clearOnOtherStations, alarmEvent.forceClearOnOtherStations);         
        }
    }
}

and the method that adds an element to it

private void addToAlarmsHistory(AlarmEvent alarmEvent) {
    synchronized (alarmsHistory) {
        LOG.error("addToAlarmsHistory " + alarmEvent.type + " " + alarmEvent.source);
        alarmsHistory.add(alarmEvent);
    }
}

both methods and the Set

private volatile Set<AlarmEvent> alarmsHistory = new LinkedHashSet<AlarmEvent>();

are defined in

JmxGwReloadThread extends Thread class

which is an inner class in

AlarmManager class

that has a method

private void addToReplayHistory(AlarmEvent alarmEvent) {
    if ((jmxThread != null) && (jmxThread.isAlive())) {
        jmxThread.addToAlarmsHistory(alarmEvent);
    }
}

which is being called by different interfaces (cannot assure when and how often)

At some point JmxThread is started and calls replayHistory method

java.util.ConcurrentModificationException is thrown, the root is from the

for (AlarmEvent alarmEvent : alarmsHistory) {

The code propably tries to add an element to the alarmsHistory and when interator

java.util.ConcurrentModificationException
    at java.util.LinkedHashMap$LinkedHashIterator.nextEntry(LinkedHashMap.java:390)
    at java.util.LinkedHashMap$KeyIterator.next(LinkedHashMap.java:401)
    at AlarmManager$JmxGwReloadThread.replayHistory(AlarmManager.java:568)
    at AlarmManager$JmxGwReloadThread.run(AlarmManager.java:532)

throws exception upon calling nextEntry, but should't synchronization prevent from such an issue?

Logs show that synchronization does not work - replayHistory should iterate over all its elements (I can asure its more then one single HEARTBEAT_INFO FM) but it's interrupted by the addToReplayHistory call.

2013-07-11 11:58:33,951 Thread-280 ERROR AlarmManager$JmxGwReloadThread.replayHistory(AlarmManager.java:570)  - replayHistory HEARTBEAT_INFO FM
2013-07-11 11:58:33,951 Thread-280 ERROR AlarmManager$JmxGwReloadThread.addToAlarmsHistory(AlarmManager.java:550)  -  addToAlarmsHistory HEARTBEAT_INFO FM
2013-07-11 11:58:33,952 Thread-280 ERROR Log4jConfigurator$UncaughtExceptionHandler.uncaughtException(Log4jConfigurator.java:253)  - Detected uncaught exception in thread: Thread-280

Answer 1

One thing OP (and probably most people) should be aware of:

ConcurrentModificationException has nothing to do with multi-threading.

Although multi-threading makes it much easier to happen, but the core of this problem has nothing to do with multi-threading.

This is mostly caused by scenario like,

1. Get the iterator from a collection,
2. Before finish using that iterator, the collection is structurally modified.
3. Keep on using the iterator after 2. The iterator will detect the collection is structurally modified and will throw out ConcurrentModificationException.

Of course, not all collection have such behavior, e.g. ConcurrentHashMap. Definition of "Structurally Modified" is different for different collection too.

That means, even I have only 1 thread, if I do something like:

    List<String> strings = new ArrayList<String>();
    //....
    for (String s: strings) {  // iterating through the collection
        strings.add("x");   // structurally modifying the collection
    }

I will get ConcurrentModificationException even it is all happening in single thread.

There are different ways to solve the problem, depending on your requirement or problem. e.g.

1. If it is simply due to multi-thread access, proper synchronize access can be one solution
2. You may make use of Collection that iterator is safe from structural modification (e.g. ConcurrentHashMap)
3. Adjust your logic to either re-acquire the iterator again if you modified the collection, or makes your modification using the iterator (some collection impls allows that), or make sure modification to collection happens after you finish using the iterator.

Given that your code seems having proper synchronization on alarmHistory, there are two directions you would want to check

1. Is there any possible modification of alarmHistory inside sendNotification()? For example, adding or removing from alarmHistory?
2. Is there other possible unsynchronized access to alarmHistory which may modify the structure?