CODEWITHSUNDEEP
javaconcurrentmodification
anonmous
anonmous

Reputation: 137

ConcurrentModificationException, but no modification

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!

Upvotes: 0

Views: 386

Answers (2)

Mike &#39;Pomax&#39; Kamermans
Mike &#39;Pomax&#39; Kamermans

Reputation: 53685

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

Upvotes: 0

Bohemian
Bohemian

Reputation: 425318

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.

Upvotes: 2

Related Questions