Refactor launch_game so it can be called without awaiting

server/ladder_service.py

@@ -463,17 +463,19 @@ def get_player_mean(player):
                 game_options=game_options
             )
 
-            def game_options(player: Player) -> GameLaunchOptions:
+            def make_game_options(player: Player) -> GameLaunchOptions:
                 return options._replace(
                     team=game.get_player_option(player.id, "Team"),
                     faction=player.faction,
                     map_position=game.get_player_option(player.id, "StartSpot")
                 )
 
-            await host.lobby_connection.launch_game(
-                game, is_host=True, options=game_options(host)
-            )
             try:
+                host.lobby_connection.write_launch_game(
+                    game,
+                    is_host=True,
+                    options=make_game_options(host)
+                )
                 await game.wait_hosted(60)
             finally:
                 # TODO: Once the client supports `match_cancelled`, don't
@@ -482,13 +484,13 @@ def game_options(player: Player) -> GameLaunchOptions:
                 # think it is searching for ladder, even though the server has
                 # already removed it from the queue.
 
-                await asyncio.gather(*[
-                    guest.lobby_connection.launch_game(
-                        game, is_host=False, options=game_options(guest)
-                    )
-                    for guest in all_guests
-                    if guest.lobby_connection is not None
-                ])
+                for guest in all_guests:
+                    if guest.lobby_connection is not None:
+                        guest.lobby_connection.write_launch_game(
+                            game,
+                            is_host=False,
+                            options=make_game_options(guest)
+                        )
             await game.wait_launched(60 + 10 * len(all_guests))
             self._logger.debug("Ladder game launched successfully %s", game)
         except Exception as e:

server/lobbyconnection.py

@@ -41,6 +41,7 @@
     CoopGame,
     CustomGame,
     FeaturedModType,
+    Game,
     GameConnectionState,
     GameState,
     InitMode,
@@ -86,11 +87,11 @@ def __init__(
         self.rating_service = rating_service
         self.oauth_service = oauth_service
         self._authenticated = False
-        self.player = None  # type: Player
-        self.game_connection = None  # type: GameConnection
-        self.peer_address = None  # type: Optional[Address]
+        self.player: Optional[Player] = None
+        self.game_connection: Optional[GameConnection] = None
+        self.peer_address: Optional[Address] = None
         self.session = int(random.randrange(0, 4294967295))
-        self.protocol: Protocol = None
+        self.protocol: Optional[Protocol] = None
         self.user_agent = None
         self.version = None
 
@@ -1010,15 +1011,42 @@ async def command_match_ready(self, message):
 
     async def launch_game(
         self,
-        game,
-        is_host=False,
-        options=GameLaunchOptions(),
-    ):
-        assert self.player is not None
-        # TODO: Fix setting up a ridiculous amount of cyclic pointers here
+        game: Game,
+        is_host: bool = False,
+        options: GameLaunchOptions = GameLaunchOptions(),
+    ) -> None:
         if self.game_connection:
             await self.game_connection.abort("Player launched a new game")
+            self.game_connection = None
+
+        await self.send(self._prepare_launch_game(
+            game,
+            is_host=is_host,
+            options=options
+        ))
+
+    def write_launch_game(
+        self,
+        game: Game,
+        is_host: bool = False,
+        options: GameLaunchOptions = GameLaunchOptions(),
+    ) -> None:
+        self.write(self._prepare_launch_game(
+            game,
+            is_host=is_host,
+            options=options
+        ))
+
+    def _prepare_launch_game(
+        self,
+        game: Game,
+        is_host: bool = False,
+        options: GameLaunchOptions = GameLaunchOptions(),
+    ):
+        assert self.player is not None
+        assert self.game_connection is None
 
+        # TODO: Fix setting up a ridiculous amount of cyclic pointers here
         if is_host:
             game.host = self.player
 
@@ -1051,7 +1079,7 @@ async def launch_game(
             **options._asdict()
         }
 
-        await self.send({k: v for k, v in cmd.items() if v is not None})
+        return {k: v for k, v in cmd.items() if v is not None}
 
     async def command_modvault(self, message):
         type = message["type"]

tests/unit_tests/test_ladder_service.py

@@ -143,18 +143,14 @@ async def test_start_game_1v1(
     monkeypatch.setattr(LadderGame, "wait_hosted", CoroutineMock())
     monkeypatch.setattr(LadderGame, "wait_launched", CoroutineMock())
     monkeypatch.setattr(LadderGame, "timeout_game", CoroutineMock())
-    for player in (player1, player2):
-        player.lobby_connection.launch_game = CoroutineMock(
-            spec=LobbyConnection.launch_game
-        )
 
     await ladder_service.start_game([player1], [player2], queue)
 
     game = game_service[game_service.game_id_counter]
 
-    assert player1.lobby_connection.launch_game.called
+    assert player1.lobby_connection.write_launch_game.called
     # TODO: Once client supports `match_cancelled` change this to `assert not`
-    assert player2.lobby_connection.launch_game.called
+    assert player2.lobby_connection.write_launch_game.called
     assert isinstance(game, LadderGame)
     assert game.rating_type == queue.rating_type
     assert game.max_players == 2
@@ -215,14 +211,52 @@ async def test_start_game_timeout(
         "command": "match_cancelled",
         "game_id": 41956
     })
-    assert p1.lobby_connection.launch_game.called
+    assert p1.lobby_connection.write_launch_game.called
     # TODO: Once client supports `match_cancelled` change this to `assert not`
     # and uncomment the following lines.
-    assert p2.lobby_connection.launch_game.called
+    assert p2.lobby_connection.write_launch_game.called
     # assert p1.state is PlayerState.IDLE
     # assert p2.state is PlayerState.IDLE
 
 
+@fast_forward(200)
+async def test_start_game_timeout_on_send(
+    ladder_service: LadderService,
+    player_factory,
+    monkeypatch
+):
+    queue = ladder_service.queues["ladder1v1"]
+    p1 = player_factory("Dostya", player_id=1, lobby_connection_spec="auto")
+    p2 = player_factory("Rhiza", player_id=2, lobby_connection_spec="auto")
+
+    monkeypatch.setattr(LadderGame, "timeout_game", CoroutineMock())
+    monkeypatch.setattr(LadderGame, "on_game_end", CoroutineMock())
+
+    async def wait_forever(*args, **kwargs):
+        await asyncio.sleep(1000)
+    # Even though launch_game isn't called by start_game, these mocks are
+    # important for the test in case someone refactors the code to call it.
+    p1.lobby_connection.launch_game.side_effect = wait_forever
+    p2.lobby_connection.launch_game.side_effect = wait_forever
+
+    await asyncio.wait_for(
+        ladder_service.start_game([p1], [p2], queue),
+        timeout=150
+    )
+
+    LadderGame.timeout_game.assert_called_once()
+    LadderGame.on_game_end.assert_called()
+    p1.lobby_connection.write.assert_called_once_with({
+        "command": "match_cancelled",
+        "game_id": 41956
+    })
+    p2.lobby_connection.write.assert_called_once_with({
+        "command": "match_cancelled",
+        "game_id": 41956
+    })
+    assert p1.lobby_connection.write_launch_game.called
+
+
 @given(
     player1=st_players("p1", player_id=1, lobby_connection_spec="mock"),
     player2=st_players("p2", player_id=2, lobby_connection_spec="mock"),
@@ -245,10 +279,6 @@ async def test_start_game_with_teams(
     monkeypatch.setattr(LadderGame, "wait_hosted", CoroutineMock())
     monkeypatch.setattr(LadderGame, "wait_launched", CoroutineMock())
     monkeypatch.setattr(LadderGame, "timeout_game", CoroutineMock())
-    for player in (player1, player2, player3, player4):
-        player.lobby_connection.launch_game = CoroutineMock(
-            spec=LobbyConnection.launch_game
-        )
 
     await ladder_service.start_game(
         [player1, player3],
@@ -258,10 +288,10 @@ async def test_start_game_with_teams(
 
     game = game_service[game_service.game_id_counter]
 
-    assert player1.lobby_connection.launch_game.called
-    assert player2.lobby_connection.launch_game.called
-    assert player3.lobby_connection.launch_game.called
-    assert player4.lobby_connection.launch_game.called
+    assert player1.lobby_connection.write_launch_game.called
+    assert player2.lobby_connection.write_launch_game.called
+    assert player3.lobby_connection.write_launch_game.called
+    assert player4.lobby_connection.write_launch_game.called
     assert isinstance(game, LadderGame)
     assert game.rating_type == queue.rating_type
     assert game.max_players == 4