From 86bf69b9e8e6958b1e38087f9b4b6b6e964242aa Mon Sep 17 00:00:00 2001 From: Randall Wood <297232+rhwood@users.noreply.github.com> Date: Sat, 23 May 2020 14:01:20 -0400 Subject: [PATCH] fix: actually, you know, work It turns out no tests were testing that the DefaultShutDownManager actually ran ShutDownTasks when shutting down --- java/src/jmri/ShutDownManager.java | 2 +- .../jmri/managers/DefaultShutDownManager.java | 86 ++++++---- .../managers/DefaultShutDownManagerTest.java | 159 ++++++++++++++++-- 3 files changed, 196 insertions(+), 51 deletions(-) diff --git a/java/src/jmri/ShutDownManager.java b/java/src/jmri/ShutDownManager.java index 7cce24a9419..4345e846dc2 100644 --- a/java/src/jmri/ShutDownManager.java +++ b/java/src/jmri/ShutDownManager.java @@ -71,7 +71,7 @@ public interface ShutDownManager extends PropertyChangeProvider { * * @param task the task not to call */ - public void deregister(@CheckForNull Callable task); + public void deregister(@CheckForNull Callable task); /** * Deregister a task. Attempts to deregister a task that is not diff --git a/java/src/jmri/managers/DefaultShutDownManager.java b/java/src/jmri/managers/DefaultShutDownManager.java index 8f8d947a817..3178bdde637 100644 --- a/java/src/jmri/managers/DefaultShutDownManager.java +++ b/java/src/jmri/managers/DefaultShutDownManager.java @@ -8,12 +8,17 @@ import java.util.*; import java.util.concurrent.Callable; -import java.util.concurrent.TimeUnit; +import java.util.stream.Collectors; + import jmri.ShutDownManager; import jmri.ShutDownTask; import jmri.beans.Bean; +import jmri.util.ThreadingUtil; + import org.openide.util.RequestProcessor; +import org.openide.util.Task; +import org.openide.util.TaskListener; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -49,13 +54,15 @@ public class DefaultShutDownManager extends Bean implements ShutDownManager { private static boolean shuttingDown = false; - private final static Logger log = LoggerFactory.getLogger(DefaultShutDownManager.class); + private static final Logger log = LoggerFactory.getLogger(DefaultShutDownManager.class); private final List tasks = new ArrayList<>(); private final Set> callables = new HashSet<>(); private final Set runnables = new HashSet<>(); protected final Thread shutdownHook; // use up to 8 threads for parallel tasks private static final RequestProcessor RP = new RequestProcessor("On Start/Stop", 8); // NOI18N + private static final String NO_NULL_TASK = "Shutdown task cannot be null."; // NOI18N + private static final String PROP_SHUTTING_DOWN = "shuttingDown"; // NOI18N /** * Create a new shutdown manager. @@ -68,10 +75,8 @@ public DefaultShutDownManager() { // calling System.exit() within a shutdown hook will cause the // application to hang. // This shutdown hook also allows OS X Application->Quit to trigger our - // shutdown tasks, since that simply calls System.exit(); - this.shutdownHook = jmri.util.ThreadingUtil.newThread(() -> { - DefaultShutDownManager.this.shutdown(0, false); - }); + // shutdown tasks, since that simply calls System.exit() + this.shutdownHook = ThreadingUtil.newThread(() -> DefaultShutDownManager.this.shutdown(0, false)); try { Runtime.getRuntime().addShutdownHook(this.shutdownHook); } catch (IllegalStateException ex) { @@ -83,8 +88,8 @@ public DefaultShutDownManager() { * {@inheritDoc} */ @Override - synchronized public void register(ShutDownTask s) { - Objects.requireNonNull(s, "Shutdown task cannot be null."); + public synchronized void register(ShutDownTask s) { + Objects.requireNonNull(s, NO_NULL_TASK); if (!this.tasks.contains(s)) { this.tasks.add(s); } else { @@ -92,15 +97,15 @@ synchronized public void register(ShutDownTask s) { } this.runnables.add(s); this.callables.add(s); - this.addPropertyChangeListener("shuttingDown", s); + this.addPropertyChangeListener(PROP_SHUTTING_DOWN, s); } /** * {@inheritDoc} */ @Override - synchronized public void register(Callable task) { - Objects.requireNonNull(task, "Shutdown task cannot be null."); + public synchronized void register(Callable task) { + Objects.requireNonNull(task, NO_NULL_TASK); this.callables.add(task); } @@ -108,8 +113,8 @@ synchronized public void register(Callable task) { * {@inheritDoc} */ @Override - synchronized public void register(Runnable task) { - Objects.requireNonNull(task, "Shutdown task cannot be null."); + public synchronized void register(Runnable task) { + Objects.requireNonNull(task, NO_NULL_TASK); this.runnables.add(task); } @@ -117,8 +122,8 @@ synchronized public void register(Runnable task) { * {@inheritDoc} */ @Override - synchronized public void deregister(ShutDownTask s) { - this.removePropertyChangeListener("shuttingDown", s); + public synchronized void deregister(ShutDownTask s) { + this.removePropertyChangeListener(PROP_SHUTTING_DOWN, s); this.tasks.remove(s); this.callables.remove(s); this.runnables.remove(s); @@ -128,7 +133,7 @@ synchronized public void deregister(ShutDownTask s) { * {@inheritDoc} */ @Override - synchronized public void deregister(Callable task) { + public synchronized void deregister(Callable task) { this.callables.remove(task); } @@ -136,7 +141,7 @@ synchronized public void deregister(Callable task) { * {@inheritDoc} */ @Override - synchronized public void deregister(Runnable task) { + public synchronized void deregister(Runnable task) { this.runnables.remove(task); } @@ -211,9 +216,9 @@ protected boolean shutdown(int status, boolean exit) { log.debug("Shutting down with {} tasks", this.tasks.size()); setShuttingDown(true); // First check if shut down is allowed - for (Callable task : callables) { + for (Callable task : callables) { try { - if (task.call().equals(Boolean.FALSE)) { + if (Boolean.FALSE.equals(task.call())) { setShuttingDown(false); return false; } @@ -225,11 +230,10 @@ protected boolean shutdown(int status, boolean exit) { } // each shut down tasks must complete within _timeout_ milliseconds int timeout = 30000; - runnables.forEach(task -> RP.post(task, timeout)); // close any open windows by triggering a closing event // this gives open windows a final chance to perform any cleanup if (!GraphicsEnvironment.isHeadless()) { - Arrays.asList(Frame.getFrames()).stream().forEach((frame) -> { + Arrays.asList(Frame.getFrames()).stream().forEach(frame -> { // do not run on thread, or in parallel, as System.exit() // will get called before windows can close if (frame.isDisplayable()) { // dispose() has not been called @@ -242,18 +246,15 @@ protected boolean shutdown(int status, boolean exit) { } log.debug("windows completed closing {} milliseconds after starting shutdown", new Date().getTime() - start.getTime()); // wait for parallel tasks to complete - synchronized (start) { - while (!RP.isTerminated()) { - try { - RP.awaitTermination(100, TimeUnit.MILLISECONDS); - } catch (InterruptedException ex) { - // do nothing - } - if ((new Date().getTime() - start.getTime()) > timeout) { - log.warn("Terminating without waiting for stop tasks to complete"); - break; - } + try { + if (!runnables.isEmpty() && !new ProxyTask(runnables.stream() + .map(task -> RP.post(task, 0, Thread.currentThread().getPriority())) + .collect(Collectors.toSet())) + .waitFinished(timeout)) { + log.warn("Terminating without waiting for stop tasks to complete"); } + } catch (InterruptedException ex) { + // do nothing } // success log.debug("Shutdown took {} milliseconds.", new Date().getTime() - start.getTime()); @@ -284,7 +285,26 @@ protected void setShuttingDown(boolean state) { boolean old = shuttingDown; shuttingDown = state; log.debug("Setting shuttingDown to {}", state); - firePropertyChange("shuttingDown", old, state); + firePropertyChange(PROP_SHUTTING_DOWN, old, state); } + final class ProxyTask extends Task implements TaskListener { + private int cnt; + + public ProxyTask(Collection waitFor) { + super(null); + this.cnt = waitFor.size(); + notifyRunning(); + for (Task t : waitFor) { + t.addTaskListener(this); + } + } + + @Override + public synchronized void taskFinished(Task task) { + if (--cnt == 0) { + notifyFinished(); + } + } + } } diff --git a/java/test/jmri/managers/DefaultShutDownManagerTest.java b/java/test/jmri/managers/DefaultShutDownManagerTest.java index 67d11b5be47..581e8895846 100644 --- a/java/test/jmri/managers/DefaultShutDownManagerTest.java +++ b/java/test/jmri/managers/DefaultShutDownManagerTest.java @@ -1,34 +1,45 @@ package jmri.managers; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatCode; + import java.awt.Frame; import java.awt.GraphicsEnvironment; +import java.lang.reflect.Field; +import java.util.concurrent.Callable; import org.junit.After; import org.junit.Assert; import org.junit.Before; import org.junit.Test; +import jmri.InstanceManager; +import jmri.ShutDownManager; import jmri.ShutDownTask; +import jmri.implementation.AbstractShutDownTask; import jmri.implementation.QuietShutDownTask; +import jmri.util.JUnitAppender; import jmri.util.JUnitUtil; /** * - * @author Paul Bender Copyright (C) 2017 + * @author Paul Bender Copyright 2017 + * @author Randall Wood Copyright 2020 */ public class DefaultShutDownManagerTest { + private int runs; + private DefaultShutDownManager dsdm; + @Test public void testCTor() { - DefaultShutDownManager dsdm = new DefaultShutDownManager(); // remove the default shutdown hook to prevent crashes stopping tests Runtime.getRuntime().removeShutdownHook(dsdm.shutdownHook); Assert.assertNotNull("exists", dsdm); } @Test - public void testRegister() { - DefaultShutDownManager dsdm = new DefaultShutDownManager(); + public void testRegister_Task() { Assert.assertEquals(0, dsdm.tasks().size()); ShutDownTask task = new QuietShutDownTask("task") { @Override @@ -39,17 +50,11 @@ public void run() { Assert.assertEquals(1, dsdm.tasks().size()); dsdm.register(task); Assert.assertEquals(1, dsdm.tasks().size()); - try { - dsdm.register(null); - Assert.fail("Expected NullPointerException not thrown"); - } catch (NullPointerException ex) { - // ignore since throwing the NPE is passing - } + assertThatCode(() -> dsdm.register((ShutDownTask) null)).isInstanceOf(NullPointerException.class); } @Test - public void testDeregister() { - DefaultShutDownManager dsdm = new DefaultShutDownManager(); + public void testDeregister_Task() { Assert.assertEquals(0, dsdm.tasks().size()); ShutDownTask task = new QuietShutDownTask("task") { @Override @@ -61,11 +66,57 @@ public void run() { Assert.assertTrue(dsdm.tasks().contains(task)); dsdm.deregister(task); Assert.assertEquals(0, dsdm.tasks().size()); + assertThatCode(() -> dsdm.deregister((ShutDownTask) null)).doesNotThrowAnyException(); + } + + @Test + public void testRegister_Callable() { + Assert.assertEquals(0, dsdm.getCallables().size()); + Callable task = () -> true; + dsdm.register(task); + Assert.assertEquals(1, dsdm.getCallables().size()); + dsdm.register(task); + Assert.assertEquals(1, dsdm.getCallables().size()); + assertThatCode(() -> dsdm.register((Callable) null)).isInstanceOf(NullPointerException.class); + } + + @Test + public void testDeregister_Callable() { + Assert.assertEquals(0, dsdm.getCallables().size()); + Callable task = () -> true; + dsdm.register(task); + Assert.assertEquals(1, dsdm.getCallables().size()); + Assert.assertTrue(dsdm.getCallables().contains(task)); + dsdm.deregister(task); + Assert.assertEquals(0, dsdm.getCallables().size()); + assertThatCode(() -> dsdm.deregister((Callable) null)).doesNotThrowAnyException(); + } + + @Test + public void testRegister_Runnable() { + Assert.assertEquals(0, dsdm.getRunnables().size()); + Runnable task = () -> {}; + dsdm.register(task); + Assert.assertEquals(1, dsdm.getRunnables().size()); + dsdm.register(task); + Assert.assertEquals(1, dsdm.getRunnables().size()); + assertThatCode(() -> dsdm.register((Runnable) null)).isInstanceOf(NullPointerException.class); + } + + @Test + public void testDeregister_Runnable() { + Assert.assertEquals(0, dsdm.getRunnables().size()); + Runnable task = () -> {}; + dsdm.register(task); + Assert.assertEquals(1, dsdm.getRunnables().size()); + Assert.assertTrue(dsdm.getRunnables().contains(task)); + dsdm.deregister(task); + Assert.assertEquals(0, dsdm.getRunnables().size()); + assertThatCode(() -> dsdm.deregister((Runnable) null)).doesNotThrowAnyException(); } @Test public void testIsShuttingDown() { - DefaultShutDownManager dsdm = new DefaultShutDownManager(); Frame frame = null; if (!GraphicsEnvironment.isHeadless()) { frame = new Frame("Shutdown test frame"); @@ -78,23 +129,97 @@ public void testIsShuttingDown() { } } + @Test + public void testShutDownisInterruptedByShutDownTask() { + dsdm.register(new AbstractShutDownTask("test") { + + @Override + public Boolean call() { + return false; + } + + @Override + public void run() { + runs++; + }}); + dsdm.shutdown(0, false); + assertThat(dsdm.isShuttingDown()).isFalse(); + assertThat(runs).isEqualTo(0); + } + + @Test + public void testShutDownisNotInterruptedByShutDownTask() { + dsdm.register(new AbstractShutDownTask("test") { + + @Override + public Boolean call() { + return true; + } + + @Override + public void run() { + runs++; + }}); + dsdm.shutdown(0, false); + assertThat(dsdm.isShuttingDown()).isTrue(); + assertThat(runs).isEqualTo(1); + } + + @Test + public void testShutDownisInterruptedByCallableFalse() { + dsdm.register(() -> runs++); + dsdm.register(() -> Boolean.FALSE); + dsdm.shutdown(0, false); + assertThat(dsdm.isShuttingDown()).isFalse(); + assertThat(runs).isEqualTo(0); + } + + @Test + public void testShutDownisInterruptedByCallableException() { + dsdm.register(() -> runs++); + dsdm.register(() -> { + throw new Exception(); + }); + dsdm.shutdown(0, false); + assertThat(dsdm.isShuttingDown()).isFalse(); + assertThat(runs).isEqualTo(0); + JUnitAppender.assertErrorMessage("Unable to stop"); + } + + @Test + public void testShutDownisNotInterruptedByCallable() { + dsdm.register(() -> runs++); + dsdm.register(() -> true); + dsdm.shutdown(0, false); + assertThat(dsdm.isShuttingDown()).isTrue(); + assertThat(runs).isEqualTo(1); + } + @Test public void testInstanceManagerCreates() { - jmri.ShutDownManager sdm = jmri.InstanceManager.getNullableDefault(jmri.ShutDownManager.class); - Assert.assertNotNull(sdm); + assertThat(InstanceManager.getNullableDefault(ShutDownManager.class)).isNotNull(); } - @Before public void setUp() { JUnitUtil.setUp(); + dsdm = new DefaultShutDownManager(); + runs = 0; } @After public void tearDown() { + try { + Class c = jmri.managers.DefaultShutDownManager.class; + Field f = c.getDeclaredField("shuttingDown"); + f.setAccessible(true); + f.set(dsdm, false); + } catch (NoSuchFieldException | IllegalArgumentException | IllegalAccessException x) { + log.error("Failed to reset DefaultShutDownManager shuttingDown field", x); + } JUnitUtil.tearDown(); } - // private final static Logger log = LoggerFactory.getLogger(DefaultShutDownManagerTest.class); + private final static org.slf4j.Logger log = org.slf4j.LoggerFactory.getLogger(DefaultShutDownManagerTest.class); }