Why does this code throw a java ConcurrentModificationException?

Question

public final class ClientGateway {

   private static ClientGateway instance;
   private static List<NetworkClientListener> listeners = Collections.synchronizedList(new ArrayList<NetworkClientListener>());
   private static final Object listenersMutex = new Object();
   protected EventHandler eventHandler;


   private ClientGateway() {
      eventHandler = new EventHandler();
   }

   public static synchronized ClientGateway getInstance() {
      if (instance == null)
         instance = new ClientGateway();
      return instance;
   }

   public void addNetworkListener(NetworkClientListener listener) {
     synchronized (listenersMutex) {
        listeners.add(listener);
     }
   }


   class EventHandler {

     public void onLogin(final boolean isAdviceGiver) {
        new Thread() {
           public void run() {
              synchronized (listenersMutex) {
                 for (NetworkClientListener nl : listeners) 
                    nl.onLogin(isAdviceGiver);
              }
           }
        }.start();
     }

   }
}

This code throws a ConcurrentModificationException But I thought if they are both synchronized on the listenersMutex then they should be executed in serial? All code within functions that operate on the listeners list operate within syncrhonized blocks that are synchronized on the Mutex. The only code that modifies the list are addNetworkListener(...) and removeNetworkListener(...) but removeNetworkListener is never called at the moment.

What appears to be happening with the error is that a NetworkClientListener is still being added while the onLogin function/thread is iterating the listeners.

Thank you for your insight!

EDIT: NetworkClientListener is an interface and leaves the implementation of "onLogin" up to the coder implementing the function, but their implementation of the function does not have access to the listeners List.

Also, I just completely rechecked and there is no modification of the list outside of the addNetworkListener() and removeNetworkListener() functions, the other functions only iterate the list. Changing the code from:

for (NetworkClientListener nl : listeners) 
   nl.onLogin(isAdviceGiver);

To:

for(int i = 0; i < listeners.size(); i++)
   nl.onLogin(isAdviceGiver);

Appears to solve the concurrency issue, but I already knew this and would like to know what's causing it in the first place.

Thanks again for your continuing help!

Exception: Exception in thread "Thread-5" java.util.ConcurrentModificationException at java.util.ArrayList$Itr.checkForComodification(ArrayList.java:782) at java.util.ArrayList$Itr.next(ArrayList.java:754) at chapchat.client.networkcommunication.ClientGateway$EventHandler$5.run(ClientGateway.java:283)

EDIT Okay, I feel a little dumb. But thank you for all your help! Particularly MJB & jprete!

Answer: Someone's implementation of onLogin() added a new listener to the gateway. Therefore(since java's synchronization is based on Threads and is reentrant, so that a Thread may not lock on itself) when onLogin() was called we in his implementation, we were iterating through the listeners and in the middle of doing so, adding a new listener.

Solution: MJB's suggestion to use CopyOnWriteArrayList instead of synchronized lists

Just for the heck of it, if you replace nl.onLogin with a system.out.println(nl); does the issue still occur? — MJB, May 05 '11 at 01:26
@MJB well this is bizarre, I replaced it and it works. I'm going to go through every implementation of onLogin and see if something is doing something. But what confuses me is every implementation resides in a different package entirely, how can they modify the "private List listeners"? — random dude, May 05 '11 at 01:58
Possible duplicate of [ConcurrentModificationException despite using synchronized](http://stackoverflow.com/questions/1655362/concurrentmodificationexception-despite-using-synchronized) — Raedwald, Mar 28 '16 at 15:01

jprete · Accepted Answer · 2011-05-04T22:11:44.600

Mutexes only guard from access from multiple threads. If nl.onLogin() happens to have logic that adds a listener to the listeners list, then a ConcurrentModificationException may be thrown, because it's being accessed (by the iterator) and changed (by the add) simultaneously.

EDIT: Some more information would probably help. As I recall, Java collections check for concurrent modifications by keeping a modification count for each collection. Every time you do an operation that changes the collection, the count gets incremented. In order to check the integrity of operations, the count is checked at the beginning and end of the operation; if the count changed, then the collection throws a ConcurrentModificationException at the point of access, not at the point of modification. For iterators, it checks the counter after every call to next(), so on the next iteration of the loop through listeners, you should see the exception.

I edited the main post to note that nl.onLogin is not implemented by me, however it's implementation does not have access to the listeners List. Thanks for the info though! — random dude, May 04 '11 at 22:59

score 3 · Answer 2 · answered May 04 '11 at 21:54

3

I must admit that I don't see it either - if indeed removeListeners is not called.

What is the logic of the nl.onLogin bit? If it modified stuff, it could cause the exception.

A tip btw if you expect listeners to be moderately rare in being added, you could make the list CopyOnWriteArrayList type -- in which case you don't need your mutexes at all - CopyOnWriteArrayList is totally thread safe, and returns a weakly consistent iterator that will never throw CME (except where I just said, in nl.onLogin).

answered May 04 '11 at 21:54

MJB

9,352
6
34
49

Thanks for the tip on CopyOnWriteArrayList! I'll probably end up using this since the listeners are seldomly added/removed, however I'm still trying to understand why I'm getting the concurrent exception in the first place. (I edited the main post to note that nl.onLogin is not implemented by me, however it's implementation does not have access to the listeners List) – random dude May 04 '11 at 22:57

score 3 · Answer 3 · answered May 05 '11 at 02:53

Instead of ArrayList , use can use thread-safe class CopyOnWriteArrayList which does not throw ConcurrentModificationException even if it is modified while iterating. While iterating if it is attempted to modify(add,update) then it makes a copy of the list, but iterater will continue working on original one.

Its a bit slower than ArrayList . It is useful in cases where you do not want to syncronise the iterations.

Why does this code throw a java ConcurrentModificationException?

3 Answers3