From bcbb128691a3e696730ec187c4f7102233b792e4 Mon Sep 17 00:00:00 2001 From: asvitkine Date: Thu, 14 Mar 2024 23:05:32 -0400 Subject: [PATCH] Fix a race on startup that could clear a loaded save. (#12437) I noticed the race when running the game in debug mode from IntelliJ, where it was consistently being hit for me when I would click the button to open a save right after startup. This PR fixes it by ensuring the save game will only be loaded after the initial map load. Also, splits the function to load a map vs. a game and updates call sites. --- .../main/game/selector/GameSelectorModel.java | 85 ++++++++++++------- .../main/game/selector/GameSelectorPanel.java | 19 ++--- .../triplea/game/client/HeadedGameRunner.java | 10 ++- .../game/server/HeadlessGameServer.java | 4 +- .../game/server/HeadlessLaunchAction.java | 2 +- 5 files changed, 69 insertions(+), 51 deletions(-) diff --git a/game-app/game-core/src/main/java/games/strategy/engine/framework/startup/ui/panels/main/game/selector/GameSelectorModel.java b/game-app/game-core/src/main/java/games/strategy/engine/framework/startup/ui/panels/main/game/selector/GameSelectorModel.java index a9eadada9c7..0bcb6150873 100644 --- a/game-app/game-core/src/main/java/games/strategy/engine/framework/startup/ui/panels/main/game/selector/GameSelectorModel.java +++ b/game-app/game-core/src/main/java/games/strategy/engine/framework/startup/ui/panels/main/game/selector/GameSelectorModel.java @@ -18,6 +18,7 @@ import java.util.List; import java.util.Observable; import java.util.Optional; +import java.util.concurrent.CountDownLatch; import java.util.function.Predicate; import javax.annotation.Nullable; import lombok.Getter; @@ -45,40 +46,50 @@ public class GameSelectorModel extends Observable implements GameSelector { @Setter @Getter private ClientModel clientModelForHostBots = null; private Optional saveGameToLoad = Optional.empty(); + // Don't load a save game before the startup task to load the initial map has run, else that task + // may "lose" the race and overwrite the loaded saved game. + private final CountDownLatch readyForSaveLoad = new CountDownLatch(1); + public GameSelectorModel() {} - /** - * Loads game data by parsing a given file. - * - * @return True if successfully loaded, otherwise false. - */ - public boolean load(final Path xmlFile) { + public boolean loadMap(Path xmlFile) { + ensureExists(xmlFile); + fileName = null; + GameData gameData = parseAndValidate(xmlFile); + if (gameData != null && gameData.getGameName() == null) { + gameData = null; + } + setGameData(gameData); + this.setDefaultGame(xmlFile, gameData); + return gameData != null; + } + + public boolean loadSave(Path saveFile) { + try { + readyForSaveLoad.await(); + } catch (InterruptedException e) { + return false; + } + ensureExists(saveFile); + final GameData newData = GameDataManager.loadGame(saveFile).orElse(null); + if (newData == null) { + return false; + } + newData.setSaveGameFileName(saveFile.getFileName().toString()); + this.fileName = saveFile.getFileName().toString(); + setGameData(newData); + return true; + } + + public void setReadyForSaveLoad() { + readyForSaveLoad.countDown(); + } + + private void ensureExists(Path file) { Preconditions.checkArgument( - Files.exists(xmlFile), + Files.exists(file), "Programming error, expected file to have already been checked to exist: " - + xmlFile.toAbsolutePath()); - - // if the file name is xml, load it as a new game - if (xmlFile.getFileName().toString().toLowerCase().endsWith("xml")) { - fileName = null; - GameData gameData = parseAndValidate(xmlFile); - if (gameData != null && gameData.getGameName() == null) { - gameData = null; - } - setGameData(gameData); - this.setDefaultGame(xmlFile, gameData); - return gameData != null; - } else { - // try to load it as a saved game whatever the extension - final GameData newData = GameDataManager.loadGame(xmlFile).orElse(null); - if (newData == null) { - return false; - } - newData.setSaveGameFileName(xmlFile.getFileName().toString()); - this.fileName = xmlFile.getFileName().toString(); - setGameData(newData); - return true; - } + + file.toAbsolutePath()); } private void setDefaultGame(@Nullable final Path xmlFile, @Nullable final GameData gameData) { @@ -165,7 +176,7 @@ private void notifyObs() { /** Clears AI game over cache and loads default game in a new thread. */ @Override public void onGameEnded() { - // clear out ai cached properties (this ended up being the best place to put it, + // clear out AI cached properties (this ended up being the best place to put it, // as we have definitely left a game at this point) GameShutdownRegistry.runShutdownActions(); ThreadRunner.runInNewThread(this::loadDefaultGameSameThread); @@ -192,7 +203,17 @@ public void loadDefaultGameSameThread() { .filter(Predicate.not(String::isBlank)) .filter(GameSelectorModel::gameUriExistsOnFileSystem) .map(GameSelectorModel::pathFromGameUri) - .ifPresentOrElse(this::load, this::resetDefaultGame); + .ifPresentOrElse( + (file) -> { + // if the file name is xml, load it as a new game + if (file.getFileName().toString().toLowerCase().endsWith("xml")) { + loadMap(file); + } else { + // try to load it as a saved game whatever the extension + loadSave(file); + } + }, + this::resetDefaultGame); } private static Path pathFromGameUri(final String gameUri) { diff --git a/game-app/game-core/src/main/java/games/strategy/engine/framework/startup/ui/panels/main/game/selector/GameSelectorPanel.java b/game-app/game-core/src/main/java/games/strategy/engine/framework/startup/ui/panels/main/game/selector/GameSelectorPanel.java index aaf3c586ece..37d1b262ecf 100644 --- a/game-app/game-core/src/main/java/games/strategy/engine/framework/startup/ui/panels/main/game/selector/GameSelectorPanel.java +++ b/game-app/game-core/src/main/java/games/strategy/engine/framework/startup/ui/panels/main/game/selector/GameSelectorPanel.java @@ -238,15 +238,15 @@ private static GridBagConstraints buildGridRow(final int x, final int y, final I private static GridBagConstraints buildGrid( final int x, final int y, final Insets insets, final int width) { final int gridHeight = 1; - final double weigthX = 0; - final double weigthY = 0; + final double weightX = 0; + final double weightY = 0; final int anchor = GridBagConstraints.WEST; final int fill = GridBagConstraints.NONE; final int ipadx = 0; final int ipady = 0; return new GridBagConstraints( - x, y, width, gridHeight, weigthX, weigthY, anchor, fill, insets, ipadx, ipady); + x, y, width, gridHeight, weightX, weightY, anchor, fill, insets, ipadx, ipady); } private void setOriginalPropertiesMap(final GameData data) { @@ -367,7 +367,7 @@ public void loadSaveFile(final Path file) { .build() .run( () -> { - if (model.load(file)) { + if (model.loadSave(file)) { setOriginalPropertiesMap(model.getGameData()); } }); @@ -393,17 +393,10 @@ private void gameSelected(final Path gameFile) { BackgroundTaskRunner.runInBackground( "Loading map...", () -> { - model.load(gameFile); - // warning: NPE check is not to protect against concurrency, another thread could still - // null - // out game data. - // The NPE check is to protect against the case where there are errors loading game, in - // which case we'll have a null game data. - if (model.getGameData() != null) { + if (model.loadMap(gameFile)) { setOriginalPropertiesMap(model.getGameData()); // only for new games, not saved games, we set the default options, and set them only - // once - // (the first time it is loaded) + // once (the first time it is loaded) gamePropertiesCache.loadCachedGamePropertiesInto(model.getGameData()); } }); diff --git a/game-app/game-headed/src/main/java/org/triplea/game/client/HeadedGameRunner.java b/game-app/game-headed/src/main/java/org/triplea/game/client/HeadedGameRunner.java index fdec9f3210b..35ecfe74b44 100644 --- a/game-app/game-headed/src/main/java/org/triplea/game/client/HeadedGameRunner.java +++ b/game-app/game-headed/src/main/java/org/triplea/game/client/HeadedGameRunner.java @@ -142,7 +142,11 @@ private static void start() { headedServerSetupModel = new HeadedServerSetupModel(gameSelectorModel); MainFrame.buildMainFrame(headedServerSetupModel, gameSelectorModel); headedServerSetupModel.showSelectType(); - ThreadRunner.runInNewThread(HeadedGameRunner::showMainFrame); + ThreadRunner.runInNewThread( + () -> { + showMainFrame(); + gameSelectorModel.setReadyForSaveLoad(); + }); }); UpdateChecks.launch(); @@ -150,7 +154,7 @@ private static void start() { /** * Sets the 'main frame' to visible. In this context the main frame is the initial welcome (launch - * lobby/single player game etc..) screen presented to GUI enabled clients. + * lobby/single player game etc.) screen presented to GUI enabled clients. */ public static void showMainFrame() { GameShutdownRegistry.runShutdownActions(); @@ -169,7 +173,7 @@ public static void showMainFrame() { final String saveGameFileName = System.getProperty(TRIPLEA_GAME, ""); if (!saveGameFileName.isEmpty()) { final Path saveGameFile = Path.of(saveGameFileName); - if (Files.exists(saveGameFile) && !gameSelectorModel.load(saveGameFile)) { + if (Files.exists(saveGameFile) && !gameSelectorModel.loadSave(saveGameFile)) { // abort launch if we failed to load the specified game return; } diff --git a/game-app/game-headless/src/main/java/org/triplea/game/server/HeadlessGameServer.java b/game-app/game-headless/src/main/java/org/triplea/game/server/HeadlessGameServer.java index b3b84814902..c29028c9f78 100644 --- a/game-app/game-headless/src/main/java/org/triplea/game/server/HeadlessGameServer.java +++ b/game-app/game-headless/src/main/java/org/triplea/game/server/HeadlessGameServer.java @@ -60,7 +60,7 @@ public synchronized void setGameMapTo(final String gameName) { log.info("Requested to change map to: " + gameName); // don't change mid-game and only if we have the game if (game == null && availableGames.hasGame(gameName)) { - gameSelectorModel.load(availableGames.findGameXmlPathByGameName(gameName).orElseThrow()); + gameSelectorModel.loadMap(availableGames.findGameXmlPathByGameName(gameName).orElseThrow()); log.info("Changed to game map: " + gameName); } else { log.info( @@ -74,7 +74,7 @@ public synchronized void loadGameSave(final Path file) { Preconditions.checkArgument( Files.exists(file), "File must exist to load it: " + file.toAbsolutePath()); // don't change mid-game - if (game == null && gameSelectorModel.load(file)) { + if (game == null && gameSelectorModel.loadSave(file)) { log.info("Changed to save: " + file.getFileName()); } } diff --git a/game-app/game-headless/src/main/java/org/triplea/game/server/HeadlessLaunchAction.java b/game-app/game-headless/src/main/java/org/triplea/game/server/HeadlessLaunchAction.java index 00ef53d82f8..4ae70a1ded3 100644 --- a/game-app/game-headless/src/main/java/org/triplea/game/server/HeadlessLaunchAction.java +++ b/game-app/game-headless/src/main/java/org/triplea/game/server/HeadlessLaunchAction.java @@ -67,7 +67,7 @@ public void handleGameInterruption( serverModel.setAllPlayersToNullNodes(); final Path autoSaveFile = getAutoSaveFileUtils().getHeadlessAutoSaveFile(); if (Files.exists(autoSaveFile)) { - gameSelectorModel.load(autoSaveFile); + gameSelectorModel.loadSave(autoSaveFile); } }