Merge pull request #334 from JLLeitschuh/fix/windowsShutdownHandling

Fix windows hanging when being shutdown

Author: ThomasJClark

core/src/main/java/edu/wpi/grip/core/events/UnexpectedThrowableEvent.java

@@ -1,5 +1,7 @@
 package edu.wpi.grip.core.events;
 
+import edu.wpi.grip.core.util.SafeShutdown;
+
 import java.util.logging.Level;
 import java.util.logging.Logger;
 
@@ -50,7 +52,7 @@ public void handleSafely(UnexpectedThrowableEventHandler handler) {
             try {
                 logger.log(Level.SEVERE, "Failed to handle safely", throwable);
             } finally {
-                System.exit(1);
+                SafeShutdown.exit(1);
             }
         } finally {
             shutdownIfFatal();
@@ -67,7 +69,7 @@ public void shutdownIfFatal() {
                 logger.log(Level.SEVERE, "Shutting down from error", throwable);
             } finally {
                 // If all else fails then shutdown
-                System.exit(1);
+                SafeShutdown.exit(1);
             }
         }
     }

core/src/main/java/edu/wpi/grip/core/util/SafeShutdown.java

@@ -0,0 +1,84 @@
+package edu.wpi.grip.core.util;
+
+/**
+ * This class should be used to shutdown GRIP safely.
+ * This is because shutdown hooks may throw exceptions, as such
+ * we need to know if the application being is shutdown.
+ */
+public final class SafeShutdown {
+
+    public static volatile boolean stopping = false;
+
+    static {
+        /*
+         * Shutdown hook run order is non-deterministic but this increases our likelihood of flagging a shutdown
+         * that we can't control.
+         */
+        Runtime.getRuntime().addShutdownHook(new Thread() {
+            @Override
+            public void run() {
+                stopping = true;
+            }
+        });
+    }
+
+    /**
+     * Shutdown's the VM in such a way that flags that the vm is stopping.
+     * This is so that we don't run the normal exception handling code when
+     * shutting down the application.
+     *
+     * @param statusCode exit status.
+     * @param hook       The hook to run before the System shutdown.
+     *                   This will be run after stopping has been flagged true.
+     *                   This is nullable.
+     * @see System#exit(int)
+     */
+    public static final void exit(int statusCode, PreSystemExitHook hook) {
+        flagStopping();
+        try {
+            if (hook != null) {
+                hook.run();
+            }
+        } finally {
+            System.exit(statusCode);
+        }
+
+    }
+
+    /**
+     * Helper method that passes null as the PreSystemExitHook.
+     *
+     * @param statusCode exit status.
+     * @see #exit(int)
+     */
+    public static final void exit(int statusCode) {
+        exit(statusCode, null);
+    }
+
+
+    /**
+     * HACK!
+     * Shutdown hooks can throw exceptions.
+     * On Windows, the static method after {@link org.bytedeco.javacpp.Loader#loadLibrary} throws such an exception
+     * in a shutdown hook.
+     *
+     * @return True if if the application is shutting down.
+     * @see <a href="https://github.com/WPIRoboticsProjects/GRIP/issues/297">GRIP Issue</a>
+     * @see <a href="https://github.com/bytedeco/javacpp/issues/60">Bytedeco issue</a>
+     */
+    public static final boolean isStopping() {
+        return stopping;
+    }
+
+    /**
+     * Flags that the application is shutting down.
+     */
+    public static void flagStopping() {
+        stopping = true;
+    }
+
+    @FunctionalInterface
+    public interface PreSystemExitHook {
+        void run();
+    }
+}

ui/src/main/java/edu/wpi/grip/ui/Main.java

@@ -9,6 +9,7 @@
 import edu.wpi.grip.core.Palette;
 import edu.wpi.grip.core.events.UnexpectedThrowableEvent;
 import edu.wpi.grip.core.operations.Operations;
+import edu.wpi.grip.core.util.SafeShutdown;
 import edu.wpi.grip.generated.CVOperations;
 import edu.wpi.grip.ui.util.DPIUtility;
 import javafx.application.Application;
@@ -32,9 +33,6 @@ public class Main extends Application {
     @Inject
     private Logger logger;
 
-    private volatile boolean stopping = false;
-
-
     protected final Injector injector = Guice.createInjector(new GRIPCoreModule(), new GRIPUIModule());
 
     private final Object dialogLock = new Object();
@@ -62,8 +60,9 @@ public void start(Stage stage) throws Exception {
         CVOperations.addOperations(eventBus);
 
         stage.setOnCloseRequest((event) -> {
-            Platform.exit();
-            System.exit(0);
+            // If this isn't here this can cause a deadlock on windows
+            // See issue #297
+            SafeShutdown.exit(0, Platform::exit);
         });
 
         stage.setTitle("GRIP Computer Vision Engine");
@@ -74,30 +73,35 @@ public void start(Stage stage) throws Exception {
     }
 
     public void stop() {
-        stopping = true;
+        SafeShutdown.flagStopping();
     }
 
     @Subscribe
     @SuppressWarnings("PMD.AvoidCatchingThrowable")
     public final void onUnexpectedThrowableEvent(UnexpectedThrowableEvent event) {
         event.handleSafely((throwable, message, isFatal) -> {
-            // This should still use PlatformImpl
-            if (!stopping) {
+            // Check this so we can avoid entering the the platform wait
+            // if the program is shutting down.
+            if (!SafeShutdown.isStopping()) {
+                // This should still use PlatformImpl
                 PlatformImpl.runAndWait(() -> {
                     // WARNING! Do not post any events from within this! It could result in a deadlock!
                     synchronized (this.dialogLock) {
-                        try {
-                            // Don't create more than one exception dialog at the same time
-                            final ExceptionAlert exceptionAlert = new ExceptionAlert(root, throwable, message, isFatal, getHostServices());
-                            exceptionAlert.setInitialFocus();
-                            exceptionAlert.showAndWait();
-                        } catch (Throwable e) {
-                            // Well in this case something has gone very, very wrong
-                            // We don't want to create a feedback loop either.
+                        // Check again because the value could have been changed while waiting for the javafx thread to run.
+                        if (!SafeShutdown.isStopping()) {
                             try {
-                                logger.log(Level.SEVERE, "Failed to show exception alert", e);
-                            } finally {
-                                System.exit(1); // Ensure we shut down the application if we get an exception
+                                // Don't create more than one exception dialog at the same time
+                                final ExceptionAlert exceptionAlert = new ExceptionAlert(root, throwable, message, isFatal, getHostServices());
+                                exceptionAlert.setInitialFocus();
+                                exceptionAlert.showAndWait();
+                            } catch (Throwable e) {
+                                // Well in this case something has gone very, very wrong
+                                // We don't want to create a feedback loop either.
+                                try {
+                                    logger.log(Level.SEVERE, "Failed to show exception alert", e);
+                                } finally {
+                                    SafeShutdown.exit(1); // Ensure we shut down the application if we get an exception
+                                }
                             }
                         }
                     }