Fixes Handling of InterruptedExceptions

Before we treated InterruptedExceptions just like every other
type of exception. This is bad because it means that the thread
that had been asked to exit was having to go through the process
of displaying a UI component which was causing deadlocks.

Closes #236

Author: JLLeitschuh

core/src/main/java/edu/wpi/grip/core/GRIPCoreModule.java

@@ -16,11 +16,15 @@
 import edu.wpi.grip.core.sources.MultiImageFileSource;
 import edu.wpi.grip.core.util.ExceptionWitness;
 
+import java.util.logging.Level;
+import java.util.logging.Logger;
+
 /**
  * A Guice {@link com.google.inject.Module} for GRIP's core package.  This is where instances of {@link Pipeline},
  * {@link Palette}, {@link Project}, etc... are created.
  */
 public class GRIPCoreModule extends AbstractModule {
+    private final Logger logger = Logger.getLogger(GRIPCoreModule.class.getName());
 
     private final EventBus eventBus = new EventBus(this::onSubscriberException);
 
@@ -60,10 +64,20 @@ public <I> void hear(TypeLiteral<I> type, TypeEncounter<I> encounter) {
     }
 
     private void onSubscriberException(Throwable exception, SubscriberExceptionContext context) {
-        eventBus.post(new UnexpectedThrowableEvent(exception, "An event subscriber threw an exception"));
+        if (exception instanceof InterruptedException) {
+            logger.log(Level.FINE, "EventBus Subscriber threw InterruptedException", exception);
+            Thread.currentThread().interrupt();
+        } else {
+            eventBus.post(new UnexpectedThrowableEvent(exception, "An event subscriber threw an exception"));
+        }
     }
 
     private void onThreadException(Thread thread, Throwable exception) {
-        eventBus.post(new UnexpectedThrowableEvent(exception, thread + " threw an exception"));
+        if (exception instanceof InterruptedException) {
+            logger.log(Level.FINE, "InterruptedException from thread " + thread, exception);
+            Thread.currentThread().interrupt();
+        } else {
+            eventBus.post(new UnexpectedThrowableEvent(exception, thread + " threw an exception"));
+        }
     }
 }

core/src/main/java/edu/wpi/grip/core/StartStoppable.java

@@ -26,7 +26,7 @@ public interface StartStoppable {
      * @throws TimeoutException If the thread fails to stop in a timely manner
      * @throws IOException      If cleaning up some system resource fails.
      */
-    void stop() throws TimeoutException, IOException;
+    void stop() throws InterruptedException, TimeoutException, IOException;
 
     /**
      * Used to indicate if the source is running or stopped

core/src/main/java/edu/wpi/grip/core/Step.java

@@ -143,7 +143,9 @@ private synchronized void runPerformIfPossible() {
 
         try {
             this.operation.perform(inputSockets, outputSockets, data);
-        } catch (Exception e) {
+        } catch (RuntimeException e) {
+            // We do not want to catch all exceptions, only runtime exceptions.
+            // This is especially important when it comes to InterruptedExceptions
             final String operationFailedMessage = "The " + operation.getName() + " operation did not perform correctly.";
             logger.log(Level.WARNING, operationFailedMessage, e);
             witness.flagException(e, operationFailedMessage);

core/src/main/java/edu/wpi/grip/core/sources/CameraSource.java

@@ -208,6 +208,8 @@ public void start() throws IOException, IllegalStateException {
                         } catch (TimeoutException | IOException e) {
                             // TODO: This should use the ExceptionWitness once that has a UI component added for it
                             eventBus.post(new UnexpectedThrowableEvent(e, "Camera Frame Grabber could not be stopped!"));
+                        } catch (InterruptedException e) {
+                            Thread.currentThread().interrupt();
                         }
                     }
             );
@@ -228,13 +230,13 @@ public void start() throws IOException, IllegalStateException {
      * @throws IOException           If there is a problem stopping the Source
      * @throws IllegalStateException If the camera is already stopped.
      */
-    public void stop() throws TimeoutException, IOException {
+    public void stop() throws InterruptedException, TimeoutException, IOException {
         synchronized (this) {
             if (frameThread.isPresent()) {
                 final Thread ex = frameThread.get();
                 ex.interrupt();
                 try {
-                    ex.join(TimeUnit.SECONDS.toMillis(500));
+                    ex.join(TimeUnit.SECONDS.toMillis(10));
                     if (ex.isAlive()) {
                         throw new TimeoutException("Unable to terminate video feed from Web Camera");
                     }
@@ -243,6 +245,7 @@ public void stop() throws TimeoutException, IOException {
                 } catch (InterruptedException e) {
                     Thread.currentThread().interrupt();
                     logger.log(Level.WARNING, e.getMessage(), e);
+                    throw e;
                 } finally {
                     // This will always run even if a timeout exception occurs
                     try {
@@ -266,7 +269,7 @@ public synchronized boolean isStarted() {
     }
 
     @Subscribe
-    public void onSourceRemovedEvent(SourceRemovedEvent event) throws TimeoutException, IOException {
+    public void onSourceRemovedEvent(SourceRemovedEvent event) throws InterruptedException, TimeoutException, IOException {
         if (event.getSource() == this) {
             try {
                 if (this.isStarted()) this.stop();

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

@@ -48,10 +48,16 @@ public interface Factory {
     /**
      * Indicates to the witness that an exception has occurred. This will also post an {@link ExceptionEvent} to the {@link EventBus}
      *
-     * @param exception The exception that this is reporting
+     * @param exception The exception that this is reporting.
+     *                  If the Exception is an InterruptedException then this will not post an exception, instead,
+     *                  it will set the threads interrupted state and return.
      * @param message   Any additional details that should be associated with this message.
      */
     public final void flagException(final Exception exception, final String message) {
+        if (exception instanceof InterruptedException) {
+            Thread.currentThread().interrupt();
+            return;
+        }
         isExceptionState.set(true);
         this.eventBus.post(new ExceptionEvent(origin, exception, message));
     }

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

@@ -92,28 +92,35 @@ public void start(Stage stage) throws Exception {
     public final void onUnexpectedThrowableEvent(UnexpectedThrowableEvent event) {
         // Print throwable before showing the exception so that errors are in order in the console
         event.getThrowable().printStackTrace();
-        // 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, event.getThrowable(), event.getMessage(), event.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.
-                    e.printStackTrace();
-                    System.exit(1); // Ensure we shut down the application if we get an exception
-                }
-            }
-        });
 
-        if (event.isFatal()) {
-            System.err.println("Original fatal exception");
-            event.getThrowable().printStackTrace();
-            System.exit(1);
+        try {
+            // If this is an interrupted exception then don't show an error.
+            // We should exit as fast as possible.
+            if (!(event.getThrowable() instanceof InterruptedException)) {
+                // 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, event.getThrowable(), event.getMessage(), event.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.
+                            e.printStackTrace();
+                            System.exit(1); // Ensure we shut down the application if we get an exception
+                        }
+                    }
+                });
+            }
+        } finally {
+            if (event.isFatal()) {
+                System.err.println("Original fatal exception");
+                event.getThrowable().printStackTrace();
+                System.exit(1);
+            }
         }
     }
 }

ui/src/main/java/edu/wpi/grip/ui/components/StartStoppableButton.java

@@ -64,6 +64,9 @@ public interface Factory {
                 // If this fails then an StartedStoppedEvent will not be posted
             } catch (TimeoutException | IOException e) {
                 eventBus.post(new UnexpectedThrowableEvent(e, "Failed to stop"));
+            } catch (InterruptedException e) {
+                // Unfortunately we have to eat the exception here because we can't rethrow it
+                Thread.currentThread().interrupt();
             }
         });
 

ui/src/main/java/edu/wpi/grip/ui/util/GRIPPlatform.java

@@ -49,6 +49,7 @@ public Runnable getAction() {
      * JavaFX application thread. If we are already on the JavaFX application thread
      * then this will be run immediately. Otherwise, it will be run as an event
      * inside of the event bus.
+     * If
      * If {@link #runAsSoonAsPossible(Runnable)} is called within itself it will always run
      * immediately because the runnable will always be run in the JavaFX thread.
      *
@@ -69,25 +70,28 @@ public void runAsSoonAsPossible(Runnable action) {
     }
 
     @Subscribe
-    public void onJavaFXRunnerEvent(JavaFXRunnerEvent event) {
+    public void onJavaFXRunnerEvent(JavaFXRunnerEvent event) throws InterruptedException {
         assert !Platform.isFxApplicationThread() : "This should never be run on the application thread. This can cause a deadlock!";
+        final Thread callingThread = Thread.currentThread();
+        if (Thread.interrupted()) {
+            throw new InterruptedException("Interrupted in onJavaFXRunnerEvent");
+        }
 
         // queue on JavaFX thread and wait for completion
         final CountDownLatch doneLatch = new CountDownLatch(1);
         Platform.runLater(() -> {
             try {
-                event.getAction().run();
+                // If the calling thread was interrupted then don't run this event.
+                if(!callingThread.isInterrupted()) {
+                    event.getAction().run();
+                }
             } finally {
                 doneLatch.countDown();
             }
         });
 
-        try {
-            while (!doneLatch.await(500, TimeUnit.MILLISECONDS)) {
-                logger.log(Level.WARNING, "POTENTIAL DEADLOCK!");
-            }
-        } catch (InterruptedException e) {
-            // ignore exception
+        while (!doneLatch.await(500, TimeUnit.MILLISECONDS)) {
+            logger.log(Level.WARNING, "POTENTIAL DEADLOCK!");
         }
     }
 

ui/src/test/java/edu/wpi/grip/ui/util/GRIPPlatformTest.java

@@ -14,6 +14,7 @@
 
 import java.util.logging.Logger;
 
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 
 public class GRIPPlatformTest extends ApplicationTest {
@@ -76,4 +77,15 @@ public void testRunAsSoonAsPossibleDoesNotDeadlockWhenRunInsideItself() throws E
         waiter.await();
     }
 
+    @Test
+    public void testRunAsSoonAsPossibleWillNotCallIfInterrupted() throws Exception {
+        final boolean hasRun [] = {false};
+
+        Thread.currentThread().interrupt();
+        platform.runAsSoonAsPossible(() -> {
+            hasRun[0] = true;
+        });
+        assertFalse("runAsSoonAsPossible ran when interrupted", hasRun[0]);
+    }
+
 }