8088343: Intermittent unit test failure in javafx.concurrent.ServiceLifecycleTest #1769
Conversation
|
👋 Welcome back angorya! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
Webrevs
|
|
Reviewers: @kevinrushforth @arapte /reviewers 2 |
|
@kevinrushforth |
There was a problem hiding this comment.
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?
| switch (getState()) { | ||
| case FAILED: | ||
| case SUCCEEDED: |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I think the variations in the test timing uncovered this issue. I don't think it's a problem with the execution of the task, but rather with the reporting of its final state.
| break; | ||
| } | ||
| }); | ||
| return rv.get(); |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
good point!
In this particular case, returning true from cancel() when state will not be changed is acceptable.
The code should not set the
Task.statevalue toCANCELLEDif the said task is alreadySUCCEEDEDorFAILED.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.Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1769/head:pull/1769$ git checkout pull/1769Update a local copy of the PR:
$ git checkout pull/1769$ git pull https://git.openjdk.org/jfx.git pull/1769/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1769View PR using the GUI difftool:
$ git pr show -t 1769Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1769.diff
Using Webrev
Link to Webrev Comment