changeset 5203:1a55aa923188

[TEST ONLY] RT-30173: Possible race condition in javafx.concurrent implementation or unit tests
author rbair
date Fri, 27 Sep 2013 16:11:29 -0700
parents d29c1c555d01
children cc58b4072446
files modules/graphics/src/main/java/javafx/concurrent/ScheduledService.java modules/graphics/src/main/java/javafx/concurrent/Service.java modules/graphics/src/main/java/javafx/concurrent/Task.java modules/graphics/src/test/java/javafx/concurrent/ScheduledServiceTest.java modules/graphics/src/test/java/javafx/concurrent/ServiceLifecycleTest.java
diffstat 5 files changed, 56 insertions(+), 38 deletions(-) [+]
line wrap: on
line diff
--- a/modules/graphics/src/main/java/javafx/concurrent/ScheduledService.java	Fri Sep 27 15:00:44 2013 -0700
+++ b/modules/graphics/src/main/java/javafx/concurrent/ScheduledService.java	Fri Sep 27 16:11:29 2013 -0700
@@ -25,8 +25,6 @@
 
 package javafx.concurrent;
 
-import java.util.Timer;
-import java.util.TimerTask;
 import javafx.beans.property.BooleanProperty;
 import javafx.beans.property.IntegerProperty;
 import javafx.beans.property.ObjectProperty;
@@ -39,6 +37,8 @@
 import javafx.beans.property.SimpleObjectProperty;
 import javafx.util.Callback;
 import javafx.util.Duration;
+import java.util.Timer;
+import java.util.TimerTask;
 
 /**
  * <p>The ScheduledService is a {@link Service} which will automatically restart
--- a/modules/graphics/src/main/java/javafx/concurrent/Service.java	Fri Sep 27 15:00:44 2013 -0700
+++ b/modules/graphics/src/main/java/javafx/concurrent/Service.java	Fri Sep 27 16:11:29 2013 -0700
@@ -25,14 +25,6 @@
 
 package javafx.concurrent;
 
-import java.security.AccessController;
-import java.security.PrivilegedAction;
-import java.util.concurrent.BlockingQueue;
-import java.util.concurrent.Executor;
-import java.util.concurrent.LinkedBlockingQueue;
-import java.util.concurrent.ThreadFactory;
-import java.util.concurrent.ThreadPoolExecutor;
-import java.util.concurrent.TimeUnit;
 import javafx.application.Platform;
 import javafx.beans.property.BooleanProperty;
 import javafx.beans.property.DoubleProperty;
@@ -53,9 +45,21 @@
 import javafx.event.EventHandler;
 import javafx.event.EventTarget;
 import javafx.event.EventType;
+import java.security.AccessController;
+import java.security.PrivilegedAction;
+import java.util.concurrent.BlockingQueue;
+import java.util.concurrent.Executor;
+import java.util.concurrent.LinkedBlockingQueue;
+import java.util.concurrent.ThreadFactory;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
 import sun.util.logging.PlatformLogger;
-
-import static javafx.concurrent.WorkerStateEvent.*;
+import static javafx.concurrent.WorkerStateEvent.WORKER_STATE_CANCELLED;
+import static javafx.concurrent.WorkerStateEvent.WORKER_STATE_FAILED;
+import static javafx.concurrent.WorkerStateEvent.WORKER_STATE_READY;
+import static javafx.concurrent.WorkerStateEvent.WORKER_STATE_RUNNING;
+import static javafx.concurrent.WorkerStateEvent.WORKER_STATE_SCHEDULED;
+import static javafx.concurrent.WorkerStateEvent.WORKER_STATE_SUCCEEDED;
 
 /**
  * <p>
--- a/modules/graphics/src/main/java/javafx/concurrent/Task.java	Fri Sep 27 15:00:44 2013 -0700
+++ b/modules/graphics/src/main/java/javafx/concurrent/Task.java	Fri Sep 27 16:11:29 2013 -0700
@@ -25,13 +25,32 @@
 
 package javafx.concurrent;
 
+import javafx.application.Platform;
+import javafx.beans.property.BooleanProperty;
+import javafx.beans.property.DoubleProperty;
+import javafx.beans.property.ObjectProperty;
+import javafx.beans.property.ReadOnlyBooleanProperty;
+import javafx.beans.property.ReadOnlyDoubleProperty;
+import javafx.beans.property.ReadOnlyObjectProperty;
+import javafx.beans.property.ReadOnlyStringProperty;
+import javafx.beans.property.SimpleBooleanProperty;
+import javafx.beans.property.SimpleDoubleProperty;
+import javafx.beans.property.SimpleObjectProperty;
+import javafx.beans.property.SimpleStringProperty;
+import javafx.beans.property.StringProperty;
+import javafx.event.Event;
+import javafx.event.EventDispatchChain;
+import javafx.event.EventHandler;
+import javafx.event.EventTarget;
+import javafx.event.EventType;
 import java.util.concurrent.Callable;
 import java.util.concurrent.FutureTask;
 import java.util.concurrent.atomic.AtomicReference;
-import javafx.application.Platform;
-import javafx.beans.property.*;
-import javafx.event.*;
-import static javafx.concurrent.WorkerStateEvent.*;
+import static javafx.concurrent.WorkerStateEvent.WORKER_STATE_CANCELLED;
+import static javafx.concurrent.WorkerStateEvent.WORKER_STATE_FAILED;
+import static javafx.concurrent.WorkerStateEvent.WORKER_STATE_RUNNING;
+import static javafx.concurrent.WorkerStateEvent.WORKER_STATE_SCHEDULED;
+import static javafx.concurrent.WorkerStateEvent.WORKER_STATE_SUCCEEDED;
 
 /**
  * <p>
@@ -964,7 +983,7 @@
             // If this method was called on the FX application thread, then we can
             // just update the state directly and this will make sure that after
             // the cancel method was called, the state will be set correctly
-            // (otherwise it would be indeterminate. However if the cancel method was
+            // (otherwise it would be indeterminate). However if the cancel method was
             // called off the FX app thread, then we must use runLater, and the
             // state flag will not be readable immediately after this call. However,
             // that would be the case anyway since these properties are not thread-safe.
--- a/modules/graphics/src/test/java/javafx/concurrent/ScheduledServiceTest.java	Fri Sep 27 15:00:44 2013 -0700
+++ b/modules/graphics/src/test/java/javafx/concurrent/ScheduledServiceTest.java	Fri Sep 27 16:11:29 2013 -0700
@@ -25,15 +25,16 @@
 
 package javafx.concurrent;
 
-import java.util.TimerTask;
-import java.util.concurrent.atomic.AtomicInteger;
 import javafx.concurrent.mocks.EpicFailTask;
 import javafx.concurrent.mocks.SimpleTask;
 import javafx.util.Callback;
 import javafx.util.Duration;
+import java.util.TimerTask;
+import java.util.concurrent.atomic.AtomicInteger;
 import org.junit.Test;
-
-import static org.junit.Assert.*;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
 
 /**
  * Tests for the ScheduledService.
@@ -75,8 +76,6 @@
      */
     private long wallClock;
 
-
-
     @Override protected TestServiceFactory setupServiceFactory() {
         return new TestServiceFactory() {
             @Override protected AbstractTask createTestTask() {
--- a/modules/graphics/src/test/java/javafx/concurrent/ServiceLifecycleTest.java	Fri Sep 27 15:00:44 2013 -0700
+++ b/modules/graphics/src/test/java/javafx/concurrent/ServiceLifecycleTest.java	Fri Sep 27 16:11:29 2013 -0700
@@ -25,19 +25,21 @@
 
 package javafx.concurrent;
 
-import java.util.concurrent.Executor;
-import java.util.concurrent.atomic.AtomicBoolean;
-import java.util.concurrent.atomic.AtomicReference;
 import javafx.beans.value.ChangeListener;
 import javafx.beans.value.ObservableValue;
 import javafx.concurrent.mocks.MythicalEvent;
 import javafx.concurrent.mocks.SimpleTask;
 import javafx.event.EventHandler;
+import java.util.concurrent.Executor;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicReference;
 import org.junit.After;
 import org.junit.Test;
-import org.junit.Ignore;
-
-import static org.junit.Assert.*;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertSame;
+import static org.junit.Assert.assertTrue;
 
 /**
  * Tests various rules regarding the lifecycle of a Service.
@@ -100,7 +102,7 @@
 
     protected final class ManualTask extends AbstractTask {
         private AtomicBoolean finish = new AtomicBoolean(false);
-        private AtomicReference<Exception> exception = new AtomicReference<Exception>();
+        private AtomicReference<Exception> exception = new AtomicReference<>();
         private boolean failToCancel = false;
 
         @Override protected String call() throws Exception {
@@ -136,8 +138,9 @@
 
         @Override
         public boolean cancel(boolean mayInterruptIfRunning) {
+            boolean result = failToCancel ? false : super.cancel(mayInterruptIfRunning);
             finish.set(true);
-            return failToCancel ? false : super.cancel(mayInterruptIfRunning);
+            return result;
         }
     }
 
@@ -319,7 +322,6 @@
         assertSame(Worker.State.SCHEDULED, service.stateProperty().get());
     }
 
-    @Ignore("RT-30173")
     @Test public void callingCancelInRunningStateResultsInCancelledState() {
         service.start();
         executor.executeScheduled();
@@ -1294,7 +1296,6 @@
         assertNull(service.onCancelledProperty().get());
     }
 
-    @Ignore("RT-30173")
     @Test public void onCancelledFilterCalledBefore_onCancelled() {
         final AtomicBoolean filterCalled = new AtomicBoolean(false);
         final AtomicBoolean filterCalledFirst = new AtomicBoolean(false);
@@ -1317,7 +1318,6 @@
         assertTrue(filterCalledFirst.get());
     }
 
-    @Ignore("RT-30173")
     @Test public void cancelledCalledAfterHandler() {
         final AtomicBoolean handlerCalled = new AtomicBoolean(false);
         service.setOnCancelled(new EventHandler<WorkerStateEvent>() {
@@ -1334,7 +1334,6 @@
         assertTrue(handlerCalled.get() && factory.getCurrentTask().cancelledSemaphore.getQueueLength() == 0);
     }
 
-    @Ignore("RT-30173")
     @Test public void cancelledCalledAfterHandlerEvenIfConsumed() {
         final AtomicBoolean handlerCalled = new AtomicBoolean(false);
         service.setOnCancelled(new EventHandler<WorkerStateEvent>() {
@@ -1352,7 +1351,6 @@
         assertTrue(handlerCalled.get() && factory.getCurrentTask().cancelledSemaphore.getQueueLength() == 0);
     }
 
-    @Ignore("RT-30173")
     @Test public void onCancelledHandlerCalled() {
         final AtomicBoolean handlerCalled = new AtomicBoolean(false);
         service.addEventHandler(WorkerStateEvent.WORKER_STATE_CANCELLED, new EventHandler<WorkerStateEvent>() {
@@ -1369,7 +1367,6 @@
         assertTrue(handlerCalled.get());
     }
 
-    @Ignore("RT-30173")
     @Test public void removed_onCancelledHandlerNotCalled() {
         final AtomicBoolean handlerCalled = new AtomicBoolean(false);
         final AtomicBoolean sanity = new AtomicBoolean(false);
@@ -1394,7 +1391,6 @@
         assertFalse(handlerCalled.get());
     }
 
-    @Ignore("RT-30173")
     @Test public void removed_onCancelledFilterNotCalled() {
         final AtomicBoolean filterCalled = new AtomicBoolean(false);
         final AtomicBoolean sanity = new AtomicBoolean(false);