diff --git a/chunky/src/java/se/llbit/chunky/main/Chunky.java b/chunky/src/java/se/llbit/chunky/main/Chunky.java index 13a696535..4b69de74d 100644 --- a/chunky/src/java/se/llbit/chunky/main/Chunky.java +++ b/chunky/src/java/se/llbit/chunky/main/Chunky.java @@ -48,6 +48,7 @@ import se.llbit.log.Log; import se.llbit.log.Receiver; import se.llbit.util.TaskTracker; +import se.llbit.util.concurrent.ChunkyThread; import java.io.File; import java.io.FileInputStream; @@ -56,6 +57,7 @@ import java.nio.file.Path; import java.util.*; import java.util.concurrent.ForkJoinPool; +import java.util.concurrent.TimeUnit; import java.util.stream.Collectors; /** @@ -211,9 +213,8 @@ public static void main(final String[] args) { System.exit(1); } - int exitCode = 0; if (cmdline.mode == CommandLineOptions.Mode.CLI_OPERATION) { - exitCode = cmdline.exitCode; + System.exit(cmdline.exitCode); } else { // Initialize the common thread pool. getCommonThreads(); @@ -222,6 +223,14 @@ public static void main(final String[] args) { chunky.headless = cmdline.mode == Mode.HEADLESS_RENDER || cmdline.mode == Mode.CREATE_SNAPSHOT; chunky.loadPlugins(); + Runtime.getRuntime().addShutdownHook( + new Thread(() -> { // intentionally not a ChunkyThread + // Within a shutdown hook we need to close quickly, otherwise we risk the user/OS escalating to KILL + chunky.shutdown(1, TimeUnit.SECONDS); + }) + ); + + int exitCode = 0; try { switch (cmdline.mode) { case HEADLESS_RENDER: @@ -240,12 +249,19 @@ public static void main(final String[] args) { Log.error("Unchecked exception caused Chunky to close.", t); exitCode = 2; } - } - if (exitCode != 0) { + chunky.shutdown(5, TimeUnit.SECONDS); + // Always exit, we've done all shutdown necessary and want to exit whether non-daemon threads exist or not. + // This should prevent hangs if threads aren't cooperating. System.exit(exitCode); } } + private void shutdown(int timeout, TimeUnit unit) { + if (!ChunkyThread.interruptAndJoinAll(timeout, unit)) { + Log.error("Not all threads were joined before shutting down."); // FIXME: list all alive threads? ThreadGroups are annoying. + } + } + /** * This can be used by plugins to load the default Minecraft textures. */ @@ -342,7 +358,7 @@ public void update() { public static ForkJoinPool getCommonThreads() { if (commonThreads == null) { // use at least two threads to prevent deadlocks in some java versions (see #1631) - commonThreads = new ForkJoinPool(Math.max(PersistentSettings.getNumThreads(), 2)); + commonThreads = ChunkyThread.addForkJoinPool(new ForkJoinPool(Math.max(PersistentSettings.getNumThreads(), 2))); } return commonThreads; } @@ -353,7 +369,7 @@ public static ForkJoinPool getCommonThreads() { */ public static void setCommonThreadsCount(int threads) { ForkJoinPool t = getCommonThreads(); - commonThreads = new ForkJoinPool(threads); + commonThreads = ChunkyThread.addForkJoinPool(new ForkJoinPool(threads)); t.shutdown(); } diff --git a/chunky/src/java/se/llbit/chunky/renderer/DefaultRenderManager.java b/chunky/src/java/se/llbit/chunky/renderer/DefaultRenderManager.java index 9b091c2e4..6074c3577 100644 --- a/chunky/src/java/se/llbit/chunky/renderer/DefaultRenderManager.java +++ b/chunky/src/java/se/llbit/chunky/renderer/DefaultRenderManager.java @@ -28,6 +28,7 @@ import se.llbit.chunky.resources.BitmapImage; import se.llbit.log.Log; import se.llbit.math.ColorUtil; +import se.llbit.util.concurrent.ChunkyThread; import se.llbit.util.TaskTracker; import java.time.Duration; @@ -52,7 +53,7 @@ *

All available final renderers are stored in {@code renderers} and preview renderers * are stored in {@code previewRenderers}. */ -public class DefaultRenderManager extends Thread implements RenderManager { +public class DefaultRenderManager extends ChunkyThread implements RenderManager { /** * Map containing all the final render {@code Renderer}s. The renderer corresponding to * {@code getRendererName()} is used when a render is requested. diff --git a/chunky/src/java/se/llbit/chunky/renderer/RenderWorkerPool.java b/chunky/src/java/se/llbit/chunky/renderer/RenderWorkerPool.java index 51b129d5d..80cef5ea4 100644 --- a/chunky/src/java/se/llbit/chunky/renderer/RenderWorkerPool.java +++ b/chunky/src/java/se/llbit/chunky/renderer/RenderWorkerPool.java @@ -18,6 +18,7 @@ package se.llbit.chunky.renderer; import se.llbit.log.Log; +import se.llbit.util.concurrent.ChunkyThread; import java.util.ArrayList; import java.util.Random; @@ -39,7 +40,7 @@ public interface Factory { RenderWorkerPool create(int threads, long seed); } - public static class RenderWorker extends Thread { + public static class RenderWorker extends ChunkyThread { private final RenderWorkerPool pool; public final Random random; 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 919a4f4c2..7d0878026 100644 --- a/chunky/src/java/se/llbit/chunky/renderer/scene/AsynchronousSceneManager.java +++ b/chunky/src/java/se/llbit/chunky/renderer/scene/AsynchronousSceneManager.java @@ -26,6 +26,7 @@ import se.llbit.chunky.world.RegionPosition; import se.llbit.chunky.world.World; import se.llbit.log.Log; +import se.llbit.util.concurrent.ChunkyThread; import se.llbit.util.TaskTracker; import java.io.File; @@ -41,7 +42,7 @@ * * @author Jesper Öqvist */ -public class AsynchronousSceneManager extends Thread implements SceneManager { +public class AsynchronousSceneManager extends ChunkyThread implements SceneManager { private final SynchronousSceneManager sceneManager; private final LinkedBlockingQueue taskQueue; 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 bc4bc6bf6..82d0f3f21 100644 --- a/chunky/src/java/se/llbit/chunky/renderer/scene/Scene.java +++ b/chunky/src/java/se/llbit/chunky/renderer/scene/Scene.java @@ -62,6 +62,7 @@ import se.llbit.nbt.Tag; import se.llbit.util.*; import se.llbit.util.annotation.NotNull; +import se.llbit.util.concurrent.ChunkyThread; import se.llbit.util.io.PositionalInputStream; import se.llbit.util.io.ZipExport; import se.llbit.util.mojangapi.MinecraftProfile; @@ -858,7 +859,7 @@ public synchronized void loadChunks(TaskTracker taskTracker, World world, Map {}; - private ScheduledExecutorService executor = Executors.newSingleThreadScheduledExecutor(); + private final ScheduledExecutorService executor = ChunkyThread.addExecutorService(Executors::newSingleThreadScheduledExecutor); private boolean shouldDrawPlayers = true; diff --git a/chunky/src/java/se/llbit/chunky/ui/ChunkyFx.java b/chunky/src/java/se/llbit/chunky/ui/ChunkyFx.java index 93f6532ac..6226d4c83 100644 --- a/chunky/src/java/se/llbit/chunky/ui/ChunkyFx.java +++ b/chunky/src/java/se/llbit/chunky/ui/ChunkyFx.java @@ -29,6 +29,8 @@ import se.llbit.chunky.resources.SettingsDirectory; import se.llbit.chunky.ui.controller.ChunkyFxController; import se.llbit.fxutil.WindowPosition; +import se.llbit.log.ConsoleReceiver; +import se.llbit.log.Level; import se.llbit.log.Log; import java.io.File; @@ -79,10 +81,11 @@ public class ChunkyFx extends Application { @Override public void stop() throws Exception { + // UI is closing so we need to replace the receivers + Log.setReceiver(ConsoleReceiver.INSTANCE, Level.INFO, Level.WARNING, Level.ERROR); if(mainStage != null) { PersistentSettings.setWindowPosition(new WindowPosition(mainStage)); } - System.exit(0); } public static void startChunkyUI(Chunky chunkyInstance) { diff --git a/chunky/src/java/se/llbit/chunky/ui/controller/ResourcePackChooserController.java b/chunky/src/java/se/llbit/chunky/ui/controller/ResourcePackChooserController.java index dcac32703..251fdff9d 100644 --- a/chunky/src/java/se/llbit/chunky/ui/controller/ResourcePackChooserController.java +++ b/chunky/src/java/se/llbit/chunky/ui/controller/ResourcePackChooserController.java @@ -49,6 +49,7 @@ import se.llbit.json.JsonParser; import se.llbit.log.Log; import se.llbit.util.MinecraftText; +import se.llbit.util.concurrent.ChunkyThread; import java.awt.*; import java.io.File; @@ -451,7 +452,7 @@ public void populate( private static class PackListItem { - private final static Executor PACK_PARSER_EXECUTOR = Executors.newSingleThreadExecutor(); + private final static Executor PACK_PARSER_EXECUTOR = ChunkyThread.addExecutorService(Executors::newSingleThreadExecutor); private static PackListItem DEFAULT = null; diff --git a/chunky/src/java/se/llbit/chunky/ui/controller/SceneChooserController.java b/chunky/src/java/se/llbit/chunky/ui/controller/SceneChooserController.java index 534926aea..5c507b056 100644 --- a/chunky/src/java/se/llbit/chunky/ui/controller/SceneChooserController.java +++ b/chunky/src/java/se/llbit/chunky/ui/controller/SceneChooserController.java @@ -36,6 +36,7 @@ import se.llbit.json.JsonObject; import se.llbit.json.JsonParser; import se.llbit.log.Log; +import se.llbit.util.concurrent.ChunkyThread; import java.io.File; import java.io.FileInputStream; @@ -64,6 +65,8 @@ public class SceneChooserController implements Initializable { private Stage stage; + private static final Executor loadExecutor = ChunkyThread.addExecutorService(Executors::newSingleThreadExecutor); + private ChunkyFxController controller; private static final HashMap sceneListCache = new HashMap<>(); @@ -219,7 +222,6 @@ public void setStage(Stage stage) { private void populateSceneTable(File sceneDir) { this.sceneTbl.setPlaceholder(new Label("Loading scenes…")); - Executor loadExecutor = Executors.newSingleThreadExecutor(); loadExecutor.execute(() -> { List scenes = new ArrayList<>(); diff --git a/chunky/src/java/se/llbit/chunky/world/ChunkTopographyUpdater.java b/chunky/src/java/se/llbit/chunky/world/ChunkTopographyUpdater.java index 8c61d5b59..58ef6be99 100644 --- a/chunky/src/java/se/llbit/chunky/world/ChunkTopographyUpdater.java +++ b/chunky/src/java/se/llbit/chunky/world/ChunkTopographyUpdater.java @@ -16,6 +16,8 @@ */ package se.llbit.chunky.world; +import se.llbit.util.concurrent.ChunkyThread; + import java.util.HashSet; import java.util.Iterator; import java.util.Set; @@ -25,7 +27,7 @@ * * @author Jesper Öqvist (jesper@llbit.se) */ -public class ChunkTopographyUpdater extends Thread { +public class ChunkTopographyUpdater extends ChunkyThread { private final Set queue = new HashSet<>(); diff --git a/chunky/src/java/se/llbit/chunky/world/SkymapTexture.java b/chunky/src/java/se/llbit/chunky/world/SkymapTexture.java index 4b2500a90..d879dc157 100644 --- a/chunky/src/java/se/llbit/chunky/world/SkymapTexture.java +++ b/chunky/src/java/se/llbit/chunky/world/SkymapTexture.java @@ -25,6 +25,7 @@ import se.llbit.math.QuickMath; import se.llbit.math.Ray; import se.llbit.math.Vector4; +import se.llbit.util.concurrent.ChunkyThread; import se.llbit.util.ImageTools; /** @@ -35,7 +36,7 @@ */ public class SkymapTexture extends Texture { - class TexturePreprocessor extends Thread { + class TexturePreprocessor extends ChunkyThread { private final int x0; private final int x1; private final int y0; diff --git a/chunky/src/java/se/llbit/chunky/world/region/RegionChangeWatcher.java b/chunky/src/java/se/llbit/chunky/world/region/RegionChangeWatcher.java index 01ab9ed1b..bdc35cd96 100644 --- a/chunky/src/java/se/llbit/chunky/world/region/RegionChangeWatcher.java +++ b/chunky/src/java/se/llbit/chunky/world/region/RegionChangeWatcher.java @@ -20,13 +20,14 @@ import se.llbit.chunky.map.WorldMapLoader; import se.llbit.chunky.renderer.ChunkViewListener; import se.llbit.chunky.world.ChunkView; +import se.llbit.util.concurrent.ChunkyThread; /** * Monitors filesystem for changes to region files. * * @author Jesper Öqvist */ -public abstract class RegionChangeWatcher extends Thread implements ChunkViewListener { +public abstract class RegionChangeWatcher extends ChunkyThread implements ChunkViewListener { protected final WorldMapLoader mapLoader; protected final MapView mapView; protected volatile ChunkView view = ChunkView.EMPTY; diff --git a/chunky/src/java/se/llbit/chunky/world/region/RegionParser.java b/chunky/src/java/se/llbit/chunky/world/region/RegionParser.java index b25f0771c..841851a51 100644 --- a/chunky/src/java/se/llbit/chunky/world/region/RegionParser.java +++ b/chunky/src/java/se/llbit/chunky/world/region/RegionParser.java @@ -23,6 +23,7 @@ import se.llbit.chunky.map.WorldMapLoader; import se.llbit.chunky.world.*; import se.llbit.log.Log; +import se.llbit.util.concurrent.ChunkyThread; import se.llbit.util.Mutable; /** @@ -34,7 +35,7 @@ * * @author Jesper Öqvist (jesper@llbit.se) */ -public class RegionParser extends Thread { +public class RegionParser extends ChunkyThread { private final WorldMapLoader mapLoader; private final RegionQueue queue; diff --git a/chunky/src/java/se/llbit/util/concurrent/ChunkyThread.java b/chunky/src/java/se/llbit/util/concurrent/ChunkyThread.java new file mode 100644 index 000000000..51d4631f3 --- /dev/null +++ b/chunky/src/java/se/llbit/util/concurrent/ChunkyThread.java @@ -0,0 +1,249 @@ +package se.llbit.util.concurrent; + +import se.llbit.chunky.main.Chunky; +import se.llbit.chunky.plugin.PluginApi; +import se.llbit.util.annotation.NotNull; + +import java.util.ArrayList; +import java.util.Collection; +import java.util.concurrent.*; +import java.util.function.Function; + +/** + * {@link Thread}/{@link ExecutorService} resource management. + *

The goal of this class is to primarily:

+ * + * + *

Usage

+ * + * + */ +public class ChunkyThread extends Thread { + private static final CountDownLatch shutdownLatch = new CountDownLatch(1); + private static final Collection threads = new ArrayList<>(); + private static final Collection executorServices = new ArrayList<>(); + + /** + * Add a {@link Thread} to be interrupted and joined by chunky on shutdown + * + * @throws IllegalStateException When calling after {@link #interruptAndJoinAll(long, TimeUnit)} has been called. + */ + @PluginApi + public synchronized static T addThread(T thread) { + if (shutdownLatch.getCount() == 0) { + throw new IllegalStateException("Creating a thread as chunky is stopping."); + } + threads.add(thread); + return thread; + } + + /** + * Add an {@link ExecutorService} to be interrupted and joined by chunky on shutdown + * + * @throws IllegalStateException When calling after {@link #interruptAndJoinAll(long, TimeUnit)} has been called. + */ + @PluginApi + public synchronized static E addExecutorService(Function executorServiceSupplier) { + if (shutdownLatch.getCount() == 0) { + throw new IllegalStateException("Creating an executor service as chunky is stopping."); + } + E e = executorServiceSupplier.apply(r -> { // executor shutdown interrupts its own threads, so they don't need to be ChunkyThreads + Thread t = new Thread(r); + t.setDaemon(true); + return t; + }); + executorServices.add(e); + return e; + } + + /** + * Add a {@link ForkJoinPool} to be interrupted and joined by chunky on shutdown + * + * @throws IllegalStateException When calling after {@link #interruptAndJoinAll(long, TimeUnit)} has been called. + */ + @PluginApi + public synchronized static ForkJoinPool addForkJoinPool(ForkJoinPool pool) { + if (shutdownLatch.getCount() == 0) { + throw new IllegalStateException("Creating a fork join pool as chunky is stopping."); + } + executorServices.add(pool); + return pool; + } + + /** + * Await the joining of all threads managed by chunky. This method will wait indefinitely until a + * shutdown happens to begin its timeout. + * + *

This method is always safe to call.

+ * + *

WARNING: calling this from any thread registered with {@link #addThread(Thread)} may deadlock.

+ * + * @param timeout The maximum time to wait AFTER a shutdown is initiated + * @param unit the time unit of the timeout argument + * + * @return Whether all threads were joined before returning + */ + public static boolean joinAll(long timeout, @NotNull TimeUnit unit) { + /* + * This method should not be synchronized because: + * 1. Calls to this method that happen before interruptAndJoinAll will lock the latter interrupting thread, deadlocking. + * 2. shutdownLatch.await is at least acquire memory ordering, and modification is disabled after the latch is zero. + * As such we are guaranteed that no threads can modify the state. + */ + boolean interrupted = false; + + while (true) { + try { + // Must wait for the latch as hitting the for loop below first causes immediate evaluation of: + // - The for loop iterator, potentially missing new threads. + // - The end time, meaning waiting starts before shutdown begins. + shutdownLatch.await(); + break; + } catch (InterruptedException e) { + interrupted = true; + } + } + + long endTime = System.nanoTime() + unit.toNanos(timeout); + try { + while (System.nanoTime() < endTime) { + try { + if (joinAllInterruptable(endTime)) { + // All threads are joined, skip the rest of the wait time. + return true; + } + } catch (InterruptedException e) { + interrupted = true; + } + } + // Got to the end of the wait time without joining everything, can give no guarantees + return false; + } finally { + if (interrupted) { + Thread.currentThread().interrupt(); + } + } + } + + /** + * Interrupt and then await joining of all threads managed by chunky. + * + *

WARNING: calling this from any thread registered with {@link #addThread(Thread)} may deadlock.

+ *

Only to be called by {@link Chunky}

+ * + * @param timeout The maximum time to wait + * @param unit the time unit of the timeout argument + * @return Whether all threads were joined before the time limit was reached + */ + public static boolean interruptAndJoinAll(long timeout, @NotNull TimeUnit unit) { + synchronized (ChunkyThread.class) { + shutdownLatch.countDown(); + + for (ExecutorService executorService : executorServices) { + executorService.shutdownNow(); + } + for (Thread thread : ChunkyThread.threads) { + thread.interrupt(); + } + } + + return joinAll(timeout, unit); + } + + /** + * Await the joining of all threads managed by chunky. + * + *

This method is only safe to call if the {@link ChunkyThread#shutdownLatch} has been set.

+ * + *

WARNING: calling this from any thread registered with {@link #addThread(Thread)} may deadlock.

+ * + * @param endTimeNanos The time at which to stop waiting. + * + * @return Whether all threads were joined before returning + * + * @throws InterruptedException Propagates up when interrupted. The caller has no guarantee that shutdown has begun, + * or that any of the inner threads have been joined. + */ + private static boolean joinAllInterruptable(long endTimeNanos) throws InterruptedException { + // The intention here whether we return true or false, is to give the caller the most complete acquire load possible. + // Even if we reach the timeout given by the caller, we still establish a happens-before with every dead thread. + + boolean anyAlive = false; + for (ExecutorService executorService : executorServices) { + long waitTime = endTimeNanos - System.nanoTime(); + anyAlive |= !executorService.awaitTermination(waitTime, TimeUnit.NANOSECONDS); + } + for (Thread thread : threads) { + long waitTime = endTimeNanos - System.nanoTime(); + if (waitTime > 0) { + thread.join(waitTime); // joining with 0 is infinite wait time, very intuitive. + } + // Thread.isAlive() establishes a happens-before with the thread. As such the following are non-issues: + // - Not joining the thread, if waitTime <= 0 + // - The thread stopping between Thread.join() and Thread.isAlive(). + anyAlive |= thread.isAlive(); + } + + return !anyAlive; + } + + private void setDefaults() { + this.setDaemon(true); + addThread(this); + } + + /* Constructors from super */ + public ChunkyThread() { + super(); + setDefaults(); + } + + public ChunkyThread(Runnable task) { + super(task); + setDefaults(); + } + + public ChunkyThread(ThreadGroup group, Runnable task) { + super(group, task); + setDefaults(); + } + + public ChunkyThread(String name) { + super(name); + setDefaults(); + } + + public ChunkyThread(ThreadGroup group, String name) { + super(group, name); + setDefaults(); + } + + public ChunkyThread(Runnable task, String name) { + super(task, name); + setDefaults(); + } + + public ChunkyThread(ThreadGroup group, Runnable task, String name) { + super(group, task, name); + setDefaults(); + } + + public ChunkyThread(ThreadGroup group, Runnable task, String name, long stackSize) { + super(group, task, name, stackSize); + setDefaults(); + } + + public ChunkyThread(ThreadGroup group, Runnable task, String name, long stackSize, boolean inheritInheritableThreadLocals) { + super(group, task, name, stackSize, inheritInheritableThreadLocals); + setDefaults(); + } +}