java.util.ConcurrentModificationException on ArrayList

Question

I have a Server class and a Timer inside it which is supposed to clear dead clients (clients who crashed). I followed the example below by locking the collection when the Timer iterates over the users but I still get this exception (after I crash a connected client).

http://www.javaperformancetuning.com/articles/fastfail2.shtml

List<User> users;
List<User> connectedUsers;
ConcurrentMap<User, IClient> clients;

...

users = Collections.synchronizedList(new ArrayList<User>());
connectedUsers = new ArrayList<User>();
clients = new ConcurrentHashMap<User, IClient>();
timer = new Timer();
timer.schedule(new ClearDeadClients(), 5000, 5000);

...

class ClearDeadClients extends TimerTask {
    public void run() {
        synchronized (users) {
            Iterator<User> it = users.iterator();
            while (it.hasNext()) {
                User user = it.next(); // Throws exception
                if (!connectedUsers.contains(user)) {
                    users.remove(user);
                    clients.remove(user);
                }
            }
        }       

        connectedUsers.clear();
    }
}

Solution 1

You need to remove from the iterator not the collection. It would look like this instead:

Iterator<User> it = users.iterator();
while (it.hasNext()) {
    User user = it.next(); 
    if (!connectedUsers.contains(user)) {
         it.remove();
         clients.remove(user);
     }
}

Solution 2

You cannot modify a collection while iterating over it - unfortunately you do that here with users, and the ConcurrentModificationException is the result. From ArrayList's own javadocs:

The iterators returned by this class's iterator and listIterator methods are fail-fast: if the list is structurally modified at any time after the iterator is created, in any way except through the iterator's own remove or add methods, the iterator will throw a ConcurrentModificationException. Thus, in the face of concurrent modification, the iterator fails quickly and cleanly, rather than risking arbitrary, non-deterministic behavior at an undetermined time in the future.

To fix this particular situation, you could instead use the Iterator's own remove() method, by replacing this line:

users.remove(user);

with

it.remove();

This latter operation removes from the collection the last element that the iterator returned. (This usage avoids the exception because the iterator is aware of the change and is able to ensure it's safe; with external modifications the iterator has no way to know whether the state of its traversal is still consistent, and so fails fast).

In some situations, this immediate removal may not be workable, in which case there are three alternative general approaches:

1. Take a copy of the collection (users in this case), iterate over the copy and remove elements from the original.
2. During iteration, build up a set of elements to remove, and then perform a bulk removal after iteration has completed.
3. Use an List implementation which can deal with concurrent modifications, like the CopyOnWriteArrayList

---

This is quite a common question - see also (for example) the loop on list with remove question for other answers.

Comments

[Waldheinz]

To add to your list: 3. Use an List implementation which can deal with concurrent modifications like the CopyOnWriteArrayList.