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

[John Hendrikx]

I think that we should realize that almost all JavaFX public class 
(controls, properties, listeners, etc) are not thread safe. This means 
that they must be manipulated by a single thread at all times.  This can 
be the FX thread, but it can also be another thread, as long as there 
never are two threads manipulating these JavaFX components 
concurrently.  So it is fine to construct controls on one thread, as 
long as that thread is finished when they're handed over to the FX 
thread by making them visible (or by starting animations!).

The problem with using animations in controls (and starting them 
immediately) is that this brings a second thread into play: the FX 
thread -- this may not be what you want when you are constructing 
controls "offline".  The animation may be changing properties, while the 
offline thread is changing the same properties, which can lead to all 
kinds of issues, like listeners running concurrently for the same property.

I therefore think that a control that is starting animations before it 
is showing (attached to a scene, and is visible) is fundamentally 
flawed.  Controls should only start their animations when showing and 
must terminate them when no longer showing.  In other words, controls 
should be written to prevent the FX thread accessing them (via 
animation), unless they're sure that's currently safe (it's safe when 
they're currently showing).

So even though we can make a call like "play" safe to call on another 
thread, realize that by calling `play`, you are starting a process where 
the FX thread may start modifying the control at any time, even while 
you are still constructing the control on another thread.

Control authors need to fix this IMHO.  Tooltip was fixed by using a 
TreeShowingProperty at the time, which is similar to what I'm suggesting 
that animations should be started/stopped based on the showing status of 
your control.

This can be as easy as:

       this.sceneProperty()
           .flatMap(Scene::windowProperty)
           .flatMap(Window::showingProperty)
           .orElse(false)
           .subscribe(showing -> {
                   if (showing) animation.play();
                   else animation.stop();
           });

--John


On 29/08/2023 11:51, Jurgen Doll wrote:
> Thanks for the heads-up Kevin,
>
> I gave it a spin and found that generally because I use Task to load 
> my fxml views I had problems.
>
> Some of these I could resolve by wrapping the offending line with 
> runlater in the fxml initialise method. This reminded me though of the 
> days when Tooltips had to be wrapped as well and it felt wrong because 
> generally a view may be constructed and modified on any thread as long 
> as it's not yet attached to a Scene in a Window that is showing.
>
> This is highlighted further because I also have some third party 
> controls and libraries that are being initialized as part of the view, 
> which now just crash my application. This means that I cannot 
> instantiate these controls or libraries on any thread I want but have 
> to make sure its done on the FX thread, even though they're not 
> attached to a Scene yet.
>
> As a possible solution I was wondering since the Animation API says 
> that calls to play() and stop() are asynchronous if it wouldn't then 
> be valid to instead of throwing an exception, if the call to it isn't 
> being made on the FX thread, that it rather be delegated to the FX 
> thread with for example something like:
>
> public abstract class Animation {
>     public void play() {
>         if ( Platform.isFxApplicationThread() ) playFX();
>         else Platform.runLater( () -> playFX() );
>     }
>
>     private void playFX() {
>         // previous play() code
>     }
> }
>
> This would then prevent the NPE errors that sometimes occur but not 
> put a burden on the existing code in the wild and allow views to be 
> loaded with Task call() without worries.
>
> Thanks, 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