Why does this code throw a java ConcurrentModificationException?

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 reche开发者_如何学运维cked 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

---

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 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).

---

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.