On Fri, 11 Apr 2025 17:06:38 GMT, Andy Goryachev <[email protected]> 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
