From cd8b157289f1efcf27840cbbf114831a7fbf95a4 Mon Sep 17 00:00:00 2001 From: Maximilian Stiede Date: Mon, 6 Mar 2023 01:00:51 +0100 Subject: [PATCH] make AsynchronousSceneManager more resilient by using an ExecutorService instead of self-made thread handling, also move scene name sanitization out into separate class --- .../src/java/se/llbit/chunky/main/Chunky.java | 1 - .../scene/AsynchronousSceneManager.java | 134 ++++-------------- .../se/llbit/chunky/renderer/scene/Scene.java | 2 +- .../chunky/renderer/scene/SceneUtils.java | 75 ++++++++++ .../ui/controller/ChunkyFxController.java | 8 +- .../renderer/scene/SceneManagerTest.java | 26 ++-- 6 files changed, 123 insertions(+), 123 deletions(-) create mode 100644 chunky/src/java/se/llbit/chunky/renderer/scene/SceneUtils.java diff --git a/chunky/src/java/se/llbit/chunky/main/Chunky.java b/chunky/src/java/se/llbit/chunky/main/Chunky.java index 7fd509f909..0fec94f365 100644 --- a/chunky/src/java/se/llbit/chunky/main/Chunky.java +++ b/chunky/src/java/se/llbit/chunky/main/Chunky.java @@ -375,7 +375,6 @@ public RenderController getRenderController() { SceneProvider sceneProvider = sceneManager.getSceneProvider(); renderManager.setSceneProvider(sceneProvider); renderManager.start(); - sceneManager.start(); renderController = new RenderController(context, renderManager, sceneManager, sceneProvider); } } diff --git a/chunky/src/java/se/llbit/chunky/renderer/scene/AsynchronousSceneManager.java b/chunky/src/java/se/llbit/chunky/renderer/scene/AsynchronousSceneManager.java index a2e7b26a77..9871bba7f7 100644 --- a/chunky/src/java/se/llbit/chunky/renderer/scene/AsynchronousSceneManager.java +++ b/chunky/src/java/se/llbit/chunky/renderer/scene/AsynchronousSceneManager.java @@ -30,7 +30,8 @@ import java.io.File; import java.io.IOException; import java.util.Collection; -import java.util.concurrent.LinkedBlockingQueue; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; /** * This scene manager is used for asynchronous loading and saving of scenes. @@ -39,16 +40,39 @@ * * @author Jesper Öqvist */ -public class AsynchronousSceneManager extends Thread implements SceneManager { +public class AsynchronousSceneManager implements SceneManager { private final SynchronousSceneManager sceneManager; - private final LinkedBlockingQueue taskQueue; + private final ExecutorService executorService = Executors.newSingleThreadExecutor(runnable -> { + Thread thread = new Thread(runnable, "AsynchronousSceneManager-Thread"); + thread.setUncaughtExceptionHandler((t, exception) -> { + if (exception instanceof OutOfMemoryError) { + Log.error( + "Chunky has run out of memory! Increase the memory given to Chunky in the launcher.", + exception); + throw (Error) exception; + } else if(exception instanceof InterruptedException) { + Thread.currentThread().interrupt(); + } else { + Log.error("Scene manager has crashed due to an uncaught exception. \n" + + "Chunky might not work properly until you restart it.\n" + + "If you think this is a bug, please report it and the following lines to the developers.", + exception); + // a new thread will be created by the executor after the current one terminated and a new task is enqueued + } + }); + return thread; + }); public AsynchronousSceneManager(RenderContext context, RenderManager renderManager) { - super("Scene Manager"); - sceneManager = new SynchronousSceneManager(context, renderManager); - taskQueue = new LinkedBlockingQueue<>(); + } + + /** + * Schedule a task to be run soon. + */ + public void enqueueTask(Runnable task) { + executorService.execute(task); } @Override @@ -85,26 +109,6 @@ public void setOnChunksLoaded(Runnable onChunksLoaded) { return sceneManager.getScene(); } - @Override public void run() { - try { - while (!isInterrupted()) { - Runnable task = taskQueue.take(); - task.run(); - } - } catch (InterruptedException ignored) { - // Interrupted. - } catch (Throwable e) { - if (e instanceof OutOfMemoryError) { - Log.error("Chunky has run out of memory! Increase the memory given to Chunky in the launcher.", e); - } else { - Log.error("Scene manager has crashed due to an uncaught exception. " + - "Chunky will not work properly until you restart it. " + - "If you think this is a bug, please report it to the developers.", e); - } - throw e; - } - } - /** * Load the given scene. * @@ -229,84 +233,6 @@ public void mergeRenderDump(File renderDump) { enqueueTask(() -> sceneManager.mergeDump(renderDump)); } - /** - * Schedule a task to be run soon. - */ - public void enqueueTask(Runnable task) { - taskQueue.add(task); - } - - /** - * Remove problematic characters from scene name. - * - * @return sanitized scene name - */ - public static String sanitizedSceneName(String name, String fallbackName) { - name = name.trim(); - StringBuilder sb = new StringBuilder(); - for (int i = 0; i < name.length(); ++i) { - char c = name.charAt(i); - if (isValidSceneNameChar(c)) { - sb.append(c); - } else if (c >= '\u0020' && c <= '\u007e') { - sb.append('_'); - } - } - String stripped = sb.toString().trim(); - if (stripped.isEmpty()) { - return fallbackName; - } else { - return stripped; - } - } - - /** - * Remove problematic characters from scene name. - * - * @return sanitized scene name - */ - public static String sanitizedSceneName(String name) { - return sanitizedSceneName(name, "Scene"); - } - - /** - * @return false if the character can cause problems on any - * supported platform. - */ - public static boolean isValidSceneNameChar(char c) { - switch (c) { - case '/': - case ':': - case ';': - case '\\': // Windows file separator. - case '*': - case '?': - case '"': - case '<': - case '>': - case '|': - return false; - } - if (c < '\u0020') { - return false; - } - return c <= '\u007e' || c >= '\u00a0'; - } - - /** - * Check for scene name validity. - * - * @return true if the scene name contains only legal characters - */ - public static boolean sceneNameIsValid(String name) { - for (int i = 0; i < name.length(); ++i) { - if (!isValidSceneNameChar(name.charAt(i))) { - return false; - } - } - return true; - } - public void discardSceneChanges() { sceneManager.discardSceneChanges(); } diff --git a/chunky/src/java/se/llbit/chunky/renderer/scene/Scene.java b/chunky/src/java/se/llbit/chunky/renderer/scene/Scene.java index 20ddac8d95..2bff7fd43f 100644 --- a/chunky/src/java/se/llbit/chunky/renderer/scene/Scene.java +++ b/chunky/src/java/se/llbit/chunky/renderer/scene/Scene.java @@ -1841,7 +1841,7 @@ public Vector3i getOrigin() { * Set the scene name. */ public void setName(String newName) { - newName = AsynchronousSceneManager.sanitizedSceneName(newName); + newName = SceneUtils.sanitizedSceneName(newName); if (newName.length() > 0) { name = newName; } diff --git a/chunky/src/java/se/llbit/chunky/renderer/scene/SceneUtils.java b/chunky/src/java/se/llbit/chunky/renderer/scene/SceneUtils.java new file mode 100644 index 0000000000..50805ec73c --- /dev/null +++ b/chunky/src/java/se/llbit/chunky/renderer/scene/SceneUtils.java @@ -0,0 +1,75 @@ +package se.llbit.chunky.renderer.scene; + +public class SceneUtils { + + /** + * Remove problematic characters from scene name. + * + * @return sanitized scene name + */ + public static String sanitizedSceneName(String name, String fallbackName) { + name = name.trim(); + StringBuilder sb = new StringBuilder(); + for (int i = 0; i < name.length(); ++i) { + char c = name.charAt(i); + if (isValidSceneNameChar(c)) { + sb.append(c); + } else if (c >= '\u0020' && c <= '\u007e') { + sb.append('_'); + } + } + String stripped = sb.toString().trim(); + if (stripped.isEmpty()) { + return fallbackName; + } else { + return stripped; + } + } + + /** + * Remove problematic characters from scene name. + * + * @return sanitized scene name + */ + public static String sanitizedSceneName(String name) { + return sanitizedSceneName(name, "Scene"); + } + + /** + * @return false if the character can cause problems on any + * supported platform. + */ + public static boolean isValidSceneNameChar(char c) { + switch (c) { + case '/': + case ':': + case ';': + case '\\': // Windows file separator. + case '*': + case '?': + case '"': + case '<': + case '>': + case '|': + return false; + } + if (c < '\u0020') { + return false; + } + return c <= '\u007e' || c >= '\u00a0'; + } + + /** + * Check for scene name validity. + * + * @return true if the scene name contains only legal characters + */ + public static boolean sceneNameIsValid(String name) { + for (int i = 0; i < name.length(); ++i) { + if (!isValidSceneNameChar(name.charAt(i))) { + return false; + } + } + return true; + } +} diff --git a/chunky/src/java/se/llbit/chunky/ui/controller/ChunkyFxController.java b/chunky/src/java/se/llbit/chunky/ui/controller/ChunkyFxController.java index 8f6e6effde..751c979088 100644 --- a/chunky/src/java/se/llbit/chunky/ui/controller/ChunkyFxController.java +++ b/chunky/src/java/se/llbit/chunky/ui/controller/ChunkyFxController.java @@ -385,10 +385,10 @@ public void exportMapView() { saveScene.setOnAction(e -> asyncSceneManager.saveScene()); saveSceneAs.setOnAction((e) -> { - ValidatingTextInputDialog sceneNameDialog = new ValidatingTextInputDialog(scene.name(), AsynchronousSceneManager::sceneNameIsValid); + ValidatingTextInputDialog sceneNameDialog = new ValidatingTextInputDialog(scene.name(), SceneUtils::sceneNameIsValid); sceneNameDialog.setTitle("Save scene as…"); sceneNameDialog.setHeaderText("Enter a scene name"); - String newName = AsynchronousSceneManager.sanitizedSceneName(sceneNameDialog.showAndWait().orElse(""), ""); + String newName = SceneUtils.sanitizedSceneName(sceneNameDialog.showAndWait().orElse(""), ""); if (!newName.isEmpty() && this.promptSaveScene(newName)) { asyncSceneManager.saveSceneAs(newName); scene.setName(newName); @@ -397,10 +397,10 @@ public void exportMapView() { }); saveSceneCopy.setOnAction((e) -> { - ValidatingTextInputDialog sceneNameDialog = new ValidatingTextInputDialog("Copy of " + scene.name(), AsynchronousSceneManager::sceneNameIsValid); + ValidatingTextInputDialog sceneNameDialog = new ValidatingTextInputDialog("Copy of " + scene.name(), SceneUtils::sceneNameIsValid); sceneNameDialog.setTitle("Save a copy…"); sceneNameDialog.setHeaderText("Enter a scene name"); - String newName = AsynchronousSceneManager.sanitizedSceneName(sceneNameDialog.showAndWait().orElse(""), ""); + String newName = SceneUtils.sanitizedSceneName(sceneNameDialog.showAndWait().orElse(""), ""); if (!newName.isEmpty() && this.promptSaveScene(newName)) { File sceneDirectory = SynchronousSceneManager.resolveSceneDirectory(newName); asyncSceneManager.enqueueTask(() -> { diff --git a/chunky/src/test/se/llbit/chunky/renderer/scene/SceneManagerTest.java b/chunky/src/test/se/llbit/chunky/renderer/scene/SceneManagerTest.java index a621f540af..c8513390d8 100644 --- a/chunky/src/test/se/llbit/chunky/renderer/scene/SceneManagerTest.java +++ b/chunky/src/test/se/llbit/chunky/renderer/scene/SceneManagerTest.java @@ -30,9 +30,9 @@ public class SceneManagerTest { */ @Test public void testSceneNameSanitizing_1() { - assertEquals("ab_cd", AsynchronousSceneManager.sanitizedSceneName("ab/cd")); - assertEquals("x___cd", AsynchronousSceneManager.sanitizedSceneName("x<>|cd")); - assertEquals("______#42", AsynchronousSceneManager.sanitizedSceneName(":*?\\\"/#42")); + assertEquals("ab_cd", SceneUtils.sanitizedSceneName("ab/cd")); + assertEquals("x___cd", SceneUtils.sanitizedSceneName("x<>|cd")); + assertEquals("______#42", SceneUtils.sanitizedSceneName(":*?\\\"/#42")); } /** @@ -40,8 +40,8 @@ public void testSceneNameSanitizing_1() { */ @Test public void testSceneNameSanitizing_2() { - assertEquals("foo bar", AsynchronousSceneManager.sanitizedSceneName(" foo bar ")); - assertEquals("foo bar", AsynchronousSceneManager.sanitizedSceneName(" \nfoo bar\t ")); + assertEquals("foo bar", SceneUtils.sanitizedSceneName(" foo bar ")); + assertEquals("foo bar", SceneUtils.sanitizedSceneName(" \nfoo bar\t ")); } /** @@ -49,9 +49,9 @@ public void testSceneNameSanitizing_2() { */ @Test public void testSceneNameSanitizing_3() { - assertEquals("xyzabc", AsynchronousSceneManager.sanitizedSceneName("xyz\u007fabc")); - assertEquals("12", AsynchronousSceneManager.sanitizedSceneName("1\u009f2")); - assertEquals("AZ", AsynchronousSceneManager.sanitizedSceneName("A\u0000\u0001\u001fZ")); + assertEquals("xyzabc", SceneUtils.sanitizedSceneName("xyz\u007fabc")); + assertEquals("12", SceneUtils.sanitizedSceneName("1\u009f2")); + assertEquals("AZ", SceneUtils.sanitizedSceneName("A\u0000\u0001\u001fZ")); } /** @@ -59,12 +59,12 @@ public void testSceneNameSanitizing_3() { */ @Test public void testSceneNameSanitizing_4() { - assertEquals("Scene", AsynchronousSceneManager.sanitizedSceneName("")); - assertEquals("Scene", AsynchronousSceneManager.sanitizedSceneName(" ")); - assertEquals("Scene", AsynchronousSceneManager.sanitizedSceneName("\t\n\b\r")); - assertEquals("Scene", AsynchronousSceneManager.sanitizedSceneName("\u0080")); + assertEquals("Scene", SceneUtils.sanitizedSceneName("")); + assertEquals("Scene", SceneUtils.sanitizedSceneName(" ")); + assertEquals("Scene", SceneUtils.sanitizedSceneName("\t\n\b\r")); + assertEquals("Scene", SceneUtils.sanitizedSceneName("\u0080")); // test that whitespace is stripped after removing control chars - assertEquals("Scene", AsynchronousSceneManager.sanitizedSceneName("\u0085 \u0085")); + assertEquals("Scene", SceneUtils.sanitizedSceneName("\u0085 \u0085")); } }