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

[Kevin Rushforth]

If you mean the changes proposed in Draft PR 
https://github.com/openjdk/jfx/pull/1349 those are too-intrusive for 
where we are in JavaFX 22. I see no chance that we will get agreement on 
the approach and be able to finish the code review and CSR in the next 4 
business days.


If we later get general agreement that the changes you propose are 
needed, we could consider them for JavaFX 23. For 22, I think we're down 
to either leaving it as is or reverting the thread checks and wrapping 
the implementation in a runLater.


I don't think having the state change be asynchronous if you run it on a 
thread other than the application thread will be an issue. And, as I 
pointed out earlier, it is documented that the animation might not start 
or stop right away.


-- Kevin


On 1/25/2024 11:07 AM, Michael Strauß wrote:

Hi Kevin,

have you considered my proposal, which is basically the same approach:
it uses runLater internally to dispatch the interaction with
AbstractPrimaryTimer (instead of trying to make AbstractPrimaryTimer
work under concurrent access).

But from the point of view of the caller, the API works just as it
worked before: all state changes of the Animation class are instantly
visible, there is no surprising change as it would be with what you're
currently proposing.

With my proposal, play() can be called on a background thread, and the
behavior of the Animation class remains the same. If you're reading
getStatus() after calling play(), you will always see that the
animation is currently running (even if internally, the animation
hasn't yet been added to the primary timer).


On Thu, Jan 25, 2024 at 6:30 PM Kevin Rushforth
 wrote:

And that's why the "be careful" option is not a satisfying solution.

Let's proceed with the option to change the implementation of play/pause/stop/start to do 
the work in a runLater, and change the docs accordingly. It's the only feasible option 
for JavaFX 22 (other than "do nothing"), and I also think it is a good choice. 
If we later want to evolve it for thread-safety, which I'm not convinced that we do, we 
can consider that for a future release.

-- Kevin

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

[Michael Strauß]

Hi Kevin,

have you considered my proposal, which is basically the same approach:
it uses runLater internally to dispatch the interaction with
AbstractPrimaryTimer (instead of trying to make AbstractPrimaryTimer
work under concurrent access).

But from the point of view of the caller, the API works just as it
worked before: all state changes of the Animation class are instantly
visible, there is no surprising change as it would be with what you're
currently proposing.

With my proposal, play() can be called on a background thread, and the
behavior of the Animation class remains the same. If you're reading
getStatus() after calling play(), you will always see that the
animation is currently running (even if internally, the animation
hasn't yet been added to the primary timer).


On Thu, Jan 25, 2024 at 6:30 PM Kevin Rushforth
 wrote:
>
> And that's why the "be careful" option is not a satisfying solution.
>
> Let's proceed with the option to change the implementation of 
> play/pause/stop/start to do the work in a runLater, and change the docs 
> accordingly. It's the only feasible option for JavaFX 22 (other than "do 
> nothing"), and I also think it is a good choice. If we later want to evolve 
> it for thread-safety, which I'm not convinced that we do, we can consider 
> that for a future release.
>
> -- Kevin

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

[Michael Strauß]

Hi John, the threading restrictions are now removed.

What I mean by those seemingly contradictory stratements is the following:
This change doesn't make Animation inherently thread-safe, so if you
configure the animation on thread A, you can't just call play() on
thread B and expect it to work.
What the change *does* allow you to do, is configure the animation on
background thread A, and then call play() on the same background
thread.



On Thu, Jan 25, 2024 at 12:19 PM John Hendrikx  wrote:
>
> Hi Michael,
>
> I'm not quite sure I see the point of this change.  The PR did not
> remove the threading restrictions on play/stop.
>
> I'm also confused by the seemingly contradictory statements:
>
> - this proposal does NOT allow Animation.play/stop/etc. to be "called on
> any thread"
> - It merely removes the requirement that these methods must be called on
> the FX thread
>
> What does that mean exactly?
>
> Also, I think the "asynchronous" wording may apply to property changes
> and action handlers that run at time index 0.  These surely won't be ran
> synchronous when calling "play", and I'm pretty sure they never were.
>
> --John
>
> On 24/01/2024 22:39, Michael Strauß wrote:
> > Note: this proposal does NOT allow Animation.play/stop/etc. to be
> > "called on any thread" as mentioned in JDK-8324658 [0].
> >
> > It merely removes the requirement that these methods must be called on
> > the FX thread, but this doesn't make the class inherently thread-safe.
> > That is an important distinction to proposals that call for posting
> > the play/stop calls directly to the FX thread.
> >
> >
> > [0] https://bugs.openjdk.org/browse/JDK-8324658
> >
> >
> > On Wed, Jan 24, 2024 at 10:30 PM Michael Strauß  
> > wrote:
> >> Here's another option, which I have implemented as a proof of concept [0]:
> >>
> >> The play/stop/etc. methods are currently specified to be
> >> "asynchronous". This language should be removed, such that the methods
> >> will be implied to execute synchronously from the point of view of the
> >> caller (like every method that doesn't specify anything to the
> >> contrary).
> >>
> >> All changes to observable state will remain instantly visible for the
> >> calling thread. However, internally, interactions with
> >> AbstractPrimaryTimer are posted to the FX thread if necessary. This is
> >> not an unsurprising change, since the callback from the FX thread was
> >> always occuring at an unspecified time in the future.
> >>
> >> To make this work, AbstractPrimaryTimer::pause/resume/nanos will have
> >> to be synchronized to ensure field visibility across threads.
> >> In the Animation class, interactions with AbstractPrimaryTimer will be
> >> encapsulated in the new nested class AnimationPulseReceiver, which
> >> also deduplicates redundant interactions with AbstractPrimaryTimer.
> >> For example, repeatedly calling start() and stop() in quick succession
> >> may require just a single interaction with AbstractPrimaryTimer in the
> >> future (if we ended up in the running state), or no interaction at all
> >> (if we ended up in the stopped state).
> >>
> >>
> >> [0] https://github.com/openjdk/jfx/pull/1349

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

[Kevin Rushforth]

And that's why the "be careful" option is not a satisfying solution.

Let's proceed with the option to change the implementation of 
play/pause/stop/start to do the work in a runLater, and change the docs 
accordingly. It's the only feasible option for JavaFX 22 (other than "do 
nothing"), and I also think it is a good choice. If we later want to 
evolve it for thread-safety, which I'm not convinced that we do, we can 
consider that for a future release.


-- Kevin


On 1/25/2024 5:46 AM, Nir Lisker wrote:


Option 3 is basically document it as "be careful" and remove the
thread restriction recently introduced, am I correct?


Yes :)

IMHO that can simply can't work at all, because this is what
(theoretically) can happen:
1. Non-FX thread starts animation
 - Fields are manipulated in AbstractPrimaryTimer
 - There is no synchronization, so no guarantee anything is
flushed (it may all reside in CPU caches)


2. FX Thread becomes involved to actually process the animation
 - Sees partially flushed state fields, and acts on garbage
information (ie. number of animations > 0, but array contains only
null's)
There just is no safe way to do this in without synchronization or
having everything immutable (and this extends to references to
"thread safe" structures).

What about something like a PauseTransition that doesn't 
manipulate properties?


On Thu, Jan 25, 2024 at 11:02 AM John Hendrikx 
 wrote:



On 24/01/2024 22:06, Nir Lisker wrote:

This is the ballpark of what I meant with "the downside might be
some surprise when these methods behave differently".

That can be documented, and basically already is (because that is
what the "asynchronous" is alluding to, the fact that after
calling "play" the state will change asynchronously).


The example you give will only show a potential change if 'play'
is not called from the FX thread. In such a case, what's the
chance that this is an undeliberate misuse vs. an informed use?
This brings me again to: what is the use case for running an
animation from a background thread?


The only possible use case I can think of is when using FX
properties stand-alone (ie. only using javafx.base), without any
FX thread involvement.  Even in that case though one has to
remember that properties themselves are not thread safe either. 
Any "animation" would need to be done on the same thread that is
manipulating properties.

However, Animation and Timeline simply can't work in such
scenario's, as they're tied to javafx.graphics, to the FX system
being started, and the FX thread.  For such a use case you'd need
to write your own system, or provide the option of specifying an
Executor for Animations/Timelines.



In your simple example, listening on the Status property would
work. Also, while runLater makes no guarantees, I've never seen a
non-negligible delay in its execution, so how surprising is it
really going to be?

If you want to be able to run an animation from a background
thread with no race conditions, why not opt for option 3?


Option 3 is basically document it as "be careful" and remove the
thread restriction recently introduced, am I correct?

IMHO that can simply can't work at all, because this is what
(theoretically) can happen:

1. Non-FX thread starts animation
 - Fields are manipulated in AbstractPrimaryTimer
 - There is no synchronization, so no guarantee anything is
flushed (it may all reside in CPU caches)

2. FX Thread becomes involved to actually process the animation
 - Sees partially flushed state fields, and acts on garbage
information (ie. number of animations > 0, but array contains only
null's)

There just is no safe way to do this in without synchronization or
having everything immutable (and this extends to references to
"thread safe" structures).

--John



And option 1 is also new and surprising because now code that
worked (or pretended to work) throws, which ruins properly
written code (with respect to multithreading), or exposes a bug.

On Wed, Jan 24, 2024 at 9:04 PM Michael Strauß
 wrote:

I am not in favor of option 2 if the implementation was
simply "wrap
the implementation in runLater", as this would be a
surprising change.
Consider the following code:

    var transition = new FadeTransition();
    transition.play();

    // Will always print "RUNNING" currently, but might print
"STOPPED"
    // if we go with the proposed change:
    System.out.println(transition.getStatus());

Regardless of the race condition in AbstractPrimaryTimer,
this code
always seemed to be working quite fine (albeit
superficially), because
the play/stop/etc. methods 

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

[Nir Lisker]

>
> Option 3 is basically document it as "be careful" and remove the thread
> restriction recently introduced, am I correct?
>

Yes :)

IMHO that can simply can't work at all, because this is what
> (theoretically) can happen:
> 1. Non-FX thread starts animation
>  - Fields are manipulated in AbstractPrimaryTimer
>  - There is no synchronization, so no guarantee anything is flushed
> (it may all reside in CPU caches)


> 2. FX Thread becomes involved to actually process the animation
>  - Sees partially flushed state fields, and acts on garbage
> information (ie. number of animations > 0, but array contains only null's)
> There just is no safe way to do this in without synchronization or having
> everything immutable (and this extends to references to "thread safe"
> structures).


What about something like a PauseTransition that doesn't
manipulate properties?

On Thu, Jan 25, 2024 at 11:02 AM John Hendrikx 
wrote:

>
> On 24/01/2024 22:06, Nir Lisker wrote:
>
> This is the ballpark of what I meant with "the downside might be some
> surprise when these methods behave differently".
>
> That can be documented, and basically already is (because that is what the
> "asynchronous" is alluding to, the fact that after calling "play" the state
> will change asynchronously).
>
>
> The example you give will only show a potential change if 'play' is not
> called from the FX thread. In such a case, what's the chance that this is
> an undeliberate misuse vs. an informed use? This brings me again to: what
> is the use case for running an animation from a background thread?
>
> The only possible use case I can think of is when using FX properties
> stand-alone (ie. only using javafx.base), without any FX thread
> involvement.  Even in that case though one has to remember that properties
> themselves are not thread safe either.  Any "animation" would need to be
> done on the same thread that is manipulating properties.
>
> However, Animation and Timeline simply can't work in such scenario's, as
> they're tied to javafx.graphics, to the FX system being started, and the FX
> thread.  For such a use case you'd need to write your own system, or
> provide the option of specifying an Executor for Animations/Timelines.
>
> In your simple example, listening on the Status property would work. Also,
> while runLater makes no guarantees, I've never seen a non-negligible delay
> in its execution, so how surprising is it really going to be?
>
> If you want to be able to run an animation from a background thread with
> no race conditions, why not opt for option 3?
>
> Option 3 is basically document it as "be careful" and remove the thread
> restriction recently introduced, am I correct?
>
> IMHO that can simply can't work at all, because this is what
> (theoretically) can happen:
>
> 1. Non-FX thread starts animation
>  - Fields are manipulated in AbstractPrimaryTimer
>  - There is no synchronization, so no guarantee anything is flushed
> (it may all reside in CPU caches)
>
> 2. FX Thread becomes involved to actually process the animation
>  - Sees partially flushed state fields, and acts on garbage
> information (ie. number of animations > 0, but array contains only null's)
>
> There just is no safe way to do this in without synchronization or having
> everything immutable (and this extends to references to "thread safe"
> structures).
>
> --John
>
>
> And option 1 is also new and surprising because now code that worked (or
> pretended to work) throws, which ruins properly written code (with respect
> to multithreading), or exposes a bug.
>
> On Wed, Jan 24, 2024 at 9:04 PM Michael Strauß 
> wrote:
>
>> I am not in favor of option 2 if the implementation was simply "wrap
>> the implementation in runLater", as this would be a surprising change.
>> Consider the following code:
>>
>> var transition = new FadeTransition();
>> transition.play();
>>
>> // Will always print "RUNNING" currently, but might print "STOPPED"
>> // if we go with the proposed change:
>> System.out.println(transition.getStatus());
>>
>> Regardless of the race condition in AbstractPrimaryTimer, this code
>> always seemed to be working quite fine (albeit superficially), because
>> the play/stop/etc. methods change that status of the animation as one
>> would expect.
>>
>> You are proposing to change that, such that calling these methods will
>> not predictably change the status of the animation. Instead, these
>> methods now act more like "requests" that may be fulfilled at some
>> later point in time, rather than statements of fact.
>> This is not a change that I think we should be doing on an ad-hoc
>> basis, since the same considerations potentially apply for many
>> methods in many places.
>>
>> If we were to allow calling play/stop/etc. on a background thread, I
>> would be in favor of keeping the semantics that these methods
>> instantly and predictably affect the status of the animation. Only the
>> internal operation 

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

[John Hendrikx]

Hi Michael,

I'm not quite sure I see the point of this change.  The PR did not 
remove the threading restrictions on play/stop.


I'm also confused by the seemingly contradictory statements:

- this proposal does NOT allow Animation.play/stop/etc. to be "called on 
any thread"
- It merely removes the requirement that these methods must be called on 
the FX thread


What does that mean exactly?

Also, I think the "asynchronous" wording may apply to property changes 
and action handlers that run at time index 0.  These surely won't be ran 
synchronous when calling "play", and I'm pretty sure they never were.


--John

On 24/01/2024 22:39, Michael Strauß wrote:

Note: this proposal does NOT allow Animation.play/stop/etc. to be
"called on any thread" as mentioned in JDK-8324658 [0].

It merely removes the requirement that these methods must be called on
the FX thread, but this doesn't make the class inherently thread-safe.
That is an important distinction to proposals that call for posting
the play/stop calls directly to the FX thread.


[0] https://bugs.openjdk.org/browse/JDK-8324658


On Wed, Jan 24, 2024 at 10:30 PM Michael Strauß  wrote:

Here's another option, which I have implemented as a proof of concept [0]:

The play/stop/etc. methods are currently specified to be
"asynchronous". This language should be removed, such that the methods
will be implied to execute synchronously from the point of view of the
caller (like every method that doesn't specify anything to the
contrary).

All changes to observable state will remain instantly visible for the
calling thread. However, internally, interactions with
AbstractPrimaryTimer are posted to the FX thread if necessary. This is
not an unsurprising change, since the callback from the FX thread was
always occuring at an unspecified time in the future.

To make this work, AbstractPrimaryTimer::pause/resume/nanos will have
to be synchronized to ensure field visibility across threads.
In the Animation class, interactions with AbstractPrimaryTimer will be
encapsulated in the new nested class AnimationPulseReceiver, which
also deduplicates redundant interactions with AbstractPrimaryTimer.
For example, repeatedly calling start() and stop() in quick succession
may require just a single interaction with AbstractPrimaryTimer in the
future (if we ended up in the running state), or no interaction at all
(if we ended up in the stopped state).


[0] https://github.com/openjdk/jfx/pull/1349

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

[John Hendrikx]

On 24/01/2024 22:06, Nir Lisker wrote:
This is the ballpark of what I meant with "the downside might be some 
surprise when these methods behave differently".
That can be documented, and basically already is (because that is what 
the "asynchronous" is alluding to, the fact that after calling "play" 
the state will change asynchronously).


The example you give will only show a potential change if 'play' is 
not called from the FX thread. In such a case, what's the chance that 
this is an undeliberate misuse vs. an informed use? This brings me 
again to: what is the use case for running an animation from a 
background thread?


The only possible use case I can think of is when using FX properties 
stand-alone (ie. only using javafx.base), without any FX thread 
involvement.  Even in that case though one has to remember that 
properties themselves are not thread safe either. Any "animation" would 
need to be done on the same thread that is manipulating properties.


However, Animation and Timeline simply can't work in such scenario's, as 
they're tied to javafx.graphics, to the FX system being started, and the 
FX thread.  For such a use case you'd need to write your own system, or 
provide the option of specifying an Executor for Animations/Timelines.



In your simple example, listening on the Status property would work. 
Also, while runLater makes no guarantees, I've never seen a 
non-negligible delay in its execution, so how surprising is it really 
going to be?


If you want to be able to run an animation from a background thread 
with no race conditions, why not opt for option 3?


Option 3 is basically document it as "be careful" and remove the thread 
restriction recently introduced, am I correct?


IMHO that can simply can't work at all, because this is what 
(theoretically) can happen:


1. Non-FX thread starts animation
 - Fields are manipulated in AbstractPrimaryTimer
 - There is no synchronization, so no guarantee anything is flushed 
(it may all reside in CPU caches)


2. FX Thread becomes involved to actually process the animation
 - Sees partially flushed state fields, and acts on garbage 
information (ie. number of animations > 0, but array contains only null's)


There just is no safe way to do this in without synchronization or 
having everything immutable (and this extends to references to "thread 
safe" structures).


--John



And option 1 is also new and surprising because now code that worked 
(or pretended to work) throws, which ruins properly written code (with 
respect to multithreading), or exposes a bug.


On Wed, Jan 24, 2024 at 9:04 PM Michael Strauß 
 wrote:


I am not in favor of option 2 if the implementation was simply "wrap
the implementation in runLater", as this would be a surprising change.
Consider the following code:

    var transition = new FadeTransition();
    transition.play();

    // Will always print "RUNNING" currently, but might print
"STOPPED"
    // if we go with the proposed change:
    System.out.println(transition.getStatus());

Regardless of the race condition in AbstractPrimaryTimer, this code
always seemed to be working quite fine (albeit superficially), because
the play/stop/etc. methods change that status of the animation as one
would expect.

You are proposing to change that, such that calling these methods will
not predictably change the status of the animation. Instead, these
methods now act more like "requests" that may be fulfilled at some
later point in time, rather than statements of fact.
This is not a change that I think we should be doing on an ad-hoc
basis, since the same considerations potentially apply for many
methods in many places.

If we were to allow calling play/stop/etc. on a background thread, I
would be in favor of keeping the semantics that these methods
instantly and predictably affect the status of the animation. Only the
internal operation of adding the animation to AbstractPrimaryTimer
should be posted to the FX thread. If that is not possible, this
suggests to me that we should choose option 1. Introducing new and
surprising semantics to an existing API is not the way to go.


On Wed, Jan 24, 2024 at 7:27 PM Nir Lisker  wrote:
>
> These are the options I see as reasonable:
>
> 1. Document that these methods *must* be run on the FX thread
and throw an exception otherwise. This leaves it to the
responsibility of the user. However, this will cause the backwards
compatibility problems that Jugen brought up. As a side note, we
do this in other methods already, but I always questioned why let
the developer do something illegal - if there's only one execution
path, force it.
> 2. Document that these methods *are* run on the FX thread (the
user doesn't need to do anything) and change the implementation to
call runLater(...) internally unless they are 

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

[Michael Strauß]

Note: this proposal does NOT allow Animation.play/stop/etc. to be
"called on any thread" as mentioned in JDK-8324658 [0].

It merely removes the requirement that these methods must be called on
the FX thread, but this doesn't make the class inherently thread-safe.
That is an important distinction to proposals that call for posting
the play/stop calls directly to the FX thread.


[0] https://bugs.openjdk.org/browse/JDK-8324658


On Wed, Jan 24, 2024 at 10:30 PM Michael Strauß  wrote:
>
> Here's another option, which I have implemented as a proof of concept [0]:
>
> The play/stop/etc. methods are currently specified to be
> "asynchronous". This language should be removed, such that the methods
> will be implied to execute synchronously from the point of view of the
> caller (like every method that doesn't specify anything to the
> contrary).
>
> All changes to observable state will remain instantly visible for the
> calling thread. However, internally, interactions with
> AbstractPrimaryTimer are posted to the FX thread if necessary. This is
> not an unsurprising change, since the callback from the FX thread was
> always occuring at an unspecified time in the future.
>
> To make this work, AbstractPrimaryTimer::pause/resume/nanos will have
> to be synchronized to ensure field visibility across threads.
> In the Animation class, interactions with AbstractPrimaryTimer will be
> encapsulated in the new nested class AnimationPulseReceiver, which
> also deduplicates redundant interactions with AbstractPrimaryTimer.
> For example, repeatedly calling start() and stop() in quick succession
> may require just a single interaction with AbstractPrimaryTimer in the
> future (if we ended up in the running state), or no interaction at all
> (if we ended up in the stopped state).
>
>
> [0] https://github.com/openjdk/jfx/pull/1349

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

[Michael Strauß]

Here's another option, which I have implemented as a proof of concept [0]:

The play/stop/etc. methods are currently specified to be
"asynchronous". This language should be removed, such that the methods
will be implied to execute synchronously from the point of view of the
caller (like every method that doesn't specify anything to the
contrary).

All changes to observable state will remain instantly visible for the
calling thread. However, internally, interactions with
AbstractPrimaryTimer are posted to the FX thread if necessary. This is
not an unsurprising change, since the callback from the FX thread was
always occuring at an unspecified time in the future.

To make this work, AbstractPrimaryTimer::pause/resume/nanos will have
to be synchronized to ensure field visibility across threads.
In the Animation class, interactions with AbstractPrimaryTimer will be
encapsulated in the new nested class AnimationPulseReceiver, which
also deduplicates redundant interactions with AbstractPrimaryTimer.
For example, repeatedly calling start() and stop() in quick succession
may require just a single interaction with AbstractPrimaryTimer in the
future (if we ended up in the running state), or no interaction at all
(if we ended up in the stopped state).


[0] https://github.com/openjdk/jfx/pull/1349


On Wed, Jan 24, 2024 at 7:27 PM Nir Lisker  wrote:
>
> These are the options I see as reasonable:
>
> 1. Document that these methods *must* be run on the FX thread and throw an 
> exception otherwise. This leaves it to the responsibility of the user. 
> However, this will cause the backwards compatibility problems that Jugen 
> brought up. As a side note, we do this in other methods already, but I always 
> questioned why let the developer do something illegal - if there's only one 
> execution path, force it.
> 2. Document that these methods *are* run on the FX thread (the user doesn't 
> need to do anything) and change the implementation to call runLater(...) 
> internally unless they are already on the FX thread. This will be backwards 
> compatible for the most part (see option 3). The downside might be some 
> surprise when these methods behave differently.
> 3. Document that it's *recommended* that these methods be run on the FX 
> thread and let the user be responsible for the threading. We can explain that 
> manipulating nodes that are attached to an active scenegraph should be 
> avoided.
>
> I prefer option 2 over 1 regardless of the backwards compatibility issue 
> even, but would like to know if I'm missing something here because in theory 
> this change could be done to any "must run on the FX thread" method and I 
> question why the user had the option to get an exception.
> Option 3 is risky and I wager a guess that it will be used wrongly more often 
> than not. It does allow some (what I would call) valid niche uses. I never 
> did it.
>

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

[Nir Lisker]

This is the ballpark of what I meant with "the downside might be some
surprise when these methods behave differently".

The example you give will only show a potential change if 'play' is not
called from the FX thread. In such a case, what's the chance that this is
an undeliberate misuse vs. an informed use? This brings me again to: what
is the use case for running an animation from a background thread?

In your simple example, listening on the Status property would work. Also,
while runLater makes no guarantees, I've never seen a non-negligible delay
in its execution, so how surprising is it really going to be?

If you want to be able to run an animation from a background thread with no
race conditions, why not opt for option 3?

And option 1 is also new and surprising because now code that worked (or
pretended to work) throws, which ruins properly written code (with respect
to multithreading), or exposes a bug.

On Wed, Jan 24, 2024 at 9:04 PM Michael Strauß 
wrote:

> I am not in favor of option 2 if the implementation was simply "wrap
> the implementation in runLater", as this would be a surprising change.
> Consider the following code:
>
> var transition = new FadeTransition();
> transition.play();
>
> // Will always print "RUNNING" currently, but might print "STOPPED"
> // if we go with the proposed change:
> System.out.println(transition.getStatus());
>
> Regardless of the race condition in AbstractPrimaryTimer, this code
> always seemed to be working quite fine (albeit superficially), because
> the play/stop/etc. methods change that status of the animation as one
> would expect.
>
> You are proposing to change that, such that calling these methods will
> not predictably change the status of the animation. Instead, these
> methods now act more like "requests" that may be fulfilled at some
> later point in time, rather than statements of fact.
> This is not a change that I think we should be doing on an ad-hoc
> basis, since the same considerations potentially apply for many
> methods in many places.
>
> If we were to allow calling play/stop/etc. on a background thread, I
> would be in favor of keeping the semantics that these methods
> instantly and predictably affect the status of the animation. Only the
> internal operation of adding the animation to AbstractPrimaryTimer
> should be posted to the FX thread. If that is not possible, this
> suggests to me that we should choose option 1. Introducing new and
> surprising semantics to an existing API is not the way to go.
>
>
> On Wed, Jan 24, 2024 at 7:27 PM Nir Lisker  wrote:
> >
> > These are the options I see as reasonable:
> >
> > 1. Document that these methods *must* be run on the FX thread and throw
> an exception otherwise. This leaves it to the responsibility of the user.
> However, this will cause the backwards compatibility problems that Jugen
> brought up. As a side note, we do this in other methods already, but I
> always questioned why let the developer do something illegal - if there's
> only one execution path, force it.
> > 2. Document that these methods *are* run on the FX thread (the user
> doesn't need to do anything) and change the implementation to call
> runLater(...) internally unless they are already on the FX thread. This
> will be backwards compatible for the most part (see option 3). The downside
> might be some surprise when these methods behave differently.
> > 3. Document that it's *recommended* that these methods be run on the FX
> thread and let the user be responsible for the threading. We can explain
> that manipulating nodes that are attached to an active scenegraph should be
> avoided.
> >
> > I prefer option 2 over 1 regardless of the backwards compatibility issue
> even, but would like to know if I'm missing something here because in
> theory this change could be done to any "must run on the FX thread" method
> and I question why the user had the option to get an exception.
> > Option 3 is risky and I wager a guess that it will be used wrongly more
> often than not. It does allow some (what I would call) valid niche uses. I
> never did it.
>

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

[Kevin Rushforth]

The point you raise is one reason why I wouldn't advocate doing this in 
other places, e.g., for scene graph updates, which do need to run 
synchronously.


I note that the Animation docs currently state that these operations are 
asynchronous (and yes, I know you were proposing the change this). So 
while it might be surprising to some apps, it is within the current 
spec, and I'm not sure it is any more surprising than the exception we 
now throw in JavaFX 22.


If we could safely do what you suggest, and just wrap the part that 
touches the AbstractPrimaryTimer, that might be the best option. I don't 
know if that is feasible, though, especially going from a running state 
to a stopped or paused state.


-- Kevin


On 1/24/2024 11:04 AM, Michael Strauß wrote:

I am not in favor of option 2 if the implementation was simply "wrap
the implementation in runLater", as this would be a surprising change.
Consider the following code:

 var transition = new FadeTransition();
 transition.play();

 // Will always print "RUNNING" currently, but might print "STOPPED"
 // if we go with the proposed change:
 System.out.println(transition.getStatus());

Regardless of the race condition in AbstractPrimaryTimer, this code
always seemed to be working quite fine (albeit superficially), because
the play/stop/etc. methods change that status of the animation as one
would expect.

You are proposing to change that, such that calling these methods will
not predictably change the status of the animation. Instead, these
methods now act more like "requests" that may be fulfilled at some
later point in time, rather than statements of fact.
This is not a change that I think we should be doing on an ad-hoc
basis, since the same considerations potentially apply for many
methods in many places.

If we were to allow calling play/stop/etc. on a background thread, I
would be in favor of keeping the semantics that these methods
instantly and predictably affect the status of the animation. Only the
internal operation of adding the animation to AbstractPrimaryTimer
should be posted to the FX thread. If that is not possible, this
suggests to me that we should choose option 1. Introducing new and
surprising semantics to an existing API is not the way to go.


On Wed, Jan 24, 2024 at 7:27 PM Nir Lisker  wrote:

These are the options I see as reasonable:

1. Document that these methods *must* be run on the FX thread and throw an 
exception otherwise. This leaves it to the responsibility of the user. However, 
this will cause the backwards compatibility problems that Jugen brought up. As 
a side note, we do this in other methods already, but I always questioned why 
let the developer do something illegal - if there's only one execution path, 
force it.
2. Document that these methods *are* run on the FX thread (the user doesn't 
need to do anything) and change the implementation to call runLater(...) 
internally unless they are already on the FX thread. This will be backwards 
compatible for the most part (see option 3). The downside might be some 
surprise when these methods behave differently.
3. Document that it's *recommended* that these methods be run on the FX thread 
and let the user be responsible for the threading. We can explain that 
manipulating nodes that are attached to an active scenegraph should be avoided.

I prefer option 2 over 1 regardless of the backwards compatibility issue even, but would 
like to know if I'm missing something here because in theory this change could be done to 
any "must run on the FX thread" method and I question why the user had the 
option to get an exception.
Option 3 is risky and I wager a guess that it will be used wrongly more often 
than not. It does allow some (what I would call) valid niche uses. I never did 
it.

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

[Michael Strauß]

I am not in favor of option 2 if the implementation was simply "wrap
the implementation in runLater", as this would be a surprising change.
Consider the following code:

var transition = new FadeTransition();
transition.play();

// Will always print "RUNNING" currently, but might print "STOPPED"
// if we go with the proposed change:
System.out.println(transition.getStatus());

Regardless of the race condition in AbstractPrimaryTimer, this code
always seemed to be working quite fine (albeit superficially), because
the play/stop/etc. methods change that status of the animation as one
would expect.

You are proposing to change that, such that calling these methods will
not predictably change the status of the animation. Instead, these
methods now act more like "requests" that may be fulfilled at some
later point in time, rather than statements of fact.
This is not a change that I think we should be doing on an ad-hoc
basis, since the same considerations potentially apply for many
methods in many places.

If we were to allow calling play/stop/etc. on a background thread, I
would be in favor of keeping the semantics that these methods
instantly and predictably affect the status of the animation. Only the
internal operation of adding the animation to AbstractPrimaryTimer
should be posted to the FX thread. If that is not possible, this
suggests to me that we should choose option 1. Introducing new and
surprising semantics to an existing API is not the way to go.


On Wed, Jan 24, 2024 at 7:27 PM Nir Lisker  wrote:
>
> These are the options I see as reasonable:
>
> 1. Document that these methods *must* be run on the FX thread and throw an 
> exception otherwise. This leaves it to the responsibility of the user. 
> However, this will cause the backwards compatibility problems that Jugen 
> brought up. As a side note, we do this in other methods already, but I always 
> questioned why let the developer do something illegal - if there's only one 
> execution path, force it.
> 2. Document that these methods *are* run on the FX thread (the user doesn't 
> need to do anything) and change the implementation to call runLater(...) 
> internally unless they are already on the FX thread. This will be backwards 
> compatible for the most part (see option 3). The downside might be some 
> surprise when these methods behave differently.
> 3. Document that it's *recommended* that these methods be run on the FX 
> thread and let the user be responsible for the threading. We can explain that 
> manipulating nodes that are attached to an active scenegraph should be 
> avoided.
>
> I prefer option 2 over 1 regardless of the backwards compatibility issue 
> even, but would like to know if I'm missing something here because in theory 
> this change could be done to any "must run on the FX thread" method and I 
> question why the user had the option to get an exception.
> Option 3 is risky and I wager a guess that it will be used wrongly more often 
> than not. It does allow some (what I would call) valid niche uses. I never 
> did it.

Re: [External] : Re: HEADS-UP: Threading restriction for Animation play, pause, stop now enforced USE CASE

[Kevin Rushforth]

Thanks!

I just filed https://bugs.openjdk.org/browse/JDK-8324658

It should mostly revert JDK-8159048 (although the sentence added to the 
class docs about animations running on the JavaFX app thread is still 
valid), and some of the unit tests might still be valid.


-- Kevin

On 1/24/2024 10:50 AM, Nir Lisker wrote:
If there's an agreement, I can do it tomorrow. It will effectively 
revert JDK-8159048 and supersede JDK-8324219.


On Wed, Jan 24, 2024 at 8:42 PM Kevin Rushforth 
 wrote:


I also now favor option 2 and was in the process of writing
something up recommending that we do that. Phil and I (and a
couple others) had an offline discussion and think this is the way
to go.

Given the short amount of time to get this into 22, I will file
the JBS issue in the next hour or so.

Nir: if you want to take it, that would be great. Otherwise, I
will do it. We need the PR and CSR filed before the end of this week.

Regarding other methods that choose option 1, many of them are
more complicated (e.g., scene mutation can be done on a background
thread as long as the scene is not "showing") or must be
synchronous. Animation starts something that conceptually happens
later on the FX animation thread anyway, so wrapping it in a
runLater (if not already on the right thread) doesn't really
change the semantics in an appreciable way.

-- Kevin


On 1/24/2024 10:26 AM, Nir Lisker wrote:

These are the options I see as reasonable:

1. Document that these methods *must* be run on the FX thread and
throw an exception otherwise. This leaves it to the
responsibility of the user. However, this will cause the
backwards compatibility problems that Jugen brought up. As a side
note, we do this in other methods already, but I always
questioned why let the developer do something illegal - if
there's only one execution path, force it.
2. Document that these methods *are* run on the FX thread (the
user doesn't need to do anything) and change the implementation
to call runLater(...) internally unless they are already on the
FX thread. This will be backwards compatible for the most part
(see option 3). The downside might be some surprise when these
methods behave differently.
3. Document that it's *recommended* that these methods be run on
the FX thread and let the user be responsible for the threading.
We can explain that manipulating nodes that are attached to an
active scenegraph should be avoided.

I prefer option 2 over 1 regardless of the backwards
compatibility issue even, but would like to know if I'm missing
something here because in theory this change could be done to any
"must run on the FX thread" method and I question why the user
had the option to get an exception.
Option 3 is risky and I wager a guess that it will be used
wrongly more often than not. It does allow some (what I would
call) valid niche uses. I never did it.

On Wed, Jan 24, 2024 at 4:21 PM Kevin Rushforth
 wrote:

I'd like to hear from the others on this. I don't see any
fundamental problem with having the play/pause/stop methods
wrap their implementation in a runLater (if not on the FX
Application thread already), and documenting that it does so,
if we can get general agreement.

-- Kevin


On 1/24/2024 5:29 AM, Jurgen Doll wrote:

Hi Kevin

If I may make one more final appeal then to an alternative
solution please.

Could we then instead of throwing an Exception rather invoke
runLater if needed inside play, stop, and resume.

Putting the onus on the developer is fine if it is the
developer that is invoking the call, but if it's in a
library then it's a no go.

In my application I have two libraries that I know of where
this happens. The major problem is that with FX22 as it now
stands my application just crashes because play() does an FX
thread check and throws an Exception which it never did
before. There are bound to be other applications out there
that are going to find themselves in a similar position.

PLEASE !

Regards
Jurgen



On Wed, 24 Jan 2024 15:15:31 +0200, Kevin Rushforth

 wrote:

Thank you to Jurgen for raising the question and to Nir,
John, and Michael for evaluating it.

I conclude that there is insufficient motivation to
revert the change in behavior implemented by JDK-8159048
to allow calling the play/pause/stop methods of
Animation on a background thread. Doing so without
making it fully multi-thread-safe would be asking for
problems, and making it fully multi-thread-safe would be
a fair bit of 

Re: [External] : Re: HEADS-UP: Threading restriction for Animation play, pause, stop now enforced USE CASE

[Nir Lisker]

If there's an agreement, I can do it tomorrow. It will effectively revert
JDK-8159048 and supersede JDK-8324219.

On Wed, Jan 24, 2024 at 8:42 PM Kevin Rushforth 
wrote:

> I also now favor option 2 and was in the process of writing something up
> recommending that we do that. Phil and I (and a couple others) had an
> offline discussion and think this is the way to go.
>
> Given the short amount of time to get this into 22, I will file the JBS
> issue in the next hour or so.
>
> Nir: if you want to take it, that would be great. Otherwise, I will do it.
> We need the PR and CSR filed before the end of this week.
>
> Regarding other methods that choose option 1, many of them are more
> complicated (e.g., scene mutation can be done on a background thread as
> long as the scene is not "showing") or must be synchronous. Animation
> starts something that conceptually happens later on the FX animation thread
> anyway, so wrapping it in a runLater (if not already on the right thread)
> doesn't really change the semantics in an appreciable way.
>
> -- Kevin
>
>
> On 1/24/2024 10:26 AM, Nir Lisker wrote:
>
> These are the options I see as reasonable:
>
> 1. Document that these methods *must* be run on the FX thread and throw an
> exception otherwise. This leaves it to the responsibility of the user.
> However, this will cause the backwards compatibility problems that Jugen
> brought up. As a side note, we do this in other methods already, but I
> always questioned why let the developer do something illegal - if there's
> only one execution path, force it.
> 2. Document that these methods *are* run on the FX thread (the user
> doesn't need to do anything) and change the implementation to call
> runLater(...) internally unless they are already on the FX thread. This
> will be backwards compatible for the most part (see option 3). The downside
> might be some surprise when these methods behave differently.
> 3. Document that it's *recommended* that these methods be run on the FX
> thread and let the user be responsible for the threading. We can explain
> that manipulating nodes that are attached to an active scenegraph should be
> avoided.
>
> I prefer option 2 over 1 regardless of the backwards compatibility issue
> even, but would like to know if I'm missing something here because in
> theory this change could be done to any "must run on the FX thread" method
> and I question why the user had the option to get an exception.
> Option 3 is risky and I wager a guess that it will be used wrongly more
> often than not. It does allow some (what I would call) valid niche uses. I
> never did it.
>
> On Wed, Jan 24, 2024 at 4:21 PM Kevin Rushforth <
> kevin.rushfo...@oracle.com> wrote:
>
>> I'd like to hear from the others on this. I don't see any fundamental
>> problem with having the play/pause/stop methods wrap their implementation
>> in a runLater (if not on the FX Application thread already), and
>> documenting that it does so, if we can get general agreement.
>>
>> -- Kevin
>>
>>
>> On 1/24/2024 5:29 AM, Jurgen Doll wrote:
>>
>> Hi Kevin
>>
>> If I may make one more final appeal then to an alternative solution
>> please.
>>
>> Could we then instead of throwing an Exception rather invoke runLater if
>> needed inside play, stop, and resume.
>>
>> Putting the onus on the developer is fine if it is the developer that is
>> invoking the call, but if it's in a library then it's a no go.
>>
>> In my application I have two libraries that I know of where this happens.
>> The major problem is that with FX22 as it now stands my application just
>> crashes because play() does an FX thread check and throws an Exception
>> which it never did before. There are bound to be other applications out
>> there that are going to find themselves in a similar position.
>>
>> PLEASE !
>>
>> Regards
>> Jurgen
>>
>>
>>
>> On Wed, 24 Jan 2024 15:15:31 +0200, Kevin Rushforth
>>   wrote:
>>
>> Thank you to Jurgen for raising the question and to Nir, John, and
>> Michael for evaluating it.
>>
>> I conclude that there is insufficient motivation to revert the change in
>> behavior implemented by JDK-8159048 to allow calling the play/pause/stop
>> methods of Animation on a background thread. Doing so without making it
>> fully multi-thread-safe would be asking for problems, and making it fully
>> multi-thread-safe would be a fair bit of work to do it right without a
>> clear benefit.
>>
>> We will proceed with the current approach and let JDK-8159048 stand.
>> Further, we will proceed with https://bugs.openjdk.org/browse/JDK-8324219
>> which is under review in https://github.com/openjdk/jfx/pull/1342
>> 
>>
>> -- Kevin
>>
>> On 1/24/2024 12:30 AM, Nir Lisker wrote:
>>
>> After playing around with the code sample, I think that this is not the
>> right way to use the animation. The 

Re: [External] : Re: HEADS-UP: Threading restriction for Animation play, pause, stop now enforced USE CASE

[Kevin Rushforth]

I also now favor option 2 and was in the process of writing something up 
recommending that we do that. Phil and I (and a couple others) had an 
offline discussion and think this is the way to go.


Given the short amount of time to get this into 22, I will file the JBS 
issue in the next hour or so.


Nir: if you want to take it, that would be great. Otherwise, I will do 
it. We need the PR and CSR filed before the end of this week.


Regarding other methods that choose option 1, many of them are more 
complicated (e.g., scene mutation can be done on a background thread as 
long as the scene is not "showing") or must be synchronous. Animation 
starts something that conceptually happens later on the FX animation 
thread anyway, so wrapping it in a runLater (if not already on the right 
thread) doesn't really change the semantics in an appreciable way.


-- Kevin


On 1/24/2024 10:26 AM, Nir Lisker wrote:

These are the options I see as reasonable:

1. Document that these methods *must* be run on the FX thread and 
throw an exception otherwise. This leaves it to the responsibility of 
the user. However, this will cause the backwards compatibility 
problems that Jugen brought up. As a side note, we do this in other 
methods already, but I always questioned why let the developer do 
something illegal - if there's only one execution path, force it.
2. Document that these methods *are* run on the FX thread (the user 
doesn't need to do anything) and change the implementation to call 
runLater(...) internally unless they are already on the FX thread. 
This will be backwards compatible for the most part (see option 3). 
The downside might be some surprise when these methods behave differently.
3. Document that it's *recommended* that these methods be run on the 
FX thread and let the user be responsible for the threading. We can 
explain that manipulating nodes that are attached to an active 
scenegraph should be avoided.


I prefer option 2 over 1 regardless of the backwards compatibility 
issue even, but would like to know if I'm missing something here 
because in theory this change could be done to any "must run on the FX 
thread" method and I question why the user had the option to get an 
exception.
Option 3 is risky and I wager a guess that it will be used wrongly 
more often than not. It does allow some (what I would call) valid 
niche uses. I never did it.


On Wed, Jan 24, 2024 at 4:21 PM Kevin Rushforth 
 wrote:


I'd like to hear from the others on this. I don't see any
fundamental problem with having the play/pause/stop methods wrap
their implementation in a runLater (if not on the FX Application
thread already), and documenting that it does so, if we can get
general agreement.

-- Kevin


On 1/24/2024 5:29 AM, Jurgen Doll wrote:

Hi Kevin

If I may make one more final appeal then to an alternative
solution please.

Could we then instead of throwing an Exception rather invoke
runLater if needed inside play, stop, and resume.

Putting the onus on the developer is fine if it is the developer
that is invoking the call, but if it's in a library then it's a
no go.

In my application I have two libraries that I know of where this
happens. The major problem is that with FX22 as it now stands my
application just crashes because play() does an FX thread check
and throws an Exception which it never did before. There are
bound to be other applications out there that are going to find
themselves in a similar position.

PLEASE !

Regards
Jurgen



On Wed, 24 Jan 2024 15:15:31 +0200, Kevin Rushforth
 
wrote:

Thank you to Jurgen for raising the question and to Nir,
John, and Michael for evaluating it.

I conclude that there is insufficient motivation to revert
the change in behavior implemented by JDK-8159048 to allow
calling the play/pause/stop methods of Animation on a
background thread. Doing so without making it fully
multi-thread-safe would be asking for problems, and making it
fully multi-thread-safe would be a fair bit of work to do it
right without a clear benefit.

We will proceed with the current approach and let JDK-8159048
stand. Further, we will proceed with
https://bugs.openjdk.org/browse/JDK-8324219 which is under
review in https://github.com/openjdk/jfx/pull/1342



-- Kevin

On 1/24/2024 12:30 AM, Nir Lisker wrote:

After playing around with the code sample, I think that this
is not the right way to use the animation. The reason is
that there is no point in starting the animation before the
control is attached to the 

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

[Nir Lisker]

These are the options I see as reasonable:

1. Document that these methods *must* be run on the FX thread and throw an
exception otherwise. This leaves it to the responsibility of the user.
However, this will cause the backwards compatibility problems that Jugen
brought up. As a side note, we do this in other methods already, but I
always questioned why let the developer do something illegal - if there's
only one execution path, force it.
2. Document that these methods *are* run on the FX thread (the user doesn't
need to do anything) and change the implementation to call runLater(...)
internally unless they are already on the FX thread. This will be backwards
compatible for the most part (see option 3). The downside might be some
surprise when these methods behave differently.
3. Document that it's *recommended* that these methods be run on the FX
thread and let the user be responsible for the threading. We can explain
that manipulating nodes that are attached to an active scenegraph should be
avoided.

I prefer option 2 over 1 regardless of the backwards compatibility issue
even, but would like to know if I'm missing something here because in
theory this change could be done to any "must run on the FX thread" method
and I question why the user had the option to get an exception.
Option 3 is risky and I wager a guess that it will be used wrongly more
often than not. It does allow some (what I would call) valid niche uses. I
never did it.

On Wed, Jan 24, 2024 at 4:21 PM Kevin Rushforth 
wrote:

> I'd like to hear from the others on this. I don't see any fundamental
> problem with having the play/pause/stop methods wrap their implementation
> in a runLater (if not on the FX Application thread already), and
> documenting that it does so, if we can get general agreement.
>
> -- Kevin
>
>
> On 1/24/2024 5:29 AM, Jurgen Doll wrote:
>
> Hi Kevin
>
> If I may make one more final appeal then to an alternative solution please.
>
> Could we then instead of throwing an Exception rather invoke runLater if
> needed inside play, stop, and resume.
>
> Putting the onus on the developer is fine if it is the developer that is
> invoking the call, but if it's in a library then it's a no go.
>
> In my application I have two libraries that I know of where this happens.
> The major problem is that with FX22 as it now stands my application just
> crashes because play() does an FX thread check and throws an Exception
> which it never did before. There are bound to be other applications out
> there that are going to find themselves in a similar position.
>
> PLEASE !
>
> Regards
> Jurgen
>
>
>
> On Wed, 24 Jan 2024 15:15:31 +0200, Kevin Rushforth
>   wrote:
>
> Thank you to Jurgen for raising the question and to Nir, John, and Michael
> for evaluating it.
>
> I conclude that there is insufficient motivation to revert the change in
> behavior implemented by JDK-8159048 to allow calling the play/pause/stop
> methods of Animation on a background thread. Doing so without making it
> fully multi-thread-safe would be asking for problems, and making it fully
> multi-thread-safe would be a fair bit of work to do it right without a
> clear benefit.
>
> We will proceed with the current approach and let JDK-8159048 stand.
> Further, we will proceed with https://bugs.openjdk.org/browse/JDK-8324219
> which is under review in https://github.com/openjdk/jfx/pull/1342
>
> -- Kevin
>
> On 1/24/2024 12:30 AM, Nir Lisker wrote:
>
> After playing around with the code sample, I think that this is not the
> right way to use the animation. The reason is that there is no point in
> starting the animation before the control is attached to the scenegraph, or
> even made visible. A small refactoring where, e.g., the controller class
> exposes a method to start the animation in onSucceeded or just calls it on
> the FX thread is enough. I never start an animation as part of the
> construction because it's not the right time. John suggested tying the
> lifecycle of the animation to the showing of the node, which also solves
> the problem.
>
> There are animations like PauseTransition or other non-interfering
> Timelines that could reasonably be run on a background thread. Or maybe
> just on an unconnected control. This could be a reason to not limit
> animation methods to the FX thread at the expense of possible user errors,
> but document the pitfall.
>
> I don't see a good use case for modifying controls in a background thread
> while still interacting with the scenegraph, hence for adding multithread
> support.
>
> - Nir
>
> On Mon, Jan 22, 2024, 12:59 Jurgen Doll  wrote:
>
>> Here's an example as requested by Nir:
>>
>> public class FxTimeLineTest extends Application
>>
>> {
>>
>> private BorderPane bp = new BorderPane( new Label("Loading") );
>>
>> public static void main( String[] args ) {
>>
>> launch( FxTimeLineTest.class, args );
>>
>> }
>>
>> @Override
>>
>> public void start( Stage primaryStage ) throws Exception {
>>
>> new Thread( new LoadScene() 

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

[Kevin Rushforth]

I'd like to hear from the others on this. I don't see any fundamental 
problem with having the play/pause/stop methods wrap their 
implementation in a runLater (if not on the FX Application thread 
already), and documenting that it does so, if we can get general agreement.


-- Kevin


On 1/24/2024 5:29 AM, Jurgen Doll wrote:

Hi Kevin

If I may make one more final appeal then to an alternative solution 
please.


Could we then instead of throwing an Exception rather invoke runLater 
if needed inside play, stop, and resume.


Putting the onus on the developer is fine if it is the developer that 
is invoking the call, but if it's in a library then it's a no go.


In my application I have two libraries that I know of where this 
happens. The major problem is that with FX22 as it now stands my 
application just crashes because play() does an FX thread check and 
throws an Exception which it never did before. There are bound to be 
other applications out there that are going to find themselves in a 
similar position.


PLEASE !

Regards
Jurgen



On Wed, 24 Jan 2024 15:15:31 +0200, Kevin Rushforth 
 wrote:


Thank you to Jurgen for raising the question and to Nir, John, and
Michael for evaluating it.

I conclude that there is insufficient motivation to revert the
change in behavior implemented by JDK-8159048 to allow calling the
play/pause/stop methods of Animation on a background thread. Doing
so without making it fully multi-thread-safe would be asking for
problems, and making it fully multi-thread-safe would be a fair
bit of work to do it right without a clear benefit.

We will proceed with the current approach and let JDK-8159048
stand. Further, we will proceed with
https://bugs.openjdk.org/browse/JDK-8324219 which is under review
in https://github.com/openjdk/jfx/pull/1342

-- Kevin

On 1/24/2024 12:30 AM, Nir Lisker wrote:

After playing around with the code sample, I think that this is
not the right way to use the animation. The reason is that there
is no point in starting the animation before the control is
attached to the scenegraph, or even made visible. A small
refactoring where, e.g., the controller class exposes a method to
start the animation in onSucceeded or just calls it on the FX
thread is enough. I never start an animation as part of the
construction because it's not the right time. John suggested
tying the lifecycle of the animation to the showing of the node,
which also solves the problem.

There are animations like PauseTransition or other
non-interfering Timelines that could reasonably be run on a
background thread. Or maybe just on an unconnected control. This
could be a reason to not limit animation methods to the FX thread
at the expense of possible user errors, but document the pitfall.

I don't see a good use case for modifying controls in a
background thread while still interacting with the scenegraph,
hence for adding multithread support.

- Nir

On Mon, Jan 22, 2024, 12:59 Jurgen Doll 
wrote:

Here's an example as requested by Nir:

publicclassFxTimeLineTest extendsApplication

{

privateBorderPane bp= newBorderPane( newLabel("Loading") );

publicstaticvoidmain( String[] args) {

launch( FxTimeLineTest.class, args);

}

@Override

publicvoidstart( Stage primaryStage) throwsException {

newThread( newLoadScene() ).start();

primaryStage.setScene( newScene( bp, 300, 200 ) );

primaryStage.setTitle( "Memory Usage");

primaryStage.show();

}

privateclassLoadScene extendsTask {

@OverrideprotectedParent call() throwsException {

Parent p= FXMLLoader.load( getClass(
).getResource("TestView.fxml") );

Thread.sleep( 1000 );

returnp;

}

@Overrideprotectedvoidsucceeded() {

bp.setCenter( getValue() );

}

@Overrideprotectedvoidfailed() {

getException().printStackTrace();

}

}

}


--


publicclassTestView

{

@FXMLprivateLabel memory;

privatestaticfinaldoubleMEGABYTE= 1024 * 1024;

@FXMLprivatevoidinitialize()

{

varupdater= newTimeline

(

newK eyFrame( Duration.seconds(2.5), event->

{

varruntime= Runtime.getRuntime();

doublemaxMemory= runtime.maxMemory() / MEGABYTE;

doubleusedMemory= (runtime.totalMemory() -
runtime.freeMemory()) / MEGABYTE;

memory.setText( (int) usedMemory+ " MB / "+ (int) maxMemory+"
MB");

})

);

updater.setCycleCount(Animation.INDEFINITE); // This FXML is
being loaded on a background thread

updater.play();

}

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

[Jurgen Doll]

Hi Kevin

If I may make one more final appeal then to an alternative solution please.

Could we then instead of throwing an Exception rather invoke runLater if  
needed inside play, stop, and resume.


Putting the onus on the developer is fine if it is the developer that is  
invoking the call, but if it's in a library then it's a no go.


In my application I have two libraries that I know of where this happens.  
The major problem is that with FX22 as it now stands my application just  
crashes because play() does an FX thread check and throws an Exception  
which it never did before. There are bound to be other applications out  
there that are going to find themselves in a similar position.


PLEASE !

Regards
Jurgen



On Wed, 24 Jan 2024 15:15:31 +0200, Kevin Rushforth  
 wrote:


Thank you to Jurgen for raising the question and to Nir, John, and  
Michael for evaluating it.


I conclude that there is insufficient motivation to revert the change in  
behavior implemented by JDK-8159048 to allow calling the  
play/>pause/stop methods of Animation on a background thread. Doing so  
without making it fully multi-thread-safe would be asking for problems,  
>and making it fully multi-thread-safe would be a fair bit of work to do  
it right without a clear benefit.


We will proceed with the current approach and let JDK-8159048 stand.  
Further, we will proceed with  
https://bugs.openjdk.org/browse/>JDK-8324219 which is under review in  
https://github.com/openjdk/jfx/pull/1342


-- Kevin

On 1/24/2024 12:30 AM, Nir Lisker wrote:
After playing around with the code sample, I think that this is not the  
right way to use the animation. The reason is that there is >>no point  
in starting the animation before the control is attached to the  
scenegraph, or even made visible. A small refactoring >>where, e.g.,  
the controller class exposes a method to start the animation in  
onSucceeded or just calls it on the FX thread is >>enough. I never  
start an animation as part of the construction because it's not the  
right time. John suggested tying the lifecycle >>of the animation to  
the showing of the node, which also solves the problem.  
There are animations like PauseTransition or other non-interfering  
Timelines that could reasonably be run on a background >>thread. Or  
maybe just on an unconnected control. This could be a reason to not  
limit animation methods to the FX thread at >>the expense of possible  
user errors, but document the pitfall.


I don't see a good use case for modifying controls in a background  
thread while still interacting with the scenegraph, hence for >>adding  
multithread support.

- Nir

On Mon, Jan 22, 2024, 12:59 Jurgen Doll  wrote:

Here's an example as requested by Nir:


public class FxTimeLineTest extends Application


{

   private BorderPane bp = new BorderPane( new Label("Loading") );



   public static void main( String[] args ) {

   launch( FxTimeLineTest.class, args );

   }



   @Override

   public void start( Stage primaryStage ) throws Exception {

   new Thread( new LoadScene() ).start();

   primaryStage.setScene(new Scene( bp, 300, 200 ) );

   primaryStage.setTitle( "Memory Usage" );

   primaryStage.show();

   }



   private class LoadScene extends Task {

   @Override protected Parent call() throws Exception {

   Parent p = FXMLLoader.load( getClass(
).getResource("TestView.fxml") );

   Thread.sleep( 1000 );

   return p;

   }



   @Override protected void succeeded() {

   bp.setCenter( getValue() );

   }



   @Override protected void failed() {

   getException().printStackTrace();

   }

   }

}


--


public class TestView

{

   @FXML private Label memory;



   private static final double  MEGABYTE = 1024 * 1024;



   @FXML private void initialize()

   {

   var updater = new Timeline

   (

   new K
eyFrame( Duration.seconds(2.5), event ->

   {

   var runtime = Runtime.getRuntime();

   double maxMemory = runtime.maxMemory() / MEGABYTE;

   double usedMemory = (runtime.totalMemory() -  
runtime.freeMemory()) / MEGABYTE;


   memory.setText( (int) usedMemory + " MB / " + (int)  
maxMemory +" MB" );


   })

   );



   updater.setCycleCount(Animation.INDEFINITE);
   // This FXML is being loaded on a background thread

   updater.play();

   }

}


--

TestView.fxml











http://javafx.com/fxml/1;  
fx:controller="TestView">


  

 



 

  





On Sat, 20 Jan 2024 17:08:41 +0200, Nir Lisker   
wrote:



Hi Jurgen,
What I'm confused about the most is what it is you are actually  
trying to do that necessitates the use of animations 

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

[Kevin Rushforth]

Thank you to Jurgen for raising the question and to Nir, John, and 
Michael for evaluating it.


I conclude that there is insufficient motivation to revert the change in 
behavior implemented by JDK-8159048 to allow calling the play/pause/stop 
methods of Animation on a background thread. Doing so without making it 
fully multi-thread-safe would be asking for problems, and making it 
fully multi-thread-safe would be a fair bit of work to do it right 
without a clear benefit.


We will proceed with the current approach and let JDK-8159048 stand. 
Further, we will proceed with 
https://bugs.openjdk.org/browse/JDK-8324219 which is under review in 
https://github.com/openjdk/jfx/pull/1342


-- Kevin

On 1/24/2024 12:30 AM, Nir Lisker wrote:
After playing around with the code sample, I think that this is not 
the right way to use the animation. The reason is that there is no 
point in starting the animation before the control is attached to the 
scenegraph, or even made visible. A small refactoring where, e.g., the 
controller class exposes a method to start the animation in 
onSucceeded or just calls it on the FX thread is enough. I never start 
an animation as part of the construction because it's not the right 
time. John suggested tying the lifecycle of the animation to the 
showing of the node, which also solves the problem.


There are animations like PauseTransition or other non-interfering 
Timelines that could reasonably be run on a background thread. Or 
maybe just on an unconnected control. This could be a reason to not 
limit animation methods to the FX thread at the expense of possible 
user errors, but document the pitfall.


I don't see a good use case for modifying controls in a background 
thread while still interacting with the scenegraph, hence for adding 
multithread support.


- Nir

On Mon, Jan 22, 2024, 12:59 Jurgen Doll  wrote:

Here's an example as requested by Nir:

publicclassFxTimeLineTest extendsApplication

{

privateBorderPane bp= newBorderPane( newLabel("Loading") );

publicstaticvoidmain( String[] args) {

launch( FxTimeLineTest.class, args);

}

@Override

publicvoidstart( Stage primaryStage) throwsException {

newThread( newLoadScene() ).start();

primaryStage.setScene( newScene( bp, 300, 200 ) );

primaryStage.setTitle( "Memory Usage");

primaryStage.show();

}

privateclassLoadScene extendsTask {

@OverrideprotectedParent call() throwsException {

Parent p= FXMLLoader.load( getClass().getResource("TestView.fxml") );

Thread.sleep( 1000 );

returnp;

}

@Overrideprotectedvoidsucceeded() {

bp.setCenter( getValue() );

}

@Overrideprotectedvoidfailed() {

getException().printStackTrace();

}

}

}


--


publicclassTestView

{

@FXMLprivateLabel memory;

privatestaticfinaldoubleMEGABYTE= 1024 * 1024;

@FXMLprivatevoidinitialize()

{

varupdater= newTimeline

(

newKeyFrame( Duration.seconds(2.5), event->

{

varruntime= Runtime.getRuntime();

doublemaxMemory= runtime.maxMemory() / MEGABYTE;

doubleusedMemory= (runtime.totalMemory() - runtime.freeMemory()) /
MEGABYTE;

memory.setText( (int) usedMemory+ " MB / "+ (int) maxMemory+" MB");

})

);

updater.setCycleCount(Animation.INDEFINITE); // This FXML is being
loaded on a background thread

updater.play();

}

}


--


TestView.fxml







http://javafx.com/fxml/1;
fx:controller="TestView">
















On Sat, 20 Jan 2024 17:08:41 +0200, Nir Lisker 
wrote:

Hi Jurgen,

What I'm confused about the most is what it is you are
actually trying to do that necessitates the use of animations
outside of the FX thread. You said that you need to initialize
controls on another thread, and that you are using Task (both
of which are fine), but how does playing animations relate?
Playing an animation is something that is done explicitly,
usually in order to manipulate data. Can you give a real use
case, like a minimized version of what you're doing?

- Nir

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

[John Hendrikx]

Hi Jurgen,

See my answers inline.

On 24/01/2024 10:12, Jurgen Doll wrote:

Hi John

Thank you for the hypothetical receivers array scenario, I think it 
explains the problem exactly and is why replacing the array with 
CopyOnWriteArrayList removes the NPE.


Your perspective then is that AbstractPrimaryTimer is designed for 
single threaded use only.


Yes, it looks that way simply because I see none of the usual 
concurrency controls (Locks, synchronized, volatile), and of course it 
fits with the general design of FX where almost all FX code is assumed 
to be accessed from a single thread.


However, there are issues even with a single thread that must be taken 
care of, see below.


If that is indeed so, then could you please explain the purpose of the 
receiversLocked and animationTimersLocked flags, as well as the point 
of receivers.clone() and animationTimers.clone() all of which indicate 
to the contrary.


This is a pattern that we can also see in other areas of FX (see 
ExpressionHelper for example).  The clones and the use of a 
(non-synchronized) locking flag are to prevent issues with recursive 
calls on the same thread: During the notifying of all registered pulse 
receivers, the list is locked.  If one of those receivers decides to 
remove itself *during* the notification callback, then the list would be 
modified while iterating (as more receivers may need notifying still).  
If that happens, it will instead see that the list is currently locked, 
and make a clone.


Replacing the array with a CopyOnWriteArrayList would mean that copies 
will be made even when not needed.  Since the code seems to go through 
great lengths to even avoid the super fast ArrayList implementation 
(which IMHO would have been perfectly acceptable here) I can't help but 
wonder if that could have significant performance implications.  Perhaps 
pulse receivers are added/removed far more frequently than we would think.


--John





Thanks, regards
Jurgen


On Tue, 23 Jan 2024 18:36:16 +0200, John Hendrikx 
 wrote:


On 23/01/2024 16:56, Jurgen Doll wrote:

Hi John and others,

I don't think we are entirely on the same page, so here's the
objective.

The Goal: To determine if the FX animation thread and a SINGLE
other thread can access Animation in a safe manner wrt play,
stop, and resume.

The number of threads is irrelevant really, it's either thread
safe or it isn't.


Non Goal: Multi-threaded access of Animation play, stop, and
resume is NOT a goal.

Wrt play() and resume(), it is ALWAYS safe to call them on a
background thread because at this point the Animation isn't being
processed by the FX thread as the "Animation" isn't contained in
the AbstractPrimaryTimer receivers array.


I'm afraid that is incorrect.  The fact that your animation is not
running doesn't mean that AbstractPrimaryTimer isn't in use by
other animations that are running.  These other animations are
using the receivers array, and may be modifying it or reading from
it from the FX thread.

When you start your animation on a different thread, you are
accessing these fields with a different thread, and modifying
them.  Since the JVM is free to cache values and do other fancy
things (like reordering read/writes) in the absence of
synchronized/locking, there is no guarantee that the FX thread
will see those modifications until these are flushed to main
memory (or caches are synced).  Even then, the FX thread may have
these values cached somewhere, and so it may not go all the way to
main memory to see if its assumptions are now incorrect.  The only
way to ensure this is with proper use of synchronization.

So a hypothetical scenario:

- AbstractPrimaryTimer has no receivers
- A receiver is added via the FX thread, slot 0 is now filled and
receiversLength is now 1.  Due to cache lines being large, it also
read slot 1 (which is null)
- You start your animation on another thread.  Since you didn't
see the receivers array yet, you may see one of these states:
    - The change from the FX thread was flushed to main memory,
and you see [X, null] and receiversLength = 1
    - The change from the FX thread was partially flushed, and you
see [X, null]  and receiversLength = 0 (!!)
    - Nothing was flushed yet, and you see [null, null] and
receiversLength = 0

Now, you can see that it would be very dangerous to proceed to
modify the array based on half flushed information. Something
similar happens when you are the first to start an animation, and
then another is started later.  If the changes of your thread are
not flushed yet (or partially) then the FX thread will act on
partially flushed data, or even see no receivers yet at all...

--John

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

[Jurgen Doll]

Hi John

Thank you for the hypothetical receivers array scenario, I think it  
explains the problem exactly and is why replacing the array with  
CopyOnWriteArrayList removes the NPE.


Your perspective then is that AbstractPrimaryTimer is designed for single  
threaded use only. If that is indeed so, then could you please explain the  
purpose of the receiversLocked and animationTimersLocked flags, as well as  
the point of receivers.clone() and animationTimers.clone() all of which  
indicate to the contrary.


Thanks, regards
Jurgen


On Tue, 23 Jan 2024 18:36:16 +0200, John Hendrikx  
 wrote:



On 23/01/2024 16:56, Jurgen Doll wrote:

Hi John and others,

I don't think we are entirely on the same page, so here's the  
objective.


The Goal: To determine if the FX animation thread and a SINGLE other  
thread can access Animation in a >>safe manner wrt play, stop, and  
resume.
The number of threads is irrelevant really, it's either thread safe or  
it isn't.


Non Goal: Multi-threaded access of Animation play, stop, and resume  
is NOT a goal.


Wrt play() and resume(), it is ALWAYS safe to call them on a background  
thread because at this point the >>Animation isn't being processed by  
the FX thread as the "Animation" isn't contained in the  
>>AbstractPrimaryTimer receivers array.


I'm afraid that is incorrect.  The fact that your animation is not  
running doesn't mean that AbstractPrimaryTimer isn't in use by other  
>animations that are running.  These other animations are using the  
receivers array, and may be modifying it or reading from it from the FX  
>thread.


When you start your animation on a different thread, you are accessing  
these fields with a different thread, and modifying them.  Since the  
>JVM is free to cache values and do other fancy things (like reordering  
read/writes) in the absence of synchronized/locking, there is no  
>guarantee that the FX thread will see those modifications until these  
are flushed to main memory (or caches are synced).  Even then, the FX  
>thread may have these values cached somewhere, and so it may not go all  
the way to main memory to see if its assumptions are now >incorrect.   
The only way to ensure this is with proper use of synchronization.


So a hypothetical scenario:

- AbstractPrimaryTimer has no receivers
- A receiver is added via the FX thread, slot 0 is now filled and  
receiversLength is now 1.  Due to cache lines being large, it also read  
slot 1 >(which is null)
- You start your animation on another thread.  Since you didn't see the  
receivers array yet, you may see one of these states:
   - The change from the FX thread was flushed to main memory, and you  
see [X, null] and receiversLength = 1
   - The change from the FX thread was partially flushed, and you see  
[X, null]  and receiversLength = 0 (!!)
   - Nothing was flushed yet, and you see [null, null] and  
receiversLength = 0


Now, you can see that it would be very dangerous to proceed to modify  
the array based on half flushed information.  Something similar >happens  
when you are the first to start an animation, and then another is  
started later.  If the changes of your thread are not flushed yet (or  
>partially) then the FX thread will act on partially flushed data, or  
even see no receivers yet at all...


--John

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

[Nir Lisker]

After playing around with the code sample, I think that this is not the
right way to use the animation. The reason is that there is no point in
starting the animation before the control is attached to the scenegraph, or
even made visible. A small refactoring where, e.g., the controller class
exposes a method to start the animation in onSucceeded or just calls it on
the FX thread is enough. I never start an animation as part of the
construction because it's not the right time. John suggested tying the
lifecycle of the animation to the showing of the node, which also solves
the problem.

There are animations like PauseTransition or other non-interfering
Timelines that could reasonably be run on a background thread. Or maybe
just on an unconnected control. This could be a reason to not limit
animation methods to the FX thread at the expense of possible user errors,
but document the pitfall.

I don't see a good use case for modifying controls in a background thread
while still interacting with the scenegraph, hence for adding multithread
support.

- Nir

On Mon, Jan 22, 2024, 12:59 Jurgen Doll  wrote:

> Here's an example as requested by Nir:
>
> public class FxTimeLineTest extends Application
>
> {
>
> private BorderPane bp = new BorderPane( new Label("Loading") );
>
>
> public static void main( String[] args ) {
>
> launch( FxTimeLineTest.class, args );
>
> }
>
>
> @Override
>
> public void start( Stage primaryStage ) throws Exception {
>
> new Thread( new LoadScene() ).start();
>
> primaryStage.setScene( new Scene( bp, 300, 200 ) );
>
> primaryStage.setTitle( "Memory Usage" );
>
> primaryStage.show();
>
> }
>
>
> private class LoadScene extends Task {
>
> @Override protected Parent call() throws Exception {
>
> Parent p = FXMLLoader.load( getClass().getResource("TestView.fxml") );
>
> Thread.sleep( 1000 );
>
> return p;
>
> }
>
>
> @Override protected void succeeded() {
>
> bp.setCenter( getValue() );
>
> }
>
>
> @Override protected void failed() {
>
> getException().printStackTrace();
>
> }
>
> }
>
> }
>
>
> --
>
> public class TestView
>
> {
>
> @FXML private Label memory;
>
>
> private static final double MEGABYTE = 1024 * 1024;
>
>
> @FXML private void initialize()
>
> {
>
> var updater = new Timeline
>
> (
>
> new KeyFrame( Duration.seconds(2.5), event ->
>
> {
>
> var runtime = Runtime.getRuntime();
>
> double maxMemory = runtime.maxMemory() / MEGABYTE;
>
> double usedMemory = (runtime.totalMemory() - runtime.freeMemory()) /
> MEGABYTE;
>
> memory.setText( (int) usedMemory + " MB / " + (int) maxMemory +" MB" );
>
> })
>
> );
>
>
> updater.setCycleCount(Animation.INDEFINITE);
> // This FXML is being loaded on a background thread
>
> updater.play();
>
> }
>
> }
>
>
> --
>
> TestView.fxml
>
> 
>
>
> 
>
> 
>
>
> http://javafx.com/fxml/1; fx:controller="TestView">
>
> 
>
> 
>
> 
>
> 
>
> 
>
> 
>
>
>
> On Sat, 20 Jan 2024 17:08:41 +0200, Nir Lisker  wrote:
>
> Hi Jurgen,
>
> What I'm confused about the most is what it is you are actually trying to
> do that necessitates the use of animations outside of the FX thread. You
> said that you need to initialize controls on another thread, and that you
> are using Task (both of which are fine), but how does playing animations
> relate? Playing an animation is something that is done explicitly, usually
> in order to manipulate data. Can you give a real use case, like a minimized
> version of what you're doing?
>
> - Nir
>
>

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

[Michael Strauß]

Hi Jurgen,

even assuming that it was as easy as you say it is, there are more
problems with this proposal.

1. If non-FX thread access to some methods is allowed, this will
become a permanent addition to the API and a complication we have to
deal with. There are proposals to refactor the animation framework,
and this will become harder as a result. This also has direct
implications on the implementation (you already pointed out two
instances where fields would have to be marked volatile).
Multithreaded code is never easy, even in easy cases. Mutation
visibility will become a permanent question for future development.

2. We will need to specify that the `onFinished` handler might be
invoked on the thread that calls play() or on the FX thread, or we
need to make sure that `onFinished` is always invoked on the FX
thread.

3. Any change that would expressly allow concurrent access to the FX
framework will require lots of testing to ensure that we don't
introduce regressions or race conditions. This competes with reviewer
time for other features and fixes.

4. That being said, all of the effort might still be a good investment
if the added value was significant. But I don't see the significance
of it, as you can simply wrap the play() call in Platform.runLater and
be done with it.


On Tue, Jan 23, 2024 at 4:56 PM Jurgen Doll  wrote:
>
> Hi John and others,
>
> I don't think we are entirely on the same page, so here's the objective.
>
> The Goal: To determine if the FX animation thread and a SINGLE other thread 
> can access Animation in a safe manner wrt play, stop, and resume.
>
> Non Goal: Multi-threaded access of Animation play, stop, and resume is NOT a 
> goal.
>
> Wrt play() and resume(), it is ALWAYS safe to call them on a background 
> thread because at this point the Animation isn't being processed by the FX 
> thread as the "Animation" isn't contained in the AbstractPrimaryTimer 
> receivers array.
>
> Wrt stop() from what I can tell there are no shared method calls that need 
> synchronization (see audit below), however the following two boolean flags 
> should be marked as volatile in order to ensure that animation is cut short 
> if executing:
>
> TimelineClipCore.abort
> ClipEnvelope.abort
>
> This is simple enough to add to my original proposal of replacing the arrays 
> in AbstractPrimaryTimer with CopyOnWriteArrayList which is thread safe and 
> replicates the intended behavior in a clear and concise way.
>
> Here are the methods that are called when stop() is invoked:
>
> Timeline.stop()
>
> getStatus()
>
> clipCore.abort()
>
> isStopped()
>
> clipEnvelope.abortCurrentPulse()
>
> doStop()
>
> timer.removePulseReceiver(pulseReceiver);
>
> // After this point it doesn't matter
>
> setStatus(Status.STOPPED)
>
> doSetCurrentRate(0.0)
>
> jumpTo(Duration.ZERO)
>
>
> And for AbstractPrimaryTimer.timePulseImpl:
>
> AbstractPrimaryTimer.timePulseImpl
>
> Animation.PulseReceiver.timePulse
>
> Animation.doTimePulse
>
> clipEnvelope.timePulse
>
> animation.doPlayTo
>
> clipCore.playTo
>
> visitKeyFrame
>
> setTime
>
> animation.setCurrentTicks
>
> clipInterpolator.interpolate
>
> animation.doJumpTo
>
> sync(false)
>
> setCurrentTicks
>
> clipCore.jumpTo
>
> timeline.getStatus()
>
> clipInterpolator.interpolate
>
>
> Regards
> Jurgen

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

[John Hendrikx]

On 23/01/2024 16:56, Jurgen Doll wrote:

Hi John and others,

I don't think we are entirely on the same page, so here's the objective.

The Goal: To determine if the FX animation thread and a SINGLE other 
thread can access Animation in a safe manner wrt play, stop, and resume.
The number of threads is irrelevant really, it's either thread safe or 
it isn't.


Non Goal: Multi-threaded access of Animation play, stop, and resume is 
NOT a goal.


Wrt play() and resume(), it is ALWAYS safe to call them on a 
background thread because at this point the Animation isn't being 
processed by the FX thread as the "Animation" isn't contained in the 
AbstractPrimaryTimer receivers array.


I'm afraid that is incorrect.  The fact that your animation is not 
running doesn't mean that AbstractPrimaryTimer isn't in use by other 
animations that are running.  These other animations are using the 
receivers array, and may be modifying it or reading from it from the FX 
thread.


When you start your animation on a different thread, you are accessing 
these fields with a different thread, and modifying them.  Since the JVM 
is free to cache values and do other fancy things (like reordering 
read/writes) in the absence of synchronized/locking, there is no 
guarantee that the FX thread will see those modifications until these 
are flushed to main memory (or caches are synced).  Even then, the FX 
thread may have these values cached somewhere, and so it may not go all 
the way to main memory to see if its assumptions are now incorrect.  The 
only way to ensure this is with proper use of synchronization.


So a hypothetical scenario:

- AbstractPrimaryTimer has no receivers
- A receiver is added via the FX thread, slot 0 is now filled and 
receiversLength is now 1.  Due to cache lines being large, it also read 
slot 1 (which is null)
- You start your animation on another thread.  Since you didn't see the 
receivers array yet, you may see one of these states:
    - The change from the FX thread was flushed to main memory, and you 
see [X, null] and receiversLength = 1
    - The change from the FX thread was partially flushed, and you see 
[X, null]  and receiversLength = 0 (!!)
    - Nothing was flushed yet, and you see [null, null] and 
receiversLength = 0


Now, you can see that it would be very dangerous to proceed to modify 
the array based on half flushed information.  Something similar happens 
when you are the first to start an animation, and then another is 
started later.  If the changes of your thread are not flushed yet (or 
partially) then the FX thread will act on partially flushed data, or 
even see no receivers yet at all...


--John

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

[Jurgen Doll]

Hi John and others,

I don't think we are entirely on the same page, so here's the objective.

The Goal: To determine if the FX animation thread and a SINGLE other  
thread can access Animation in a safe manner wrt play, stop, and resume.


Non Goal: Multi-threaded access of Animation play, stop, and resume is NOT  
a goal.


Wrt play() and resume(), it is ALWAYS safe to call them on a background  
thread because at this point the Animation isn't being processed by the FX  
thread as the "Animation" isn't contained in the AbstractPrimaryTimer  
receivers array.


Wrt stop() from what I can tell there are no shared method calls that need  
synchronization (see audit below), however the following two boolean flags  
should be marked as volatile in order to ensure that animation is cut  
short if executing:


TimelineClipCore.abort
ClipEnvelope.abort

This is simple enough to add to my original proposal of replacing the  
arrays in AbstractPrimaryTimer with CopyOnWriteArrayList which is thread  
safe and replicates the intended behavior in a clear and concise way.


Here are the methods that are called when stop() is invoked:


Timeline.stop()

   getStatus()

   clipCore.abort()

   isStopped()

   clipEnvelope.abortCurrentPulse()

   doStop()

  timer.removePulseReceiver(pulseReceiver);

  // After this point it doesn't matter
  setStatus(Status.STOPPED)

  doSetCurrentRate(0.0)

  jumpTo(Duration.ZERO)

And for AbstractPrimaryTimer.timePulseImpl:


AbstractPrimaryTimer.timePulseImpl

   Animation.PulseReceiver.timePulse

  Animation.doTimePulse

 clipEnvelope.timePulse

 animation.doPlayTo

clipCore.playTo

   visitKeyFrame

  setTime

 animation.setCurrentTicks

  clipInterpolator.interpolate

 animation.doJumpTo

sync(false)

setCurrentTicks

clipCore.jumpTo

   timeline.getStatus()

   clipInterpolator.interpolate

Regards
Jurgen


On Tue, 23 Jan 2024 01:52:42 +0200, John Hendrikx  
 wrote:


This is not a good idea, the NPE is a symptom of the problem, but there  
can be many more subtler problems (and new ones may surface after  
>fixing the NPE) that can affect other animations making use of the  
shared structures involved.  For one thing, absent a method of  
>synchronization, Java doesn't even guarantee that fields (or arrays)  
contain the same value for all threads.  So your thread may modify some  
>state, but the FX thread wouldn't even know about it (or only  
partially), and may make assumptions about an incorrect state, and then  
make >further changes based on incorrect assumptions.


A good example is that even a much simpler class like HashMap when  
accessed by multiple threads can get into a really bad state.  It will  
>usually throw a "ConcurrentModificationException" (or perhaps a weird  
NPE or IllegalStateException) to alert you that you're using it wrong  
>-- but those are the **best** case scenarios... In the worst case  
scenario, modifying a HashMap on multiple threads can result in an  
infinite >loop -- this can happen when two modifications occur, both  
updating the same bucket, and the threads end up changing things in such  
a way >that the entries point to each other.  Iterating it then (or  
accessing the wrong key) will cause an infinite loop (not hypothetical,  
I've had this >happen).


I'm afraid that even the synchronization option I proposed is not very  
realistic due to the amount of work it will entail.  We'd need to trace  
all >code paths that a call to play or stop could get to, and all those  
code paths would need to be made thread safe.


--John

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

[Jurgen Doll]

Hi Michael


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.


Agreed. Actually we should drive this further and say that "No changes  
should be made to properties that are being 'animated' while animation is  
in progress regardless of whether you're on the FX thread or not, or if  
the object is in the scene graph or not".


updater.play();
memoryLabel.setStyle( "-fx-text-fill: green" );  // This is OK
memoryLabel.setText( "This is NOT okay" );  // This is NOT, move  
to before play


Interesting API idea, however I don't think it's really needed for  
sensible developers.


Regards
Jurgen


On Mon, 22 Jan 2024 22:53:48 +0200, Michael Strauß  
 wrote:



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");
}

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

[John Hendrikx]

On 22/01/2024 17:46, Jurgen Doll wrote:
I've been delving into the usage of `aborted` and `inTimePulse` as 
mentioned by John and gleaned the following:


1. stop makes a best effort to abort the 'animation' if it is in the 
process of execution.

2. `aborted` and `inTimePulse` are reset with every pulse.

As to the options that John mentioned there's also a fourth:

Accept my original proposal of fixing the NPE which is a known problem 
and not worry about potential synchronization issues. I mean does it 
really matter if play, stop, or pause miss a beat due to 
synchronization, as the API does say this could happen. Furthermore it 
doesn't appear as though the animation code can be left in some 
strange inconsistent state as a result of this.



This is not a good idea, the NPE is a symptom of the problem, but there 
can be many more subtler problems (and new ones may surface after fixing 
the NPE) that can affect other animations making use of the shared 
structures involved.  For one thing, absent a method of synchronization, 
Java doesn't even guarantee that fields (or arrays) contain the same 
value for all threads.  So your thread may modify some state, but the FX 
thread wouldn't even know about it (or only partially), and may make 
assumptions about an incorrect state, and then make further changes 
based on incorrect assumptions.


A good example is that even a much simpler class like HashMap when 
accessed by multiple threads can get into a really bad state.  It will 
usually throw a "ConcurrentModificationException" (or perhaps a weird 
NPE or IllegalStateException) to alert you that you're using it wrong -- 
but those are the **best** case scenarios... In the worst case scenario, 
modifying a HashMap on multiple threads can result in an infinite loop 
-- this can happen when two modifications occur, both updating the same 
bucket, and the threads end up changing things in such a way that the 
entries point to each other.  Iterating it then (or accessing the wrong 
key) will cause an infinite loop (not hypothetical, I've had this happen).


I'm afraid that even the synchronization option I proposed is not very 
realistic due to the amount of work it will entail.  We'd need to trace 
all code paths that a call to play or stop could get to, and all those 
code paths would need to be made thread safe.


--John



Jurgen

On Mon, 22 Jan 2024 17:58:20 +0200, John Hendrikx 
 wrote:


This seems like a reasonable use case, and perhaps this was the
original intent of the "asynchronous call" documentation.

The problem though is that the play/stop code does not seem to
take into account being called from a different thread (there are
several synchronization issues when I delved into that code).

So then there's a choice to make I think, either:

- Disallow it completely, and have users wrap it into
Platform.runLater()
- Have play/stop do the wrapping itself
- Make the methods thread safe by fixing the synchronization issues

--John

Re: 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  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

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

[Kevin Rushforth]

I would not support your proposed 4th option. It's basically a partial 
fix for the solution John mentioned as his third option.


Of the three John mentions, I favor either 1 or 2. I don't see a 
compelling reason to guarantee thread-safety for Animation objects 
(option 3), in which case the question becomes whether it is worth 
having the play*/pause/stop methods do the runLater or require the user 
to do it. The latter is more explicit, whereas the former is more 
convenient.


What do others think?

-- Kevin


On 1/22/2024 8:46 AM, Jurgen Doll wrote:
I've been delving into the usage of `aborted` and `inTimePulse` as 
mentioned by John and gleaned the following:


1. stop makes a best effort to abort the 'animation' if it is in the 
process of execution.

2. `aborted` and `inTimePulse` are reset with every pulse.

As to the options that John mentioned there's also a fourth:

Accept my original proposal of fixing the NPE which is a known problem 
and not worry about potential synchronization issues. I mean does it 
really matter if play, stop, or pause miss a beat due to 
synchronization, as the API does say this could happen. Furthermore it 
doesn't appear as though the animation code can be left in some 
strange inconsistent state as a result of this.


Jurgen

On Mon, 22 Jan 2024 17:58:20 +0200, John Hendrikx 
 wrote:


This seems like a reasonable use case, and perhaps this was the
original intent of the "asynchronous call" documentation.

The problem though is that the play/stop code does not seem to
take into account being called from a different thread (there are
several synchronization issues when I delved into that code).

So then there's a choice to make I think, either:

- Disallow it completely, and have users wrap it into
Platform.runLater()
- Have play/stop do the wrapping itself
- Make the methods thread safe by fixing the synchronization issues

--John

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

[Jurgen Doll]

I've been delving into the usage of `aborted` and `inTimePulse` as  
mentioned by John and gleaned the following:


1. stop makes a best effort to abort the 'animation' if it is in the  
process of execution.

2. `aborted` and `inTimePulse` are reset with every pulse.

As to the options that John mentioned there's also a fourth:

Accept my original proposal of fixing the NPE which is a known problem and  
not worry about potential synchronization issues. I mean does it really  
matter if play, stop, or pause miss a beat due to synchronization, as the  
API does say this could happen. Furthermore it doesn't appear as though  
the animation code can be left in some strange inconsistent state as a  
result of this.


Jurgen


On Mon, 22 Jan 2024 17:58:20 +0200, John Hendrikx  
 wrote:




This seems like a reasonable use case, and perhaps this was the original  
intent of the "asynchronous call" documentation.


The problem though is that the play/stop code does not seem to take into  
account being called from a different thread (there are several  
>synchronization issues when I delved into that code).


So then there's a choice to make I think, either:

- Disallow it completely, and have users wrap it into Platform.runLater()
- Have play/stop do the wrapping itself
- Make the methods thread safe by fixing the synchronization issues

--John

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

[John Hendrikx]

This seems like a reasonable use case, and perhaps this was the original 
intent of the "asynchronous call" documentation.


The problem though is that the play/stop code does not seem to take into 
account being called from a different thread (there are several 
synchronization issues when I delved into that code).


So then there's a choice to make I think, either:

- Disallow it completely, and have users wrap it into Platform.runLater()
- Have play/stop do the wrapping itself
- Make the methods thread safe by fixing the synchronization issues

--John

On 22/01/2024 11:59, Jurgen Doll wrote:

Here's an example as requested by Nir:

publicclassFxTimeLineTest extendsApplication

{

privateBorderPane bp= newBorderPane( newLabel("Loading") );

publicstaticvoidmain( String[] args) {

launch( FxTimeLineTest.class, args);

}

@Override

publicvoidstart( Stage primaryStage) throwsException {

newThread( newLoadScene() ).start();

primaryStage.setScene( newScene( bp, 300, 200 ) );

primaryStage.setTitle( "Memory Usage");

primaryStage.show();

}

privateclassLoadScene extendsTask {

@OverrideprotectedParent call() throwsException {

Parent p= FXMLLoader.load( getClass().getResource("TestView.fxml") );

Thread.sleep( 1000 );

returnp;

}

@Overrideprotectedvoidsucceeded() {

bp.setCenter( getValue() );

}

@Overrideprotectedvoidfailed() {

getException().printStackTrace();

}

}

}

--


publicclassTestView

{

@FXMLprivateLabel memory;

privatestaticfinaldoubleMEGABYTE= 1024 * 1024;

@FXMLprivatevoidinitialize()

{

varupdater= newTimeline

(

newKeyFrame( Duration.seconds(2.5), event->

{

varruntime= Runtime.getRuntime();

doublemaxMemory= runtime.maxMemory() / MEGABYTE;

doubleusedMemory= (runtime.totalMemory() - runtime.freeMemory()) / 
MEGABYTE;


memory.setText( (int) usedMemory+ " MB / "+ (int) maxMemory+" MB");

})

);

updater.setCycleCount(Animation.INDEFINITE); // This FXML is being 
loaded on a background thread


updater.play();

}

}

--


TestView.fxml







http://javafx.com/fxml/1; fx:controller="TestView">
















On Sat, 20 Jan 2024 17:08:41 +0200, Nir Lisker  wrote:

Hi Jurgen,

What I'm confused about the most is what it is you are actually
trying to do that necessitates the use of animations outside of
the FX thread. You said that you need to initialize controls on
another thread, and that you are using Task (both of which are
fine), but how does playing animations relate? Playing an
animation is something that is done explicitly, usually in order
to manipulate data. Can you give a real use case, like a minimized
version of what you're doing?

- Nir

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

[Jurgen Doll]

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

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

[Nir Lisker]

Hi Jurgen,

What I'm confused about the most is what it is you are actually trying to
do that necessitates the use of animations outside of the FX thread. You
said that you need to initialize controls on another thread, and that you
are using Task (both of which are fine), but how does playing animations
relate? Playing an animation is something that is done explicitly, usually
in order to manipulate data. Can you give a real use case, like a minimized
version of what you're doing?

- Nir

On Sat, Jan 20, 2024 at 2:42 AM Michael Strauß 
wrote:

> Hi Jurgen,
>
> the start, stop, pause, etc. methods on the Animation class cannot
> reasonably work in the way you suggest.
>
> While it is generally possible to construct a JavaFX object graph on a
> thread other than the JavaFX application thread, this relies on the
> fact that JavaFX will not interfere with your newly constructed object
> graph unless you attach it to a scene. An object graph is
> fundamentally single-threaded, which means that if you don't provide
> external synchronization, you're going to have undefined behavior
> under concurrent access.
>
> But that's exactly what would happen if we were to allow calling
> animation methods on a background thread. Wrapping the call in
> Platform.runLater doesn't help, because as soon as the animation call
> is dispatched to the FX thread, the animation can affect your scene
> graph in all sorts of unpredictable ways. Every attempt to access or
> modify a node after calling an animation method is an access under
> potential race, and you can't provide any external synchronization to
> make the FX thread wait until you're finished doing whatever needs to
> be done on the background thread.
>
> The fact that this sometimes seems to work when concurrent
> modifications of the node are truly orthogonal in their effects
> doesn't mean that it can work in general, and there's no way we can
> make it work in general.
> There isn't even a known set of preconditions where we are sure that
> concurrent modifications are okay, so we can't include documentation
> to that effect.
>
> Since we can't make it work in general, and we can't know the
> preconditions that may allow it to work in some cases, I think the
> best course of action is to disallow this dangerous and unpredictable
> use of this API entirely.
>
>
>
> On Fri, Jan 19, 2024 at 1:06 PM 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 

Re: [External] : Re: HEADS-UP: Threading restriction for Animation play, pause, stop now enforced

[Kevin Rushforth]

Thanks, Nir.

I see that Michael replied to this thread a short time ago with a pretty 
convincing argument for letting the change make in JDK-8159048 stand.


-- Kevin


On 1/19/2024 2:06 PM, Nir Lisker wrote:
I will look at this tomorrow due to the short time window. Haven't 
kept up with this thread.


On Fri, Jan 19, 2024 at 10:34 PM Kevin Rushforth 
 wrote:


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, 

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

[Nir Lisker]

I will look at this tomorrow due to the short time window. Haven't kept up
with this thread.

On Fri, Jan 19, 2024 at 10:34 PM Kevin Rushforth 
wrote:

> 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 

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

[Michael Strauß]

Hi Jurgen,

the start, stop, pause, etc. methods on the Animation class cannot
reasonably work in the way you suggest.

While it is generally possible to construct a JavaFX object graph on a
thread other than the JavaFX application thread, this relies on the
fact that JavaFX will not interfere with your newly constructed object
graph unless you attach it to a scene. An object graph is
fundamentally single-threaded, which means that if you don't provide
external synchronization, you're going to have undefined behavior
under concurrent access.

But that's exactly what would happen if we were to allow calling
animation methods on a background thread. Wrapping the call in
Platform.runLater doesn't help, because as soon as the animation call
is dispatched to the FX thread, the animation can affect your scene
graph in all sorts of unpredictable ways. Every attempt to access or
modify a node after calling an animation method is an access under
potential race, and you can't provide any external synchronization to
make the FX thread wait until you're finished doing whatever needs to
be done on the background thread.

The fact that this sometimes seems to work when concurrent
modifications of the node are truly orthogonal in their effects
doesn't mean that it can work in general, and there's no way we can
make it work in general.
There isn't even a known set of preconditions where we are sure that
concurrent modifications are okay, so we can't include documentation
to that effect.

Since we can't make it work in general, and we can't know the
preconditions that may allow it to work in some cases, I think the
best course of action is to disallow this dangerous and unpredictable
use of this API entirely.



On Fri, Jan 19, 2024 at 1:06 PM 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
>

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

[Kevin Rushforth]

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 

Re: 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

Re: 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

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

[Jurgen Doll]

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

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

[Jurgen Doll]

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

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

[Kevin Rushforth]

Btw, you can find the JavaFX 22+5 build here:

https://jdk.java.net/javafx22/

-- Kevin


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