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

[Michael Strauß]

Hi Jurgen,

starting an animation on a background thread would only be safe if the
object graph affected by the animation is not accessed on the
background thread after calling the play() method. Any access may
potentially corrupt internal state.

>From what I can see in your example, you're not doing anything with
TestView after starting the animation, so you should be good here.

>From an API perspective, I wonder if that makes it too easy to
introduce subtle bugs into an application. As a general principle, I
think an API should make it easy to do the right thing, and hard to do
the wrong thing. Allowing potentially unsafe method calls without some
explicit procedure (like wrapping the call in Platform.runLater) might
not be a good idea.

Maybe we should provide an API that allows developers to safely modify
an object graph on a background thread and call Platform.runLater
without worrying about concurrent modifications.

Something like this:

try (var scope = Platform.deferredScope()) {
    var updater = new Timeline(
       new KeyFrame(Duration.seconds(2.5), event ->
       {
          int maxMemory = ....;
          int usedMemory = ....;
          memoryLabel.setText(usedMemory + " MB / "+ maxMemory +" MB");
       })
    );

    // This call will only be dispatched after the deferred scope is closed:
    scope.runLater(updater::play);

    // Still safe to call in the deferred scope, wouldn't be safe if
    // Platform.runLater had been called before:
    memoryLabel.setText("N/A");
}


On Mon, Jan 22, 2024 at 11:57 AM Jurgen Doll <javafx at ivoryemr.co.za> wrote:
>
> Hi Michael
>
> It seems we are misunderstanding one another.
>
> Firstly I agree that the animation code itself must and always has run
> only on the FX thread.
>
> So for example in the following code:
>
>     new KeyFrame( Duration.seconds(2.5), event ->
>     {
>        int maxMemory = ....;
>        int usedMemory = ....;
>        memoryLabel.setText( usedMemory +" MB / "+ maxMemory +" MB" );
>     })
>
> The lambda part ALWAYS executes on the FX thread by default, no matter on
> which thread this KeyFrame is created.
>
>
> And we all agree that the following is ILLEGAL:
>
>     new KeyFrame( Duration.seconds(2.5), event ->
>     {
>        new Thread( () -> memoryLabel.setText( usedMemory +" MB / "+
> maxMemory +" MB" ) ).start();
>     })
>
>
> With the above clarified, what I'm contending is that the following can be
> excuted on any thread:
>
>     var updater = new Timeline
>     (
>        new KeyFrame( Duration.seconds(2.5), event ->
>        {
>           int maxMemory = ....;
>           int usedMemory = ....;
>           memoryLabel.setText( usedMemory +" MB / "+ maxMemory +" MB" );
>        })
>     );
>     updater.setCycleCount(Animation.INDEFINITE);
>     updater.play();
>
>
> The reason is because play, stop, and pause simply causes the Timeline to
> be added too or removed from an array in AbstractPrimaryTimer. If a pulse
> is busy executing so that the array is currently being accessed by the FX
> thread then a copy of the array is made and the addition/removal is made
> to the copy. (However there is a bug in this code that causes an NPE under
> certain conditions.)
>
> So when the API documentation says that it's asynchronous it means that
> the 'animation' execution may not yet have started/stopped when the call
> returns depending on when during the pulse the method was invoked. If play
> is invoked just before the next pulse then the 'animation' could be
> running, or if stop is invoked just as a pulse completes then it will not
> be executing. Otherwise the 'animation' will only actually be (not)
> executing in the next pulse.
>
> Hope this clarifies things.
>
> Regards
> Jurgen