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

Can you please make your code [SSCCE](http://sscce.org/)? Especially show us a complete class that shows this behaviour. Because your current posted code should work so far. — Uwe Plonus, Jul 11 '13 at 08:46
How can you iterate directly over a LinkedHashMap? alarmsHistory cannot be a LinkedHashMap as "for (AlarmEvent alarmEvent : alarmsHistory) { .. }" would not compile. Your example makes no sense to me. Please post the full example as the comment before proposes. — mmirwaldt, Jul 11 '13 at 08:58
Edited my initial post, sorry for incomplete and misleading data. — Łukasz, Jul 11 '13 at 09:27
The code uses a `LinkedHashSet`, but the stack trace mentions a `LinkedHashMap`? — Dahaka, Jul 11 '13 at 09:34
Also I think the first call to `LOG` should be in the loop, not before it. — Dahaka, Jul 11 '13 at 10:09
Yes, it's in the lopp, I had some problems with identation and copy-pasted it in the wrong line. — Łukasz, Jul 11 '13 at 10:15
@mmirwaldt I am not iterating over LinkedHashMap, LinkedHashSet extends HashSet. Hashset has public Iterator iterator() { return map.keySet().iterator(); } which retrieves iterator from map implementation this is why it's in the stack trace. — Łukasz, Jul 11 '13 at 10:31

Adrian Shum · Answer 1 · 2014-09-05T01:44:07.617

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,

Get the iterator from a collection,
Before finish using that iterator, the collection is structurally modified.
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.

If it is simply due to multi-thread access, proper synchronize access can be one solution
You may make use of Collection that iterator is safe from structural modification (e.g. ConcurrentHashMap)
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

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

Bohemian · Answer 2 · 2013-07-11T09:10:04.973

2

If one thread iterates, and another thread adds, you're hosed.

Given that your code seems to synchronise access to the two relevant blocks of code, look for other unsynchronized code that adds/removes from alarmsHistory.

edited Jul 11 '13 at 09:10

answered Jul 11 '13 at 09:02

Bohemian

412,405
93
575
722

If the lock is held by the thread which iterates, how is it possible for another thread to add?? – prasanth Jul 11 '13 at 09:03
But if the two Threads have their own instances, why is there a `ConcurrentModificationException`? – Dahaka Jul 11 '13 at 09:09
@Dahaka I just realised that the lock object is the collection. Further edits :/ – Bohemian Jul 11 '13 at 09:20

kan · Answer 3 · 2013-07-11T10:54:59.433

The only idea which comes to my head that you have an intricate logic behind the scene. I think the sendNotification somehow recursively invokes addToReplayHistory. So, the multithreading is a red herring, the log file shows only one thread involved, and immideately after sendNotification there is addToReplayHistory call which modifies the collection and breaks the interator.

More info is in the javadoc for the exception:

Note that this exception does not always indicate that an object has been concurrently modified by a different thread. If a single thread issues a sequence of method invocations that violates the contract of an object, the object may throw this exception. For example, if a thread modifies a collection directly while it is iterating over the collection with a fail-fast iterator, the iterator will throw this exception.

score 0 · Answer 4 · edited May 23 '17 at 10:25

To add some details to kan's answer:

The synchronized block in Java is reentrant:

Reentrant Synchronization
Recall that a thread cannot acquire a lock owned by another thread. But a thread can acquire a lock that it already owns. Allowing a thread to acquire the same lock more than once enables reentrant synchronization. This describes a situation where synchronized code, directly or indirectly, invokes a method that also contains synchronized code, and both sets of code use the same lock. Without reentrant synchronization, synchronized code would have to take many additional precautions to avoid having a thread cause itself to block.

So like kan pointed out, it might in fact be that not multiple threads are modifying your collection but only one thread which, due to the reentrant behavior, may always acquire the lock.

The things you should look for to fix the exception are recursive calls between synchronized blocks, or unsychronized access to alarmsHistory.

You can also look into concurrent collections like ConcurrentSkipList or CopyOnWriteArraySet. Both should prevent the exception, but beware of their behavior and performance characteristics described in the JavaDoc.

java.util.ConcurrentModificationException upon LinkedHashSet iteration

4 Answers4

Reentrant Synchronization