HEADS-UP: Threading restriction for Animation play, pause, stop now enforced

[Jurgen Doll]

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