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
