From 335b3fc2083ed9c2016f2ae5e4140fd1f015d072 Mon Sep 17 00:00:00 2001 From: Askaholic Date: Tue, 2 Jan 2024 22:56:12 -0500 Subject: [PATCH] Add STARTING_GAME state and ensure GameConnections are cleaned up We need to protect against double game_host/join messages which means we need to establish a time window after a game_launch message is sent during which we are waiting to see that the game has opened and don't let the client do any other game related actions. This also means we need to clean up the GameConnection objects which would previously stick around forever if a client never responded to a game_launch message. --- server/gameconnection.py | 39 +++++++++---------- server/games/game.py | 1 + server/ladder_service/ladder_service.py | 2 - server/lobbyconnection.py | 10 ++++- server/players.py | 1 + tests/integration_tests/test_game.py | 14 ++++++- .../test_matchmaker_violations.py | 33 ++++++++++++++++ tests/integration_tests/test_server.py | 29 ++++++++++++++ tests/unit_tests/conftest.py | 2 +- tests/unit_tests/test_gameconnection.py | 8 ++-- tests/unit_tests/test_lobbyconnection.py | 6 +-- 11 files changed, 112 insertions(+), 33 deletions(-) diff --git a/server/gameconnection.py b/server/gameconnection.py index ed0a1321e..0a0ce2ddf 100644 --- a/server/gameconnection.py +++ b/server/gameconnection.py @@ -42,7 +42,8 @@ def __init__( protocol: Protocol, player_service: PlayerService, games: GameService, - state: GameConnectionState = GameConnectionState.INITIALIZING + state: GameConnectionState = GameConnectionState.INITIALIZING, + setup_timeout: int = 60, ): """ Construct a new GameConnection @@ -52,38 +53,35 @@ def __init__( f"{self.__class__.__qualname__}.{game.id}" ) self._db = database - self._logger.debug("GameConnection initializing") self.protocol = protocol self._state = state self.game_service = games self.player_service = player_service - self._player = player + self.player = player player.game_connection = self # Set up weak reference to self - self._game = game + self.game = game - self.finished_sim = False + self.setup_timeout = setup_timeout - @property - def state(self) -> GameConnectionState: - return self._state + self.finished_sim = False - @property - def game(self) -> Game: - return self._game + self._logger.debug("GameConnection initializing") + if self.state is GameConnectionState.INITIALIZING: + asyncio.get_event_loop().create_task( + self.timeout_game_connection(setup_timeout) + ) - @game.setter - def game(self, val: Game): - self._game = val + async def timeout_game_connection(self, timeout): + await asyncio.sleep(timeout) + if self.state is GameConnectionState.INITIALIZING: + self._logger.debug("GameConection timed out...") + await self.abort("Player took too long to start the game") @property - def player(self) -> Player: - return self._player - - @player.setter - def player(self, val: Player): - self._player = val + def state(self) -> GameConnectionState: + return self._state def is_host(self) -> bool: if not self.game or not self.player: @@ -122,6 +120,7 @@ async def _handle_idle_state(self): self.game.add_game_connection(self) self.player.state = PlayerState.HOSTING else: + self._state = GameConnectionState.INITIALIZED self.player.state = PlayerState.JOINING async def _handle_lobby_state(self): diff --git a/server/games/game.py b/server/games/game.py index fe47226ec..11ef9ac3e 100644 --- a/server/games/game.py +++ b/server/games/game.py @@ -103,6 +103,7 @@ def __init__( self.displayed_rating_range = displayed_rating_range or InclusiveRange() self.enforce_rating_range = enforce_rating_range self.matchmaker_queue_id = matchmaker_queue_id + self.setup_timeout = setup_timeout self.state = GameState.INITIALIZING self._connections = {} self._configured_player_ids: set[int] = set() diff --git a/server/ladder_service/ladder_service.py b/server/ladder_service/ladder_service.py index ee73d3061..39cda2709 100644 --- a/server/ladder_service/ladder_service.py +++ b/server/ladder_service/ladder_service.py @@ -611,8 +611,6 @@ def make_game_options(player: Player) -> GameLaunchOptions: game_id = game.id if game else None msg = {"command": "match_cancelled", "game_id": game_id} for player in all_players: - if player.state == PlayerState.STARTING_AUTOMATCH: - player.state = PlayerState.IDLE player.write_message(msg) if abandoning_players: diff --git a/server/lobbyconnection.py b/server/lobbyconnection.py index 45c0ca130..937876dbd 100644 --- a/server/lobbyconnection.py +++ b/server/lobbyconnection.py @@ -1069,6 +1069,10 @@ def _prepare_launch_game( ): assert self.player is not None assert self.game_connection is None + assert self.player.state in ( + PlayerState.IDLE, + PlayerState.STARTING_AUTOMATCH, + ) # TODO: Fix setting up a ridiculous amount of cyclic pointers here if is_host: @@ -1080,9 +1084,13 @@ def _prepare_launch_game( player=self.player, protocol=self.protocol, player_service=self.player_service, - games=self.game_service + games=self.game_service, + setup_timeout=game.setup_timeout, ) + if self.player.state is PlayerState.IDLE: + self.player.state = PlayerState.STARTING_GAME + self.player.game = game cmd = { "command": "game_launch", diff --git a/server/players.py b/server/players.py index 5f8b37129..0380d7602 100644 --- a/server/players.py +++ b/server/players.py @@ -21,6 +21,7 @@ class PlayerState(Enum): JOINING = 4 SEARCHING_LADDER = 5 STARTING_AUTOMATCH = 6 + STARTING_GAME = 7 class Player: diff --git a/tests/integration_tests/test_game.py b/tests/integration_tests/test_game.py index df3aba269..bdf63d154 100644 --- a/tests/integration_tests/test_game.py +++ b/tests/integration_tests/test_game.py @@ -632,7 +632,12 @@ async def test_double_host_message(lobby_server): "mod": "faf", "visibility": "public", }) - await read_until_command(proto, "game_launch", timeout=10) + msg = await read_until_command(proto, "notice", timeout=10) + assert msg == { + "command": "notice", + "style": "error", + "text": "Can't host a game while in state STARTING_GAME", + } @fast_forward(30) @@ -661,7 +666,12 @@ async def test_double_join_message(lobby_server): "command": "game_join", "uid": game_id }) - await read_until_command(guest_proto, "game_launch", timeout=10) + msg = await read_until_command(guest_proto, "notice", timeout=10) + assert msg == { + "command": "notice", + "style": "error", + "text": "Can't join a game while in state STARTING_GAME", + } @fast_forward(30) diff --git a/tests/integration_tests/test_matchmaker_violations.py b/tests/integration_tests/test_matchmaker_violations.py index 542df8452..ca941c834 100644 --- a/tests/integration_tests/test_matchmaker_violations.py +++ b/tests/integration_tests/test_matchmaker_violations.py @@ -120,12 +120,24 @@ async def test_violation_persisted_across_logins(mocker, lobby_server): await read_until_command(host, "match_cancelled", timeout=120) await read_until_command(guest, "match_cancelled", timeout=10) + await read_until_command(host, "game_info", timeout=10, state="closed") + + # DEPRECATED: Because the game sends a game_launch to the guest after the + # host times out, we need to simulate opening and closing the game before + # we can queue again. If we wait for the timeout, our violations expire. + await open_fa(guest) + await guest.send_message({ + "target": "game", + "command": "GameState", + "args": ["Ended"] + }) # Second time searching there is no ban await start_search(host) await start_search(guest) await read_until_command(host, "match_cancelled", timeout=120) await read_until_command(guest, "match_cancelled", timeout=10) + await read_until_command(host, "game_info", timeout=10, state="closed") # Third time searching there is a short ban await host.send_message({ @@ -177,6 +189,17 @@ async def test_violation_persisted_across_parties(mocker, lobby_server): await read_until_command(host, "match_cancelled", timeout=120) await read_until_command(guest, "match_cancelled", timeout=10) + await read_until_command(host, "game_info", timeout=10, state="closed") + + # DEPRECATED: Because the game sends a game_launch to the guest after the + # host times out, we need to simulate opening and closing the game before + # we can queue again. If we wait for the timeout, our violations expire. + await open_fa(guest) + await guest.send_message({ + "target": "game", + "command": "GameState", + "args": ["Ended"] + }) # Second time searching there is no ban await start_search(host) @@ -184,6 +207,16 @@ async def test_violation_persisted_across_parties(mocker, lobby_server): await read_until_command(host, "match_cancelled", timeout=120) await read_until_command(guest, "match_cancelled", timeout=10) + # DEPRECATED: Because the game sends a game_launch to the guest after the + # host times out, we need to simulate opening and closing the game before + # we can queue again. If we wait for the timeout, our violations expire. + await open_fa(guest) + await guest.send_message({ + "target": "game", + "command": "GameState", + "args": ["Ended"] + }) + # Third time searching there is a short ban await host.send_message({ "command": "game_matchmaking", diff --git a/tests/integration_tests/test_server.py b/tests/integration_tests/test_server.py index 107fd0676..9a5d38d14 100644 --- a/tests/integration_tests/test_server.py +++ b/tests/integration_tests/test_server.py @@ -733,6 +733,35 @@ async def test_game_host_name_non_ascii(lobby_server): } +@fast_forward(30) +async def test_game_host_then_queue(lobby_server): + player_id, session, proto = await connect_and_sign_in( + ("test", "test_password"), + lobby_server + ) + await read_until_command(proto, "game_info") + + # Send game_host but don't send GameState: Idle, then immediately queue + await proto.send_message({ + "command": "game_host", + "title": "My Game", + "mod": "faf", + "visibility": "public", + }) + await proto.send_message({ + "command": "game_matchmaking", + "state": "start", + "faction": "uef" + }) + + msg = await read_until_command(proto, "notice", timeout=10) + assert msg == { + "command": "notice", + "style": "error", + "text": "Can't join a queue while test is in state STARTING_GAME", + } + + @fast_forward(10) async def test_play_game_while_queueing(lobby_server): player_id, session, proto = await connect_and_sign_in( diff --git a/tests/unit_tests/conftest.py b/tests/unit_tests/conftest.py index 7d65cdbb7..b7f3093c9 100644 --- a/tests/unit_tests/conftest.py +++ b/tests/unit_tests/conftest.py @@ -74,7 +74,7 @@ async def violation_service(): @pytest.fixture -def game_connection( +async def game_connection( request, database, game, diff --git a/tests/unit_tests/test_gameconnection.py b/tests/unit_tests/test_gameconnection.py index 47efb5b16..ab94f2a24 100644 --- a/tests/unit_tests/test_gameconnection.py +++ b/tests/unit_tests/test_gameconnection.py @@ -62,12 +62,12 @@ async def fake_send_dc(player_id): return "OK" # Set up a peer that will disconnect without error - ok_disconnect = mock.create_autospec(GameConnection) + ok_disconnect = mock.create_autospec(game_connection) ok_disconnect.state = GameConnectionState.CONNECTED_TO_HOST ok_disconnect.send_DisconnectFromPeer = fake_send_dc # Set up a peer that will throw an exception - fail_disconnect = mock.create_autospec(GameConnection) + fail_disconnect = mock.create_autospec(game_connection) fail_disconnect.send_DisconnectFromPeer.return_value = Exception("Test exception") fail_disconnect.state = GameConnectionState.CONNECTED_TO_HOST @@ -81,7 +81,7 @@ async def fake_send_dc(player_id): async def test_connect_to_peer(game_connection): - peer = mock.create_autospec(GameConnection) + peer = mock.create_autospec(game_connection) await game_connection.connect_to_peer(peer) @@ -92,7 +92,7 @@ async def test_connect_to_peer_disconnected(game_connection): # Weak reference has dissapeared await game_connection.connect_to_peer(None) - peer = mock.create_autospec(GameConnection) + peer = mock.create_autospec(game_connection) peer.send_ConnectToPeer.side_effect = DisconnectedError("Test error") # The client disconnects right as we send the message diff --git a/tests/unit_tests/test_lobbyconnection.py b/tests/unit_tests/test_lobbyconnection.py index c0bd7e70a..6085992db 100644 --- a/tests/unit_tests/test_lobbyconnection.py +++ b/tests/unit_tests/test_lobbyconnection.py @@ -264,7 +264,7 @@ async def test_launch_game(lobbyconnection, game, player_factory): assert lobbyconnection.player.game == game assert lobbyconnection.player.game_connection == lobbyconnection.game_connection assert lobbyconnection.game_connection.player == lobbyconnection.player - assert lobbyconnection.player.state == PlayerState.IDLE + assert lobbyconnection.player.state == PlayerState.STARTING_GAME lobbyconnection.send.assert_called_once() @@ -791,7 +791,6 @@ async def test_game_connection_not_restored_if_game_state_prohibits( game_service: GameService, game_stats_service, game_state, - mocker, database ): del lobbyconnection.player.game_connection @@ -998,8 +997,9 @@ async def test_command_matchmaker_info( }) -async def test_connection_lost(lobbyconnection): +async def test_connection_lost(lobbyconnection, players): lobbyconnection.game_connection = mock.create_autospec(GameConnection) + lobbyconnection.game_connection.player = players.hosting await lobbyconnection.on_connection_lost() lobbyconnection.game_connection.on_connection_lost.assert_called_once()