From f1e40d5a8d10fa75445255366a7d5743971b86a7 Mon Sep 17 00:00:00 2001 From: Jonathan Leitschuh Date: Sun, 3 Jan 2016 17:17:30 -0500 Subject: [PATCH] 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 --- .../edu/wpi/grip/core/GRIPCoreModule.java | 18 ++++++- .../edu/wpi/grip/core/StartStoppable.java | 2 +- .../src/main/java/edu/wpi/grip/core/Step.java | 4 +- .../wpi/grip/core/sources/CameraSource.java | 9 ++-- .../wpi/grip/core/util/ExceptionWitness.java | 8 ++- ui/src/main/java/edu/wpi/grip/ui/Main.java | 49 +++++++++++-------- .../ui/components/StartStoppableButton.java | 3 ++ .../edu/wpi/grip/ui/util/GRIPPlatform.java | 20 +++++--- .../wpi/grip/ui/util/GRIPPlatformTest.java | 12 +++++ 9 files changed, 88 insertions(+), 37 deletions(-) diff --git a/core/src/main/java/edu/wpi/grip/core/GRIPCoreModule.java b/core/src/main/java/edu/wpi/grip/core/GRIPCoreModule.java index 36b174884d..13f20d02e5 100644 --- a/core/src/main/java/edu/wpi/grip/core/GRIPCoreModule.java +++ b/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 void hear(TypeLiteral type, TypeEncounter 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")); + } } } diff --git a/core/src/main/java/edu/wpi/grip/core/StartStoppable.java b/core/src/main/java/edu/wpi/grip/core/StartStoppable.java index e51c67a5d7..a99630d932 100644 --- a/core/src/main/java/edu/wpi/grip/core/StartStoppable.java +++ b/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 diff --git a/core/src/main/java/edu/wpi/grip/core/Step.java b/core/src/main/java/edu/wpi/grip/core/Step.java index c6b58f5a8c..0843f40d2a 100644 --- a/core/src/main/java/edu/wpi/grip/core/Step.java +++ b/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); diff --git a/core/src/main/java/edu/wpi/grip/core/sources/CameraSource.java b/core/src/main/java/edu/wpi/grip/core/sources/CameraSource.java index 7d3fc77739..7413da1be8 100644 --- a/core/src/main/java/edu/wpi/grip/core/sources/CameraSource.java +++ b/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(); diff --git a/core/src/main/java/edu/wpi/grip/core/util/ExceptionWitness.java b/core/src/main/java/edu/wpi/grip/core/util/ExceptionWitness.java index e6d0c311e5..4c4c13554d 100644 --- a/core/src/main/java/edu/wpi/grip/core/util/ExceptionWitness.java +++ b/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)); } diff --git a/ui/src/main/java/edu/wpi/grip/ui/Main.java b/ui/src/main/java/edu/wpi/grip/ui/Main.java index f7369925c0..2832acacd5 100644 --- a/ui/src/main/java/edu/wpi/grip/ui/Main.java +++ b/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); + } } } } diff --git a/ui/src/main/java/edu/wpi/grip/ui/components/StartStoppableButton.java b/ui/src/main/java/edu/wpi/grip/ui/components/StartStoppableButton.java index 3a0d99de91..88d7668092 100644 --- a/ui/src/main/java/edu/wpi/grip/ui/components/StartStoppableButton.java +++ b/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(); } }); diff --git a/ui/src/main/java/edu/wpi/grip/ui/util/GRIPPlatform.java b/ui/src/main/java/edu/wpi/grip/ui/util/GRIPPlatform.java index 4e6bbbf28c..a661320bbb 100644 --- a/ui/src/main/java/edu/wpi/grip/ui/util/GRIPPlatform.java +++ b/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!"); } } diff --git a/ui/src/test/java/edu/wpi/grip/ui/util/GRIPPlatformTest.java b/ui/src/test/java/edu/wpi/grip/ui/util/GRIPPlatformTest.java index cc55439861..ba9d99f0cb 100644 --- a/ui/src/test/java/edu/wpi/grip/ui/util/GRIPPlatformTest.java +++ b/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]); + } + } \ No newline at end of file