Merge pull request #240 from JLLeitschuh/fix/interruptedDeadlock

Fixes Handling of InterruptedExceptions

Author: ThomasJClark

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]);
+    }
+
 }