8088343: Race condition in javafx.concurrent.Task::cancel

Reviewed-by: kcr, arapte

Author: Andy Goryachev

modules/javafx.graphics/src/main/java/javafx/concurrent/Task.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2010, 2024, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2010, 2025, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -25,6 +25,14 @@
 
 package javafx.concurrent;
 
+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;
+import java.util.concurrent.Callable;
+import java.util.concurrent.FutureTask;
+import java.util.concurrent.atomic.AtomicReference;
 import javafx.application.Platform;
 import javafx.beans.property.BooleanProperty;
 import javafx.beans.property.DoubleProperty;
@@ -43,14 +51,6 @@
 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 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>
@@ -998,7 +998,8 @@ protected void failed() { }
         return cancel(true);
     }
 
-    @Override public boolean cancel(boolean mayInterruptIfRunning) {
+    @Override
+    public boolean cancel(boolean mayInterruptIfRunning) {
         // Delegate to the super implementation to actually attempt to cancel this thing
         // Assert the modifyThread permission
         boolean flag = super.cancel(mayInterruptIfRunning);
@@ -1014,12 +1015,30 @@ protected void failed() { }
             // state flag will not be readable immediately after this call. However,
             // that would be the case anyway since these properties are not thread-safe.
             if (isFxApplicationThread()) {
+                switch (getState()) {
+                case FAILED:
+                case SUCCEEDED:
+                    // a finished or failed task retains its state
+                    return false;
+                }
+
                 setState(Worker.State.CANCELLED);
             } else {
-                runLater(() -> setState(Worker.State.CANCELLED));
+                runLater(() -> {
+                    // the state must be accessed only in the fx application thread
+                    switch (getState()) {
+                    case FAILED:
+                    case SUCCEEDED:
+                        // a finished or failed task retains its state
+                        break;
+                    default:
+                        setState(Worker.State.CANCELLED);
+                        break;
+                    }
+                });
+                return flag;
             }
         }
-        // return the flag
         return flag;
     }
 

modules/javafx.graphics/src/test/java/test/javafx/concurrent/ServiceLifecycleTest.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2010, 2024, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2010, 2025, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -25,9 +25,13 @@
 
 package test.javafx.concurrent;
 
-import test.javafx.concurrent.mocks.MythicalEvent;
-import test.javafx.concurrent.mocks.SimpleTask;
-import javafx.event.EventHandler;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertSame;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 import java.util.concurrent.ConcurrentLinkedQueue;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.Executor;
@@ -40,17 +44,13 @@
 import javafx.concurrent.TaskShim;
 import javafx.concurrent.Worker;
 import javafx.concurrent.WorkerStateEvent;
-
+import javafx.event.EventHandler;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.RepeatedTest;
 import org.junit.jupiter.api.Test;
-import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.junit.jupiter.api.Assertions.assertFalse;
-import static org.junit.jupiter.api.Assertions.assertNotNull;
-import static org.junit.jupiter.api.Assertions.assertNull;
-import static org.junit.jupiter.api.Assertions.assertSame;
-import static org.junit.jupiter.api.Assertions.assertThrows;
-import static org.junit.jupiter.api.Assertions.assertTrue;
+import test.javafx.concurrent.mocks.MythicalEvent;
+import test.javafx.concurrent.mocks.SimpleTask;
 
 /**
  * Tests various rules regarding the lifecycle of a Service.
@@ -1278,6 +1278,7 @@ public void removed_onSucceededFilterNotCalled() {
         task.complete();
     }
 
+    @RepeatedTest(50)
     @Test
     public void cancelCalledFromOnSucceeded() {
         final AtomicInteger cancelNotificationCount = new AtomicInteger();
@@ -1422,6 +1423,7 @@ public void cancelCalledFromOnCancelled() {
         assertEquals(1, cancelNotificationCount.get());
     }
 
+    @RepeatedTest(50)
     @Test
     public void cancelCalledFromOnFailed() {
         final AtomicInteger cancelNotificationCount = new AtomicInteger();