synchronized block - lock more than one object

Question

I'm modelling a game where multiple players (threads) move at the same time. The information of where a player is located at the moment is stored twice: the player has a variable "hostField" that references to a field on the board and every field has an ArrayList storing the players that are currently located at this field.

I'm not very happy with the fact that I have redundant information, but I found no way avoiding that without looping through a big dataset.

However, when a player moves from one field to another, I'd like to make sure (1) the redundant information stays linked (2) nobody else is manipulating the field at the moment.

Therefore I need to do something like

synchronized(player, field) {
    // code
}

Which is not possible, right?

What should I do? :)

Answer 1

A trivial solution would be

synchronized(player) {
    synchronized(field) {
        // code
    }
}

However, make sure that you always lock the resources in the same order to avoid deadlocks.

Note that in practice, the bottleneck is the field, so a single lock on the field (or on a dedicated, common lock object, as @ripper234 rightly pointed out) may suffice (unless you are concurrently manipulating players in other, conflicting ways).

Answer 2

In fact, synchronization is for code, not objects or data. The object reference used as a parameter in synchronized block represent the lock.

So if you have code like:

class Player {

  // Same instance shared for all players... Don't show how we get it now.
  // Use one dimensional board to simplify, doesn't matter here.
  private List<Player>[] fields = Board.getBoard(); 

  // Current position
  private int x; 

  public synchronized int getX() {
    return x;
  }

  public void setX(int x) {
    synchronized(this) { // Same as synchronized method
      fields[x].remove(this);
      this.x = x;
      field[y].add(this);
    }
  }
}

Then Despite being in the synchronized block the access to field is not protected because the lock is not the same (it being on different instances). So your List of Players for your board can become inconsistent and cause runtime exceptions.

Instead if you write the following code, it will work because we have only one shared lock for all players:

class Player {

  // Same instance shared for all players... Don't show how we get it now.
  // Use one dimensional board to simplify, doesn't matter here.
  private List<Player>[] fields; 

  // Current position
  private int x;

  private static Object sharedLock = new Object(); // Any object's instance can be used as a lock.

  public int getX() {
    synchronized(sharedLock) {
      return x;
    }
  }

  public void setX(int x) {
    synchronized(sharedLock) {
      // Because of using a single shared lock,
      // several players can't access fields at the same time
      // and so can't create inconsistencies on fields.
      fields[x].remove(this); 
      this.x = x;
      field[y].add(this);
    }
  }
}

Be sure to use only a single lock to access all the players or your board's state will be inconsistent.

Answer 3

When using concurrency, it is always difficult to give good responses. It highly depends of what your are really doing and what really matter.

From my understanding, a player move involve :

1 Updading player position.

2 Removing the player from previous field.

3 Adding player to new field.

Imagine you use several locks at the same time but aquire only one at a time : - Another player can perfectly look at the wrong moment, basically between 1&2 or 2&3. Some player can appear to have disapeared from the board for exemple.

Imagine your do imbricked locking like this :

synchronized(player) {
  synchronized(previousField) {
    synchronized(nextField) {
      ...
    }
  }
}

The problem is... It don't work, see this order of execution for 2 threads :

Thread1 :
Lock player1
Lock previousField
Thread2 :
Lock nextField and see that player1 is not in nextField.
Try to lock previousField and so way for Thread1 to release it.
Thread1 :
Lock nextField
Remove player1 from previous field and add it to next field.
Release all locks
Thread 2 : 
Aquire Lock on previous field and read it : 

Thread 2 think that player1 as disapeared from the whole board. If this is a problem for your application, you can't use this solution.

Additionnal problem for imbriqued locking : threads can become stuck. Imagine 2 players : they exchange their position at exactly the same time :

player1 aquire it's own position at the same time
player2 aquire it's own position at the same time
player1 try to acquire player2 position : wait for lock on player2 position.
player2 try to acquire player1 position : wait for lock on player1 position.

=> Both players are blocked.

Best solution in my opinion is to use only one lock, for the whole state of the game.

When a player want to read the state, it lock the whole game state (players & board), and make a copy for its own usage. It can then process without any lock.

When a player want to write the state, it lock the whole game state, write the new state and then release the lock.

=> Lock is limited to read/write operations of the game state. Player can perform "long" examination of the board state on their own copy.

This prevent any inconsistant state, like a player in several field or none, but don't prevent that player can use "old" state.

It can appear weird, but it is the typical case of a chess game. When you are waiting for the other player to move, you see the board as before the move. You don't know what move the other player will make and until he has finally moved, you work on an "old" state.

Answer 4

You should not feel bad about your modelling - this is only a two way navigable association.

If you take care (as in the other answers told) to manipulate atomic, e.g. in the Field methods, thats fine.

public class Field {

  private Object lock = new Object();

  public removePlayer(Player p) {
    synchronized ( lock) {
      players.remove(p);
      p.setField(null);
    }
  }

  public addPlayer(Player p) {
    synchronized ( lock) {
      players.add(p);
      p.setField(this);
    }
  }
}

It would be fine if "Player.setField" were protected.

If you need further atomicity for "move" semantics, go one level up for board.

Answer 5

Reading all your answers, I tried to apply the following design:

1. Only lock players, not fields
2. Do field operations only in synchronized methods/blocks
3. in a synchronized method/block always first check if the preconditions that caused the synchronized method/block to be called are still the case

I think 1. avoids deadlocks and 3. is important as things could change while a player waits.

Further I can go without locking fields as in my game more than one player can stay at a field, only for certain threads an interaction has to be done. This interaction can be done by synchronising players - no need to sync fields...

What do you think?