changeset 5224:19431d2dcdd5

RT-33038: ScheduledService doesn't stop after calling cancel Reviewed by: snorthov
author rbair
date Tue, 01 Oct 2013 15:36:09 -0700
parents 932cecc14e49
children 90e48442d0ff
files modules/graphics/src/main/java/javafx/concurrent/ScheduledService.java modules/graphics/src/main/java/javafx/concurrent/Service.java modules/graphics/src/test/java/javafx/concurrent/ScheduledServiceTest.java modules/graphics/src/test/java/javafx/concurrent/ServiceLifecycleTest.java
diffstat 4 files changed, 243 insertions(+), 46 deletions(-) [+]
line wrap: on
line diff
--- a/modules/graphics/src/main/java/javafx/concurrent/ScheduledService.java	Tue Oct 01 13:54:17 2013 -0700
+++ b/modules/graphics/src/main/java/javafx/concurrent/ScheduledService.java	Tue Oct 01 15:36:09 2013 -0700
@@ -333,6 +333,13 @@
      */
     private TimerTask delayTask = null;
 
+    /**
+     * This is set to false when the "cancel" method is called, and reset to true on "reset".
+     * We need this so that any time the developer calls 'cancel', even when from within one
+     * of the event handlers, it will cause us to transition to the cancelled state.
+     */
+    private boolean stop = false;
+
     // This method is invoked by Service to actually execute the task. In the normal implementation
     // in Service, this method will simply delegate to the Executor. In ScheduledService, however,
     // we instead will delay the correct amount of time before we finally invoke executeTaskNow,
@@ -357,7 +364,7 @@
                 // If the delay is zero or null, then just start immediately
                 executeTaskNow(task);
             } else {
-                schedule(createTimerTask(task), d);
+                schedule(delayTask = createTimerTask(task), d);
             }
         } else {
             // We are executing as a result of an iteration, not a fresh start.
@@ -369,7 +376,7 @@
             if (runPeriod < cumulative) {
                 // Pause and then execute
                 assert delayTask == null;
-                schedule(createTimerTask(task), (long) (cumulative - runPeriod));
+                schedule(delayTask = createTimerTask(task), (long) (cumulative - runPeriod));
             } else {
                 // Execute immediately
                 executeTaskNow(task);
@@ -388,25 +395,19 @@
         // Reset the cumulative time
         Duration d = getPeriod();
         setCumulativePeriod(d);
+        // Have to save this off, since it will be reset here in a second
+        final boolean wasCancelled = stop;
         // Call the super implementation of reset, which will not cause us
         // to think this is a new fresh start.
-        ScheduledService.this.superReset();
+        superReset();
         assert freshStart == false;
-        // Fire it up!
-        ScheduledService.this.start();
-    }
-
-    /**
-     * @inheritDoc
-     *
-     * Implementation Note: Subclasses which override this method must call this super implementation.
-     */
-    @Override protected void cancelled() {
-        super.cancelled();
-        // Stop the delayTask if it exists
-        if (delayTask != null) {
-            delayTask.cancel();
-            delayTask = null;
+        // If it was cancelled then we will progress from READY to SCHEDULED to CANCELLED so that
+        // the lifecycle changes are predictable according to the Service specification.
+        if (wasCancelled) {
+            cancelFromReadyState();
+        } else {
+            // Fire it up!
+            start();
         }
     }
 
@@ -429,9 +430,9 @@
                 setCumulativePeriod(d);
             }
 
-            ScheduledService.this.superReset();
+            superReset();
             assert freshStart == false;
-            ScheduledService.this.start();
+            start();
         } else {
             // We've maxed out, so do nothing and things will just stop.
         }
@@ -444,6 +445,7 @@
      */
     @Override public void reset() {
         super.reset();
+        stop = false;
         setCumulativePeriod(getPeriod());
         lastValue.set(null);
         setCurrentFailureCount(0);
@@ -452,6 +454,23 @@
     }
 
     /**
+     * Cancels any currently running task and stops this scheduled service, such that
+     * no additional iterations will occur.
+     *
+     * @return whether any running task was cancelled, false if no task was cancelled.
+     *         In any case, the ScheduledService will stop iterating.
+     */
+    @Override public boolean cancel() {
+        boolean ret = super.cancel();
+        stop = true;
+        if (delayTask != null) {
+            delayTask.cancel();
+            delayTask = null;
+        }
+        return ret;
+    }
+
+    /**
      * This method exists only for testing purposes. The normal implementation
      * will delegate to a java.util.Timer, however during testing we want to simply
      * inspect the value for the delay and execute immediately.
--- a/modules/graphics/src/main/java/javafx/concurrent/Service.java	Tue Oct 01 13:54:17 2013 -0700
+++ b/modules/graphics/src/main/java/javafx/concurrent/Service.java	Tue Oct 01 15:36:09 2013 -0700
@@ -510,9 +510,10 @@
         // notifications whenever the state of the Service has changed.
         state.addListener(new ChangeListener<State>() {
             @Override public void changed(ObservableValue<? extends State> observableValue,
-                                          State oldState, State newState) {
-                // Invoke the event handlers, and then call the protected methods.
-                switch (state.get()) {
+                                          State old, State value) {
+
+                // Invoke the appropriate event handler
+                switch (value) {
                     case CANCELLED:
                         fireEvent(new WorkerStateEvent(Service.this, WORKER_STATE_CANCELLED));
                         cancelled();
@@ -543,7 +544,11 @@
         });
     }
 
-    @Override public final boolean cancel() {
+    /**
+     * Cancels any currently running Task, if any. The state will be set to CANCELLED.
+     * @return returns true if the cancel was successful
+     */
+    @Override public boolean cancel() {
         checkThread();
         if (task == null) {
             if (state.get() == State.CANCELLED || state.get() == State.SUCCEEDED) {
@@ -579,7 +584,7 @@
             // we really can just let the old task run and create a new
             // task and the Service will be none the wiser.
             state.unbind();
-            state.setValue(State.CANCELLED);
+            state.set(State.CANCELLED);
         }
 
         // Reset
@@ -656,6 +661,20 @@
     }
 
     /**
+     * This is used by ScheduledService to cancel a Service that is in the READY state. The problem is
+     * that a ScheduledService will iterate from SUCCEEDED to READY, and then call start() to transition
+     * from READY to SCHEDULED and kick off a new iteration. However, if from the SUCCEEDED event handler
+     * a developer calls "cancel" to stop a ScheduledService, we have a problem, since the Service
+     * specification does not allow to transition from a terminal state (SUCCEEDED) to another terminal
+     * state (CANCELLED), but this is clearly what we need to do. So what this method will do is allow
+     * us to transition from the READY state to the CANCELLED state, by transitioning through SCHEDULED
+     */
+    void cancelFromReadyState() {
+        state.set(State.SCHEDULED);
+        state.set(State.CANCELLED);
+    }
+
+    /**
      * <p>
      *     Uses the <code>executor</code> defined on this Service to execute the
      *     given task. If the <code>executor</code> is null, then a default
--- a/modules/graphics/src/test/java/javafx/concurrent/ScheduledServiceTest.java	Tue Oct 01 13:54:17 2013 -0700
+++ b/modules/graphics/src/test/java/javafx/concurrent/ScheduledServiceTest.java	Tue Oct 01 15:36:09 2013 -0700
@@ -27,12 +27,15 @@
 
 import javafx.concurrent.mocks.EpicFailTask;
 import javafx.concurrent.mocks.SimpleTask;
+import javafx.event.EventHandler;
 import javafx.util.Callback;
 import javafx.util.Duration;
 import java.util.TimerTask;
+import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicInteger;
 import org.junit.Test;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 
@@ -805,6 +808,44 @@
         assertNull(s.getLastValue());
     }
 
+    @Test public void callingCancelFromOnSucceededEventHandlerShouldStopScheduledService() {
+        AtomicBoolean onReadyCalled = new AtomicBoolean();
+        AtomicBoolean onScheduledCalled = new AtomicBoolean();
+        AtomicBoolean onCancelledCalled = new AtomicBoolean();
+        s.setOnSucceeded(new EventHandler<WorkerStateEvent>() {
+            @Override public void handle(WorkerStateEvent event) {
+                s.cancel();
+                // Reset these so that they only get set to true if called
+                // after the cancel step
+                onReadyCalled.set(false);
+                onScheduledCalled.set(false);
+                onCancelledCalled.set(false);
+            }
+        });
+        s.setOnReady(new EventHandler<WorkerStateEvent>() {
+            @Override public void handle(WorkerStateEvent event) {
+                onReadyCalled.set(true);
+            }
+        });
+        s.setOnScheduled(new EventHandler<WorkerStateEvent>() {
+            @Override public void handle(WorkerStateEvent event) {
+                onScheduledCalled.set(true);
+            }
+        });
+        s.setOnCancelled(new EventHandler<WorkerStateEvent>() {
+            @Override public void handle(WorkerStateEvent event) {
+                onCancelledCalled.set(true);
+            }
+        });
+
+        s.start();
+        assertFalse(s.isRunning());
+        assertEquals(Worker.State.CANCELLED, s.getState());
+        assertTrue(onReadyCalled.get());
+        assertTrue(onScheduledCalled.get());
+        assertTrue(onCancelledCalled.get());
+    }
+
     /**
      * Allows us to monkey with how the threading works for the sake of testing.
      * Basically, you just call start() in order to go through an entire first
--- a/modules/graphics/src/test/java/javafx/concurrent/ServiceLifecycleTest.java	Tue Oct 01 13:54:17 2013 -0700
+++ b/modules/graphics/src/test/java/javafx/concurrent/ServiceLifecycleTest.java	Tue Oct 01 15:36:09 2013 -0700
@@ -32,6 +32,7 @@
 import javafx.event.EventHandler;
 import java.util.concurrent.Executor;
 import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
 import java.util.concurrent.atomic.AtomicReference;
 import org.junit.After;
 import org.junit.Test;
@@ -639,7 +640,7 @@
     }
 
     /************************************************************************
-     * Proper Completion of a task                              *
+     * Proper Completion of a task                                          *
      ***********************************************************************/
 
     @Test(expected = IllegalStateException.class)
@@ -773,9 +774,9 @@
     }
 
     /***************************************************************************
-     *
-     * Tests for onReady
-     *
+     *                                                                         *
+     * Tests for onReady                                                       *
+     *                                                                         *
      **************************************************************************/
 
     @Test public void onReadyPropertyNameShouldMatchMethodName() {
@@ -880,10 +881,31 @@
         assertFalse(filterCalled.get());
     }
 
+    @Test public void cancelCalledFromOnReady() {
+        final AtomicInteger cancelNotificationCount = new AtomicInteger();
+        service.start();
+        executor.executeScheduled();
+        task.complete();
+        service.addEventFilter(WorkerStateEvent.WORKER_STATE_READY, new EventHandler<WorkerStateEvent>() {
+            @Override public void handle(WorkerStateEvent workerStateEvent) {
+                service.cancel();
+            }
+        });
+        service.addEventFilter(WorkerStateEvent.WORKER_STATE_CANCELLED, new EventHandler<WorkerStateEvent>() {
+            @Override public void handle(WorkerStateEvent event) {
+                cancelNotificationCount.incrementAndGet();
+            }
+        });
+
+        service.reset();
+        assertEquals(Worker.State.CANCELLED, service.getState());
+        assertEquals(1, cancelNotificationCount.get());
+    }
+
     /***************************************************************************
-     *
-     * Tests for onScheduled
-     *
+     *                                                                         *
+     * Tests for onScheduled                                                   *
+     *                                                                         *
      **************************************************************************/
 
     @Test public void onScheduledPropertyNameShouldMatchMethodName() {
@@ -1010,10 +1032,29 @@
         assertFalse(filterCalled.get());
     }
 
+    @Test public void cancelCalledFromOnScheduled() {
+        final AtomicInteger cancelNotificationCount = new AtomicInteger();
+        service.addEventFilter(WorkerStateEvent.WORKER_STATE_SCHEDULED, new EventHandler<WorkerStateEvent>() {
+            @Override public void handle(WorkerStateEvent workerStateEvent) {
+                service.cancel();
+            }
+        });
+        service.addEventFilter(WorkerStateEvent.WORKER_STATE_CANCELLED, new EventHandler<WorkerStateEvent>() {
+            @Override public void handle(WorkerStateEvent event) {
+                cancelNotificationCount.incrementAndGet();
+            }
+        });
+
+        service.start();
+        executor.executeScheduled();
+        assertEquals(Worker.State.CANCELLED, service.getState());
+        assertEquals(1, cancelNotificationCount.get());
+    }
+
     /***************************************************************************
-     *
-     * Tests for onRunning
-     *
+     *                                                                         *
+     * Tests for onRunning                                                     *
+     *                                                                         *
      **************************************************************************/
 
     @Test public void onRunningPropertyNameShouldMatchMethodName() {
@@ -1140,10 +1181,29 @@
         assertFalse(filterCalled.get());
     }
 
+    @Test public void cancelCalledFromOnRunning() {
+        final AtomicInteger cancelNotificationCount = new AtomicInteger();
+        service.addEventFilter(WorkerStateEvent.WORKER_STATE_RUNNING, new EventHandler<WorkerStateEvent>() {
+            @Override public void handle(WorkerStateEvent workerStateEvent) {
+                service.cancel();
+            }
+        });
+        service.addEventFilter(WorkerStateEvent.WORKER_STATE_CANCELLED, new EventHandler<WorkerStateEvent>() {
+            @Override public void handle(WorkerStateEvent event) {
+                cancelNotificationCount.incrementAndGet();
+            }
+        });
+
+        service.start();
+        executor.executeScheduled();
+        assertEquals(Worker.State.CANCELLED, service.getState());
+        assertEquals(1, cancelNotificationCount.get());
+    }
+
     /***************************************************************************
-     *
-     * Tests for onSucceeded
-     *
+     *                                                                         *
+     * Tests for onSucceeded                                                   *
+     *                                                                         *
      **************************************************************************/
 
     @Test public void onSucceededPropertyNameShouldMatchMethodName() {
@@ -1273,14 +1333,32 @@
         service.start();
         executor.executeScheduled();
         task.complete();
-        assertTrue(sanity.get());
-        assertFalse(filterCalled.get());
+    }
+
+    @Test public void cancelCalledFromOnSucceeded() {
+        final AtomicInteger cancelNotificationCount = new AtomicInteger();
+        service.addEventFilter(WorkerStateEvent.WORKER_STATE_SUCCEEDED, new EventHandler<WorkerStateEvent>() {
+            @Override public void handle(WorkerStateEvent workerStateEvent) {
+                service.cancel();
+            }
+        });
+        service.addEventFilter(WorkerStateEvent.WORKER_STATE_CANCELLED, new EventHandler<WorkerStateEvent>() {
+            @Override public void handle(WorkerStateEvent event) {
+                cancelNotificationCount.incrementAndGet();
+            }
+        });
+
+        service.start();
+        executor.executeScheduled();
+        task.complete();
+        assertEquals(Worker.State.SUCCEEDED, service.getState());
+        assertEquals(0, cancelNotificationCount.get());
     }
 
     /***************************************************************************
-     *
-     * Tests for onCancelled
-     *
+     *                                                                         *
+     * Tests for onCancelled                                                   *
+     *                                                                         *
      **************************************************************************/
 
     @Test public void onCancelledPropertyNameShouldMatchMethodName() {
@@ -1414,10 +1492,50 @@
         assertFalse(filterCalled.get());
     }
 
+    @Test public void cancelCalledFromOnCancelled() {
+        final AtomicInteger cancelNotificationCount = new AtomicInteger();
+        service.addEventFilter(WorkerStateEvent.WORKER_STATE_CANCELLED, new EventHandler<WorkerStateEvent>() {
+            @Override public void handle(WorkerStateEvent workerStateEvent) {
+                service.cancel();
+            }
+        });
+        service.addEventFilter(WorkerStateEvent.WORKER_STATE_CANCELLED, new EventHandler<WorkerStateEvent>() {
+            @Override public void handle(WorkerStateEvent event) {
+                cancelNotificationCount.incrementAndGet();
+            }
+        });
+
+        service.start();
+        executor.executeScheduled();
+        task.cancel();
+        assertEquals(Worker.State.CANCELLED, service.getState());
+        assertEquals(1, cancelNotificationCount.get());
+    }
+
+    @Test public void cancelCalledFromOnFailed() {
+        final AtomicInteger cancelNotificationCount = new AtomicInteger();
+        service.addEventFilter(WorkerStateEvent.WORKER_STATE_FAILED, new EventHandler<WorkerStateEvent>() {
+            @Override public void handle(WorkerStateEvent workerStateEvent) {
+                service.cancel();
+            }
+        });
+        service.addEventFilter(WorkerStateEvent.WORKER_STATE_CANCELLED, new EventHandler<WorkerStateEvent>() {
+            @Override public void handle(WorkerStateEvent event) {
+                cancelNotificationCount.incrementAndGet();
+            }
+        });
+
+        service.start();
+        executor.executeScheduled();
+        task.fail(new Exception("Quit"));
+        assertEquals(Worker.State.FAILED, service.getState());
+        assertEquals(0, cancelNotificationCount.get());
+    }
+
     /***************************************************************************
-     *
-     * Tests for onFailed
-     *
+     *                                                                         *
+     * Tests for onFailed                                                      *
+     *                                                                         *
      **************************************************************************/
 
     @Test public void onFailedPropertyNameShouldMatchMethodName() {