From ef039da728f101aae51ae476f1e37e512badba72 Mon Sep 17 00:00:00 2001 From: Chris Gerth Date: Thu, 28 Dec 2023 22:36:15 -0600 Subject: [PATCH] Uploaded .json settings bug fixups (#1082) Resolved race condition between saveGlobal and saveOneFile modifying settings on shutdown. Previously, saveGlobal would overwrite the action of saveOneFile on a clean shutdown. --- .../configuration/SqlConfigProvider.java | 75 +++++++++++++------ .../photonvision/server/RequestHandler.java | 24 +++--- 2 files changed, 68 insertions(+), 31 deletions(-) diff --git a/photon-core/src/main/java/org/photonvision/common/configuration/SqlConfigProvider.java b/photon-core/src/main/java/org/photonvision/common/configuration/SqlConfigProvider.java index 883b3607d8..95f01e4079 100644 --- a/photon-core/src/main/java/org/photonvision/common/configuration/SqlConfigProvider.java +++ b/photon-core/src/main/java/org/photonvision/common/configuration/SqlConfigProvider.java @@ -343,6 +343,29 @@ private void addFile(PreparedStatement ps, String key, String value) throws SQLE ps.setString(2, value); } + // NOTE to Future Developers: + // These booleans form a mechanism to prevent saveGlobal() and + // saveOneFile() from stepping on each other's toes. Both write + // to the database on disk, and both write to the same keys, but + // they use different sources. Generally, if the user has done something + // to trigger saveOneFile() to get called, it implies they want that + // configuration, and not whatever is in RAM right now (which is what + // saveGlobal() uses to write). Therefor, once saveOneFile() is invoked, + // we record which entry was overwritten in the database and prevent + // overwriting it when saveGlobal() is invoked (likely by the shutdown + // that should almost almost almost happen right after saveOneFile() is + // invoked). + // + // In the future, this may not be needed. A better architecture would involve + // manipulating the RAM representation of configuration when new .json files + // are uploaded in the UI, and eliminate all other usages of saveOneFile(). + // But, seeing as it's Dec 28 and kickoff is nigh, we put this here and moved on. + // Thank you for coming to my TED talk. + private boolean skipSavingHWCfg = false; + private boolean skipSavingHWSet = false; + private boolean skipSavingNWCfg = false; + private boolean skipSavingAPRTG = false; + private void saveGlobal(Connection conn) { PreparedStatement statement1 = null; PreparedStatement statement2 = null; @@ -351,28 +374,34 @@ private void saveGlobal(Connection conn) { // Replace this camera's row with the new settings var sqlString = "REPLACE INTO global (filename, contents) VALUES " + "(?,?);"; - statement1 = conn.prepareStatement(sqlString); - addFile( - statement1, - TableKeys.HARDWARE_SETTINGS, - JacksonUtils.serializeToString(config.getHardwareSettings())); - statement1.executeUpdate(); + if (!skipSavingHWSet) { + statement1 = conn.prepareStatement(sqlString); + addFile( + statement1, + TableKeys.HARDWARE_SETTINGS, + JacksonUtils.serializeToString(config.getHardwareSettings())); + statement1.executeUpdate(); + } - statement2 = conn.prepareStatement(sqlString); - addFile( - statement2, - TableKeys.NETWORK_CONFIG, - JacksonUtils.serializeToString(config.getNetworkConfig())); - statement2.executeUpdate(); - statement2.close(); - - statement3 = conn.prepareStatement(sqlString); - addFile( - statement3, - TableKeys.HARDWARE_CONFIG, - JacksonUtils.serializeToString(config.getHardwareConfig())); - statement3.executeUpdate(); - statement3.close(); + if (!skipSavingNWCfg) { + statement2 = conn.prepareStatement(sqlString); + addFile( + statement2, + TableKeys.NETWORK_CONFIG, + JacksonUtils.serializeToString(config.getNetworkConfig())); + statement2.executeUpdate(); + statement2.close(); + } + + if (!skipSavingHWCfg) { + statement3 = conn.prepareStatement(sqlString); + addFile( + statement3, + TableKeys.HARDWARE_CONFIG, + JacksonUtils.serializeToString(config.getHardwareConfig())); + statement3.executeUpdate(); + statement3.close(); + } } catch (SQLException | IOException e) { logger.error("Err saving global", e); @@ -431,21 +460,25 @@ private boolean saveOneFile(String fname, Path path) { @Override public boolean saveUploadedHardwareConfig(Path uploadPath) { + skipSavingHWCfg = true; return saveOneFile(TableKeys.HARDWARE_CONFIG, uploadPath); } @Override public boolean saveUploadedHardwareSettings(Path uploadPath) { + skipSavingHWSet = true; return saveOneFile(TableKeys.HARDWARE_SETTINGS, uploadPath); } @Override public boolean saveUploadedNetworkConfig(Path uploadPath) { + skipSavingNWCfg = true; return saveOneFile(TableKeys.NETWORK_CONFIG, uploadPath); } @Override public boolean saveUploadedAprilTagFieldLayout(Path uploadPath) { + skipSavingAPRTG = true; return saveOneFile(TableKeys.ATFL_CONFIG_FILE, uploadPath); } diff --git a/photon-server/src/main/java/org/photonvision/server/RequestHandler.java b/photon-server/src/main/java/org/photonvision/server/RequestHandler.java index d277f2904f..1d490c1875 100644 --- a/photon-server/src/main/java/org/photonvision/server/RequestHandler.java +++ b/photon-server/src/main/java/org/photonvision/server/RequestHandler.java @@ -89,8 +89,8 @@ public static void onSettingsImportRequest(Context ctx) { if (ConfigManager.saveUploadedSettingsZip(tempFilePath.get())) { ctx.status(200); - ctx.result("Successfully saved the uploaded settings zip, rebooting"); - logger.info("Successfully saved the uploaded settings zip, rebooting"); + ctx.result("Successfully saved the uploaded settings zip, rebooting..."); + logger.info("Successfully saved the uploaded settings zip, rebooting..."); restartProgram(); } else { ctx.status(500); @@ -153,8 +153,9 @@ public static void onHardwareConfigRequest(Context ctx) { if (ConfigManager.getInstance().saveUploadedHardwareConfig(tempFilePath.get().toPath())) { ctx.status(200); - ctx.result("Successfully saved the uploaded hardware config"); - logger.info("Successfully saved the uploaded hardware config"); + ctx.result("Successfully saved the uploaded hardware config, rebooting..."); + logger.info("Successfully saved the uploaded hardware config, rebooting..."); + restartProgram(); } else { ctx.status(500); ctx.result("There was an error while saving the uploaded hardware config"); @@ -195,8 +196,9 @@ public static void onHardwareSettingsRequest(Context ctx) { if (ConfigManager.getInstance().saveUploadedHardwareSettings(tempFilePath.get().toPath())) { ctx.status(200); - ctx.result("Successfully saved the uploaded hardware settings"); - logger.info("Successfully saved the uploaded hardware settings"); + ctx.result("Successfully saved the uploaded hardware settings, rebooting..."); + logger.info("Successfully saved the uploaded hardware settings, rebooting..."); + restartProgram(); } else { ctx.status(500); ctx.result("There was an error while saving the uploaded hardware settings"); @@ -237,8 +239,9 @@ public static void onNetworkConfigRequest(Context ctx) { if (ConfigManager.getInstance().saveUploadedNetworkConfig(tempFilePath.get().toPath())) { ctx.status(200); - ctx.result("Successfully saved the uploaded network config"); - logger.info("Successfully saved the uploaded network config"); + ctx.result("Successfully saved the uploaded network config, rebooting..."); + logger.info("Successfully saved the uploaded network config, rebooting..."); + restartProgram(); } else { ctx.status(500); ctx.result("There was an error while saving the uploaded network config"); @@ -279,8 +282,9 @@ public static void onAprilTagFieldLayoutRequest(Context ctx) { if (ConfigManager.getInstance().saveUploadedAprilTagFieldLayout(tempFilePath.get().toPath())) { ctx.status(200); - ctx.result("Successfully saved the uploaded AprilTagFieldLayout"); - logger.info("Successfully saved the uploaded AprilTagFieldLayout"); + ctx.result("Successfully saved the uploaded AprilTagFieldLayout, rebooting..."); + logger.info("Successfully saved the uploaded AprilTagFieldLayout, rebooting..."); + restartProgram(); } else { ctx.status(500); ctx.result("There was an error while saving the uploaded AprilTagFieldLayout");