HEADS-UP: Threading restriction for Animation play, pause, stop now enforced
Kevin Rushforth
kevin.rushforth at oracle.com
Fri Jan 19 19:32:40 UTC 2024
Hi Jurgen,
There is a very short window for making any changes like this that
require a CSR in JavaFX 22. If we were to do what you suggest, we would
need to:
1. File a new Enhancement request to:
-- revert JDK-8159048 (except for the doc changes about Animation being
run on the FX application thread, which is correct and useful even after
the revert)
-- update the documentation to indicate that the play/pause/stop methods
may be called on any thread
-- fix the implementation to wrap the implementation of play/pause/stop
in Platform.runLater, if not on the FX app thread
-- we may or may not also want to make the additional fix you suggest
in AbstractPrimaryTimer, but this might not be needed if we wrap the
call in a runLater, which I think should be done anyway
2. File a CSR for the spec changes for the above
All of this would need to be agreed upon and proposed within the next
few days to allow time for the CSR to be considered and approved while
we are still in RDP1. I would not consider such a change once we hit
RDP2 on Feb 1.
Before even considering this, I'd like to hear from Johan Vos, Jose
Pereda (who implemented JDK-8159048), Nir Lisker (who has done a lot of
work on animation), and Michael Strauß (who has proposed in JDK-8324219
to update the documentation to remove the reference to the fact that
stop is asynchronous), and any others who might have an interest. Note
that if we go down the route of reverting JDK-8159048, then we will
close JDK-8324219 as "not an issue".
If there is general agreement, then it would seem reasonable to shoot
for JavaFX 22. I note that it would be a low- risk change -- basically
going back to the behavior we have in JavaFX 21, which is the latest GA
release.
-- Kevin
On 1/19/2024 4:06 AM, Jurgen Doll wrote:
> Hi Kevin
>
> I was hoping that others would way in on this fix (PR #1167), but now
> that we're in RDP1 with no other input coming in I decided to looked
> into this matter again and have found that this is not the correct
> solution for the following two reasons:
>
> 1. The current solution doesn't actually fix the bug, but merely
> avoids it:
>
> Specifically with regards to bug JDK-8159048 which reports a NPE
> occuring under some conditions in AbstractMasterTimer (subsequently
> renamed to AbstractPrimaryTimer). The reporter suggests that this is
> as a result of some concurrent modification occurring and suggests a
> workaround of wrapping the start/stop animation code in a
> Platform.runLater() call.
>
> Looking at the AbstractPrimaryTimer code it is apparent that the
> original author made an effort to isolate array modifications from
> array access. However this code has a bug in it which then manifests
> as a NPE under the right timing conditions. So the correct solution is
> to make the array modification code more robust, as apposed to forcing
> changes to occur on a single thread.
>
> The safest (and proper) solution is to convert the two arrays
> ("receivers" and "animationTimers") to be CopyOnWriteArrayList(s)
> which is an ideal JDK data structure for this use case as it
> replicates the intended behavior of the current implementation. (I've
> tried this out and it does solve the NPE problem.)
>
> 2. The current solution is based on the misconception that start,
> stop, and pause must occur on the FX thread:
>
> Specifically with regards to the CSR JDK-8313378 which makes this claim.
>
> Firstly the Animation API says explicitly that these methods are
> asynchronous, which means that the thread that invokes these methods
> and the actual execution of the animation occurs on different threads,
> and looking at AbstractPrimaryTimer code it can be seen that this is
> indeed the case.
>
> Secondly JavaFX had tests, as noted in JDK-8314266, that have run
> reliably for years without invoking these methods on the FX thread.
> FWIW I've also had code and used libraries for years now, where these
> methods have been invoked on a background thread (for example while
> loading FXML) without issue.
>
>
> In conclusion then I request permission to submit a new PR containing
> the following changes:
>
> 1. Revert PR #1167 (if this is the appropriate place, however the test
> case will require it)
> 2. Changing the arrays in AbstractPrimaryTimer to be
> CopyOnWriteArrayList(s) and removing previously supporting array code.
> 3. Adding a test based on the one supplied in JDK-8159048 to check
> that a NPE isn't thrown anymore.
>
> Hope this meets with your approval.
> Regards,
> Jurgen
>
>
>> On 8/18/2023 4:17 PM, Kevin Rushforth wrote:
>> As a heads-up for app developers who use JavaFX animation (including
>> Animation, along with any subclasses, and AnimationTimer), a change
>> went into the JavaFX 22+5 build to enforce that the play, pause, and
>> stop methods must be called on the JavaFX Application thread.
>> Applications should have been doing that all along (else they would
>> have been subject to unpredictable errors), but for those who aren't
>> sure, you might want to take 22+5 for a spin and see if you have any
>> problems with your application. Please report them on the list if you
>> do.
>>
>> See JDK-8159048 [1] and CSR JDK-8313378 [2] for more information on
>> this change.
>>
>> Thanks.
>>
>> -- Kevin
>>
>> [1] https://bugs.openjdk.org/browse/JDK-8159048
>> [2] https://bugs.openjdk.org/browse/JDK-8313378
More information about the openjfx-dev
mailing list