Improve synchronization in GameController

Related:
https://github.com/joffrey-bion/seven-wonders/issues/71

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