Data Structures in 2D-Games with Multiplayer (Java)

Question

I have following problem:

I've programmed a 2D Game with a multiplayer function. Right now i store others Player Data and GameObjects in two ArrayList (The world is stored otherwise). Sometimes the Network-Thread sends Updates, which can not be applied, because the Game draws the Players/Game Objects (java.util.ConcurrentModificationException). Because this drawing process happens every Second around 60 times (because of animations) the problem apeers often (every 2 seconds). This is the code for the players ArrayList:

Draw the Players:

for (Player p : oPlayer) {
  if (p != null) {
    int x = (int) ((width / 2) + (p.x - getPlayerX()) * BLOCK_SIZE);
    int y = (int) ((height / 2) + (p.y - getPlayerY()) * BLOCK_SIZE);
    g.drawImage(onlinePlayer, x, y, BLOCK_SIZE, BLOCK_SIZE, null);
    FontMetrics fm = g.getFontMetrics();
    g.setColor(Color.DARK_GRAY);
    g.drawString(p.getName(), x + (BLOCK_SIZE / 2) - (fm.stringWidth(p.getName()) / 2), y - 5);
  }
}

Edit Information in Network-Thread:

case "ADP": //add Player
  Game.oPlayer.add(new Player(message, id));
  sendX();
  sendY();
  break;
case "SPX": // set X
  for (Player p : Game.oPlayer) {
    if (p.getId() == id) {
      p.setX(Short.parseShort(message));
      break;
    }
  }
  break;
case "SPY": // set Y
  for (Player p : Game.oPlayer) {
    if (p.getId() == id) {
      p.setY(Short.parseShort(message));
      break;
    }
  }
  break;
case "PDI": // remove Player
  for (Player p : Game.oPlayer) {
    if (p.getId() == id) {
      Game.oPlayer.remove(p);
      break;
    }
  }
  break;

Thank you in advance :)

Answer 1

What happens here is, that 2 Threads are working on the same list.
The first one is reading the List (for (Player p : oPlayer) {) and the second one is modifying it (Game.oPlayer.add(new Player(message, id));). This brings the oPlayer list into an (sort of) "inconsistent" state. Java sees that you modified something that you are reading and throws this exception to let you know, that something is not kosher.
More information about ConcurrentModificationExceptions can be found here

To clarify, you dipped into the so called Readers-writer problem. You have an reader (Thread), that reads the Data of Game.oPlayer and a writer (Thread) that writes data to Game.oPlayer.

Solutions

---

Synchronized-Keyword

The synchronized keyword is explained here. You would use it like this:

private final List<Player> players = ...;

public void addPlayer(Player player) {
    synchronized(players) {
        players.add(player);
    }
}

public void removePlayer(Player player) {
    synchronized(players) {
        players.remove(player);
    }
}

Note that the List has to bee final. Furhter i am using a local attribute instead of your static one. Remove players with Game.oPlayer to get a suited solution.
This allows only 1 thread to access players.add() and players.remove().

---

Lock

Informations about how to use Locks can be found here.

Easy said, you create a block like this:

try {
lock.lock();
// work ..
} finally {
lock.unlock();
}

so that only one thread can access the work part by saying lock.lock(). If any other thread locked the work part using lock.lock() and not unlocked it, the current thread will wait until lock.unlock() is called. The try-finall block is used, to assure, that the lock is unlocked, even if your work part is throwing an throwable.

---

Furhter i would recommend itterating over a "copy" of the player-list like this:

List<Player> toIterate;
synchronized(players) {
    toIterate = new ArrayList<>(getPlayerList());
}
for(Player player : toIterate) {
    // work
}

Or synchronizing this part completly like this:

synchronized(players) {
    for(Player player : players) {
        // work
    }
}

The first one provides you with an copy of that instance, which basically means, it contains the same Objects as the original List, but it isn't the same List. It helps you by letting more threads work on there own "list" and finish theire jobs, regardless of updates at the current time, becaus the second example will block if:

1. Any thread wants to read the list.
2. Any thread modifies the list.

So you only have to synchronize the coppy part in the first example.

---

Even furhter (not particually part of your question, but still something that would make it easyer) i would recommend not using static, as you stated in Game.oPlayer.[...] and taking a look at Dependency Injection.

You could modify your Game-class to provide the methods addPlayer(Player player);, removePlayer(Player player); and getPlayerList(); to realy code in an Object Oriented fashion.
With that design, you could easyly modify your code, to handle the new concurrency issue.