diff --git a/src/fishE.py b/src/fishE.py index e67fd4c..e987f3c 100644 --- a/src/fishE.py +++ b/src/fishE.py @@ -59,6 +59,14 @@ def __init__(self, interfaceType=INTERFACE_TYPE): # Show save file selection menu (uses the UI above) self._selectSaveFile() + # "Quit" from that menu clears running rather than ending the process, + # so the front-end still gets its cleanup() (see _selectSaveFile and + # play()). Nothing below can run without a chosen slot - get_save_path() + # raises when none was selected - and there is no run to build anyway, + # so construction stops here and play() returns immediately. + if not self.running: + return + # Load the chosen slot over the defaults if it has data. # # Existence is the only condition: a file that is present but empty is a @@ -153,7 +161,10 @@ def _selectSaveFile(self): """Display the save-file menu through the UI and let the player choose. Slots and actions are presented as numbered options (so the menu renders - and reads input through the active front-end — console or pygame).""" + and reads input through the active front-end — console or pygame). + + Returns once a slot is selected, or with self.running cleared if the + player chose "Quit" without picking one.""" while True: # loop instead of recursion to avoid stack overflow save_files = self.saveFileManager.list_save_files() @@ -217,7 +228,16 @@ def _selectSaveFile(self): self._deleteSaveFile(save_files) # loop to show the refreshed menu either way elif kind == "quit": - exit(0) + # Ending the run rather than the process. exit(0) killed the + # interpreter from inside __init__, so cleanup() was never + # reached: the pygame window vanished without pygame.quit(), + # and both browser front-ends left the tab on the save-file + # menu with no ended screen (the Pyodide entry point had to + # catch SystemExit and post one itself). __init__ returns + # early on a cleared running flag, and play() then does + # nothing but clean up - one exit path for every front-end. + self.running = False + return elif kind == "damaged": # A conforming front-end refuses to return an unavailable # option's number, so this should be unreachable. It is handled @@ -266,6 +286,21 @@ def _deleteSaveFile(self, save_files): return False def play(self): + """Run the game loop, releasing the front-end however it ends. + + The cleanup() call is here, once, rather than at each of the places a + run can finish (retiring, quitting, an unhandled error): every + front-end needs it and only this method sees all of those endings. + Without it the pygame window closed without pygame.quit(), and the + browser front-ends never published their ended screen - the tab kept + polling a server that had exited and told a player who had just retired + that the connection was lost. It is a no-op for the console.""" + try: + self._runGameLoop() + finally: + self.userInterface.cleanup() + + def _runGameLoop(self): while self.running: # show the current location and goal progress in the UI header self.userInterface.currentLocationName = self.currentLocation.capitalize() diff --git a/src/ui/webUserInterface.py b/src/ui/webUserInterface.py index ca2d63e..072cb61 100644 --- a/src/ui/webUserInterface.py +++ b/src/ui/webUserInterface.py @@ -21,6 +21,19 @@ os.path.join(os.path.dirname(os.path.abspath(__file__)), "..", "..", "web") ) +# How long cleanup() gives the browser to collect the ended screen before it +# closes the server, and how often it checks whether that has happened. +# +# The client only discovers a new screen on its next poll, so shutting the +# socket the instant the ended screen is published closes it before the screen +# can be fetched: a player who had just retired was shown "Lost connection" +# instead of the end of their run. The wait ends as soon as the screen has +# actually gone out (typically within one poll interval), so the timeout is +# only ever paid when nobody is listening - a closed tab, or a game driven by +# something other than a browser. +ENDED_SCREEN_DELIVERY_TIMEOUT_SECONDS = 2.0 +ENDED_SCREEN_DELIVERY_POLL_SECONDS = 0.02 + def _readWebAsset(name): """Read a shared browser-client file from web/, or explain why it could not.""" @@ -99,6 +112,10 @@ def htmlPage(): failures = 0; if (recovered) version = -1; // force a re-render to clear the disconnect banner if (state.version !== version) { version = state.version; FisheClient.render(state.screen); } + // The game shuts its server down once this screen has gone out, so there + // is nothing left to poll for: stop, rather than spend the rest of the + // tab's life failing to reach a process that has finished. + if (state.screen && state.screen.type === "ended") { return; } } catch (e) { failures++; // Don't clobber the intentional "game ended" screen with a scary banner. @@ -135,8 +152,12 @@ def do_GET(self): if self.path in ("/", "/index.html"): self._send(200, "text/html; charset=utf-8", htmlPage().encode("utf-8")) elif self.path.startswith("/state"): - body = json.dumps(ui.get_state()).encode("utf-8") - self._send(200, "application/json", body) + state = ui.get_state() + self._send(200, "application/json", json.dumps(state).encode("utf-8")) + # Recorded only once the bytes are on the wire, so cleanup() + # cannot close the server out from under a response it is + # still writing (see record_state_delivered). + ui.record_state_delivered(state["version"]) else: self._send(404, "text/plain", b"Not found") @@ -184,11 +205,17 @@ def __init__( host="127.0.0.1", port=8000, start_server=True, + endedScreenTimeoutSeconds=ENDED_SCREEN_DELIVERY_TIMEOUT_SECONDS, ): super().__init__(currentPrompt, timeService, player) self._lock = threading.Lock() self._screen = {"type": "loading"} self._version = 0 + # The newest screen version the browser has been handed. Starts below + # the first version so "nothing has been collected yet" is a state + # cleanup() can tell apart from "the current screen has been seen". + self._deliveredVersion = -1 + self._endedScreenTimeoutSeconds = endedScreenTimeoutSeconds self._inputQueue = queue.Queue() self._server = None if start_server: @@ -211,6 +238,29 @@ def submit_input(self, value): """Deliver the player's browser response to the waiting game thread.""" self._inputQueue.put(value) + def record_state_delivered(self, version): + """Note that the browser has been sent the screen at this version. + + Only cleanup() reads this, to know the ended screen reached the page + before the server is closed. Kept as the highest version seen so an + overlapping poll that finishes late cannot walk it backwards.""" + with self._lock: + self._deliveredVersion = max(self._deliveredVersion, version) + + def _awaitScreenDelivery(self, timeout): + """Block until the current screen has been sent to the browser. + + Returns True if it went out, False if the timeout ran out first - + which is the ordinary outcome when no page is polling.""" + deadline = time.monotonic() + timeout + while True: + with self._lock: + if self._deliveredVersion >= self._version: + return True + if time.monotonic() >= deadline: + return False + time.sleep(ENDED_SCREEN_DELIVERY_POLL_SECONDS) + # --- transport seams --------------------------------------------------- # Every screen below is built once and shared by both web front-ends; only # how a screen reaches the browser (_present) and how the response comes @@ -303,6 +353,10 @@ def timedKeyPress(self, message): def cleanup(self): self._present({"type": "ended"}) if self._server is not None: + # Hold the server open until the page has the ended screen; + # otherwise the run's last screen is never fetched and the browser + # reports a lost connection instead. + self._awaitScreenDelivery(self._endedScreenTimeoutSeconds) self._server.shutdown() self._server.server_close() self._server = None diff --git a/tests/test_fishE.py b/tests/test_fishE.py index baac6c5..cbd8fe4 100644 --- a/tests/test_fishE.py +++ b/tests/test_fishE.py @@ -900,13 +900,14 @@ def test_selectSaveFile_delete_then_quit(): # Delete submenu: Delete Slot 1 / Cancel -> "2" (Cancel) # Menu again: same options -> "4" (Quit) game.userInterface.showOptions.side_effect = ["3", "2", "4"] + game.running = True - # call/check - Quit calls exit(0), which raises SystemExit - try: - game._selectSaveFile() - assert False, "expected SystemExit" - except SystemExit as e: - assert e.code == 0 + # call - Quit returns rather than raising SystemExit; ending the process + # here skipped the front-end's cleanup() (see FishE.play) + game._selectSaveFile() + + # check - the run is over and no slot was claimed + assert game.running is False game.saveFileManager.select_save_slot.assert_not_called() @@ -1160,3 +1161,72 @@ def test_play_appends_the_fleet_report_and_the_eviction_together(): # check assert "The Marauder landed 12 fish." in game.prompt.text assert housing.EVICTION_MESSAGE in game.prompt.text + + +def test_play_cleans_up_the_front_end_when_the_run_ends(): + # Retiring or quitting used to return out of play() with nothing released: + # the pygame window closed without pygame.quit(), and neither browser + # front-end ever published its ended screen. + game = createGameForPlay() + game.locations[LocationType.HOME].run.return_value = LocationType.NONE + + # call + game.play() + + # check + game.userInterface.cleanup.assert_called_once_with() + + +def test_play_cleans_up_the_front_end_when_the_loop_raises(): + # An error mid-run is exactly when a leftover window or a still-bound + # server is hardest to explain, so cleanup happens on the way out either way. + game = createGameForPlay() + game.locations[LocationType.HOME].run.side_effect = RuntimeError("kraken") + + # call/check - the error still reaches the caller + try: + game.play() + assert False, "expected RuntimeError" + except RuntimeError as error: + assert str(error) == "kraken" + game.userInterface.cleanup.assert_called_once_with() + + +def test_quit_from_the_save_file_menu_ends_the_run_without_exiting(): + # "Quit" called exit(0) from inside __init__, ending the process before + # any front-end could clean up - which is why the Pyodide entry point had + # to catch SystemExit and publish an ended screen of its own. It now + # clears running, __init__ stops early, and play() does nothing but hand + # the front-end the same cleanup() every other ending gets. + with tempfile.TemporaryDirectory() as data_directory: + fishE.Player = Player + fishE.Stats = Stats + fishE.TimeService = TimeService + fishE.Prompt = Prompt + fishE.PlayerJsonReaderWriter = PlayerJsonReaderWriter + fishE.StatsJsonReaderWriter = StatsJsonReaderWriter + fishE.TimeServiceJsonReaderWriter = TimeServiceJsonReaderWriter + fishE.SaveFileManager = SaveFileManager + + config = Config() + config.dataDirectory = data_directory + + userInterface = MagicMock() + # An empty save directory offers "Create New Save (Slot 1)" then "Quit". + userInterface.showOptions.return_value = "2" + factory = MagicMock() + factory.create_user_interface.return_value = userInterface + + # call + with patch.object(fishE, "Config", return_value=config), patch.object( + fishE, "UserInterfaceFactory", factory + ): + game = fishE.FishE() + game.play() + + # check - no slot was claimed, nothing was written, and the front-end + # was released + assert game.running is False + assert game.saveFileManager.selected_save_slot is None + assert os.listdir(data_directory) == [] + userInterface.cleanup.assert_called_once_with() diff --git a/tests/ui/test_webUserInterface.py b/tests/ui/test_webUserInterface.py index 0a370ba..c9afb57 100644 --- a/tests/ui/test_webUserInterface.py +++ b/tests/ui/test_webUserInterface.py @@ -20,13 +20,21 @@ from world.timeService import TimeService -def makeWebUI(start_server=False, port=0): +def makeWebUI(start_server=False, port=0, endedScreenTimeoutSeconds=0.1): prompt = Prompt("What would you like to do?") player = Player() stats = Stats() timeService = TimeService(player, stats) + # No browser polls these servers, so cleanup()'s wait for the ended screen + # to be collected always runs to its timeout; the production default (two + # seconds) would be paid by every test that starts one. return WebUserInterface( - prompt, timeService, player, port=port, start_server=start_server + prompt, + timeService, + player, + port=port, + start_server=start_server, + endedScreenTimeoutSeconds=endedScreenTimeoutSeconds, ) @@ -308,3 +316,79 @@ def test_showOptions_refuses_an_unavailable_choice(): ui.submit_input("2") # available thread.join(timeout=2) assert box["result"] == "2" + + +def test_cleanup_publishes_the_ended_screen(): + # check - the run's last screen says the game is over, whether or not a + # server is involved (the Pyodide front-end inherits this path) + ui = makeWebUI() + ui.cleanup() + + assert ui.get_state()["screen"] == {"type": "ended"} + + +def test_cleanup_holds_the_server_open_until_the_ended_screen_is_fetched(): + # The browser only learns the game finished on its next poll, so closing + # the socket the instant the ended screen is published loses it: the page + # would show "Lost connection" to a player who had just retired. + ui = makeWebUI(start_server=True, port=0, endedScreenTimeoutSeconds=2.0) + host, port = ui.address + base = "http://127.0.0.1:%d" % port + + thread, box = runInThread(ui.cleanup) + try: + waitForScreen(ui, "ended") + # Still serving: the ended screen has not been collected yet. + assert thread.is_alive() + state = json.loads(urllib.request.urlopen(base + "/state", timeout=2).read()) + assert state["screen"] == {"type": "ended"} + finally: + thread.join(timeout=3) + + # check - once the page has it, the server is closed rather than left running + assert not thread.is_alive() + assert ui.address is None + + +def test_cleanup_stops_waiting_when_nothing_is_listening(): + # A closed tab (or a game driven by anything other than a browser) never + # fetches the ended screen, so the wait has to end on its own. + ui = makeWebUI(start_server=True, port=0, endedScreenTimeoutSeconds=0.2) + + startTime = time.time() + ui.cleanup() + elapsed = time.time() - startTime + + assert 0.2 <= elapsed < 2.0 + assert ui.address is None + + +def test_record_state_delivered_keeps_the_highest_version_seen(): + # Two polls can overlap, and the older one can finish last; the newer + # screen must not be reported as uncollected because of it. + ui = makeWebUI() + ui._present({"type": "dialogue", "text": "Caught a fish!"}) + version = ui.get_state()["version"] + + ui.record_state_delivered(version) + ui.record_state_delivered(version - 1) + + assert ui._awaitScreenDelivery(timeout=0) is True + + +def test_waiting_for_delivery_reports_an_uncollected_screen(): + # check - a screen published after the last delivery is not treated as seen + ui = makeWebUI() + ui.record_state_delivered(ui.get_state()["version"]) + ui._present({"type": "dialogue", "text": "Caught a fish!"}) + + assert ui._awaitScreenDelivery(timeout=0) is False + + +def test_client_stops_polling_once_the_game_has_ended(): + # check - the server is gone by the time the page renders the ended + # screen, so the poll loop stops rather than failing every 300ms for the + # rest of the tab's life + page = webUserInterface.htmlPage() + + assert 'state.screen.type === "ended"' in page diff --git a/web/pyodide_main.py b/web/pyodide_main.py index d4c50f3..972ce3a 100644 --- a/web/pyodide_main.py +++ b/web/pyodide_main.py @@ -14,29 +14,16 @@ """ from ui.enum.uiType import UIType -from ui.pyodideUserInterface import SharedArrayBufferBridge from fishE import FishE def main(): - game = None - try: - game = FishE(interfaceType=UIType.PYODIDE) - game.play() - except SystemExit: - # Choosing "Quit" in the save-file manager calls exit(). In a browser - # tab there is no process to end, and letting SystemExit escape would - # surface as a Worker error, so it is treated as a normal finish. - pass - - # Nothing else calls cleanup() when the game loop ends, and without it the - # tab would sit on the last screen forever rather than saying it's over. - if game is not None: - game.userInterface.cleanup() - else: - # Quit before the game finished being built: there is no interface to - # clean up, so tell the page directly. - SharedArrayBufferBridge().postScreen({"type": "ended"}) + # play() publishes the ended screen through the front-end's cleanup() on + # every way out of the game - including "Quit" from the save-file manager, + # which ends the run rather than the process so a tab with no process to + # end is not a special case here (see FishE.play). + game = FishE(interfaceType=UIType.PYODIDE) + game.play() main()