RFR: 8088343: Intermittent unit test failure in javafx.concurrent.ServiceLifecycleTest [v2]

[Kevin Rushforth]

On Fri, 11 Apr 2025 17:06:38 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> The code should not set the `Task.state` value to `CANCELLED` if the said task is already `SUCCEEDED` or `FAILED`.
>> 
>> This is a product bug.
>> 
>> Added `@RepeatedTest(50)` to the tests that used to fail intermittently - this made the test failed more reliably without the fix.
>
> Andy Goryachev has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains two additional commits since the last revision:
> 
>  - Merge remote-tracking branch 'origin/master' into 8088343.lifecycle
>  - 8088343

Avoiding the case where we transition from `FAILED` or `SUCCEEDED` to `CANCELLED` is good, and should fix the test failures (although I'll do some additional testing), but I can't help wondering if underlying problem is something else. The question I have is: _how_ do we get into a state where Worker.State of the Task one of the completion states (`FAILED` or `SUCCEEDED`), whereas the parent FutureTask is not completed, and so enters a canceled state. You can see this by instrumenting the code and calling `super.isCancelled()`.

I wonder if the custom service created by `TestServiceFactory` is causing, or contributing to, the problem?

modules/javafx.graphics/src/main/java/javafx/concurrent/Task.java line 1021:

> 1019:                 switch (getState()) {
> 1020:                 case FAILED:
> 1021:                 case SUCCEEDED:

Avoiding transitioning from a one of the completion states to canceled is a good thing. My question, though is: how did we get here? Is this masking some other problem?

modules/javafx.graphics/src/main/java/javafx/concurrent/Task.java line 1041:

> 1039:                     }
> 1040:                 });
> 1041:                 return rv.get();

This is ineffective since you don't wait for the runLater to execute (and waiting could lead to deadlock, so I wouldn't recommend that). I don't think there is anything better than unconditionally returning `flag` when not running on the FX app thread. That's what the current code does (and is what your proposed fix will do most of the time).

-------------

PR Review: https://git.openjdk.org/jfx/pull/1769#pullrequestreview-2773125312
PR Review Comment: https://git.openjdk.org/jfx/pull/1769#discussion_r2047294396
PR Review Comment: https://git.openjdk.org/jfx/pull/1769#discussion_r2047321781