diff options
| -rw-r--r-- | sw-server/src/main/kotlin/org/luxons/sevenwonders/server/controllers/GameController.kt | 52 |
1 files changed, 40 insertions, 12 deletions
diff --git a/sw-server/src/main/kotlin/org/luxons/sevenwonders/server/controllers/GameController.kt b/sw-server/src/main/kotlin/org/luxons/sevenwonders/server/controllers/GameController.kt index 1fbabd59..bc800dea 100644 --- a/sw-server/src/main/kotlin/org/luxons/sevenwonders/server/controllers/GameController.kt +++ b/sw-server/src/main/kotlin/org/luxons/sevenwonders/server/controllers/GameController.kt @@ -40,14 +40,15 @@ class GameController( } val game = player.game - player.isReady = true - - synchronized(lobby) { - val players = lobby.getPlayers() + // This lock doesn't have a clear rationale, but it's cleaner to check the readiness state of everyone within a + // lock together with the code that updates the readiness status, to avoid interleaving surprises. + synchronized(game) { + player.isReady = true sendPlayerReady(game.id, player) logger.info("Game {}: player {} is ready for the next turn", game.id, player) + val players = lobby.getPlayers() val allReady = players.all { it.isReady } if (allReady) { logger.info("Game {}: all players ready, sending turn info", game.id) @@ -68,6 +69,22 @@ class GameController( val player = principal.player val lobby = player.lobby val game = player.game + + // We need this lock for multiple reasons: + // 1. we don't want the player to be able to unprepare his card between prepareMove and sendPreparedCard, + // because other players may receive the "unprepared" update before the "prepared" and thus end up still + // seeing the prepared card. + // 2. we don't want a player to unprepare a card between allPlayersPreparedTheirMove and playTurn, because the + // error thrown by playTurn would be received by the current (preparing) player instead of the player + // unpreparing his card, which would be confusing because the card preparation is a success in this case. + // It is therefore better for the player who unprepares his card to wait for the lock and then get rejected + // because the turn actually happened. + // 3. we don't want a player to prepare a card between the preparation and the turn-end check, because the check + // for the current player would then take into account that other card and maybe play the turn, which means + // that we would run the turn-end check of the other player right after this one. This seems benign but it + // actually doesn't respect the implicit assumption that a move in the current turn has just been prepared + // before the check. It shouldn't cause harm at the moment but could be harmful in the future. + // 4. we don't want this code to run in the middle of unprepareMove's own lock synchronized(game) { val preparedCardBack = game.prepareMove(player.index, action.move) val preparedCard = PreparedCard(player.username, preparedCardBack) @@ -92,12 +109,16 @@ class GameController( fun unprepareMove(principal: Principal) { val player = principal.player val game = player.game + + // We don't want the player to be able to prepare a card between unprepareMove and sendPreparedCard(null), + // otherwise other players may receive the "prepared" update before the "unprepared" and thus end up not seeing + // the prepared card. Note that this protection also requires the lock inside prepareMove. synchronized(game) { game.unprepareMove(player.index) + val preparedCard = PreparedCard(player.username, null) + logger.info("Game {}: player {} unprepared his move", game.id, player) + sendPreparedCard(game.id, preparedCard) } - val preparedCard = PreparedCard(player.username, null) - logger.info("Game {}: player {} unprepared his move", game.id, player) - sendPreparedCard(game.id, preparedCard) } private fun sendPlayerReady(gameId: Long, player: Player) = @@ -120,11 +141,18 @@ class GameController( val player = principal.player val game = player.game val lobby = player.lobby - lobby.removePlayer(player.username) - logger.info("Game {}: player {} left the game", game.id, player) - if (lobby.getPlayers().isEmpty()) { - lobbyRepository.remove(lobby.id) - logger.info("Game {}: game deleted", game.id) + + // This lock is for multiple reasons: + // 1. this ensures we don't remove players while the last sendTurnInfo is still notifying other players + // 2. we don't want another player to leave between removePlayer and the lobby deletion check, because if he's + // the last player, we would delete the game here, and he would also try to delete the game a second time. + synchronized(game) { + lobby.removePlayer(player.username) + logger.info("Game {}: player {} left the game", game.id, player) + if (lobby.getPlayers().isEmpty()) { + lobbyRepository.remove(lobby.id) + logger.info("Game {}: game deleted", game.id) + } } } |