ConcurrentModificationException, but no modification

Question

I am currently programming a multiplayer game in Java. My current code (that is getting the error) is as so.

@Override
public void onClose(WebSocket conn, int code, String reason, boolean remote){
    System.out.println("Socket disconnected.");

    for(Game g : Lobby.games){
        if(g.hasPlayer(new Player(conn))){
            Player ourPlayer = null;

            for(Player p : g.getPlayers()){
                if(p.getSocket() == conn){
                    ourPlayer = p;
                    break;
                }
            }

            if(ourPlayer == null) return;

            g.removePlayer(ourPlayer);

            for(Player p : g.getPlayers()){
                send(p.getSocket(), Messages.SEND_REMOVE_PLAYER + ourPlayer.getName());
            }

            if(g.getPlayers().size() == 0){
                Lobby.removeGame(g);
            }
        }
    }
}

Now please do not ask about functions like onClose. That is not causing the issue.

I am getting ConcurrentModificationException for the following line:

for(Game g : Lobby.games){

Lobby.games is an empty ArrayList of "Game" inside Lobby.java. Games are added on through other functions.

public static ArrayList<Game> games = new ArrayList<Game>();

UPDATE: And this is removeGame:

public static void removeGame(Game game){
    Iterator<Game> itr = games.iterator();

    while(itr.hasNext()){
        Game g = itr.next();

        if(g.getId() == game.getId()){
            System.out.println("Game "+g.getId()+" removed");
            itr.remove();
        }
    }
}

Sorry for being vague. If you need any more code, I will surely add it. Thanks!

1st, I have already tried using an iterator. Secondly, I am not modifying anything without functions, which DO use iterators. — anonmous, Oct 01 '14 at 02:50
`Lobby.removeGame(g);` suggests that you are tampering with `Lobby.games` in the loop. — njzk2, Oct 01 '14 at 02:56
@anonmous: It does not matter if you use an iterator in `Lobby.removeGame(g);`. The important part is that the fast enum declared by `for(Game g : Lobby.games){` will not stand any modification to the array, by any method whatsoever. — njzk2, Oct 01 '14 at 02:58

score 2 · Answer 1 · answered Oct 01 '14 at 02:53

Your problem is (almost certainly) here:

for(Game g : Lobby.games) {
    // other code
    if (g.getPlayers().size() == 0){
        Lobby.removeGame(g);
    }
}

if Lobby.removeGame(g) changes the contents of Lobby.games, then you will be modifying Lobby.games while iterating over it (the foreach loop implicitly iterates over Lobby.games).

Either use an iterator and call Iterator.remove(), or save up a collection of games to be removed after the loop, or otherwise reorganise your code to avoid this situation.

score 0 · Accepted Answer · answered Oct 01 '14 at 02:53

Your Lobby.removeGame(g) call means you intend to modify the list despite the fact that you're iterating over it right now, which would completely invalidate the implicit ordering.

If you need do this, use an explicit reverse loop so that you're only modifying parts of the list that aren't going to affect the ordering of parts you haven't gotten to yet:

for(int i=Lobby.games.size()-1; i>-1; i--) {
  Game g = Lobby.games.get(i);

  // this is now safe, because you're only ever going to see
  // indices lower than the current "i", and the removal
  // only how long Lobby.games is *after* "i", not before it.
  Lobby.games.remove(g)
}

ConcurrentModificationException, but no modification

2 Answers2