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 <john.hendrikx at gmail.com>
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ß <michaelstrau2 at gmail.com>
> 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 <nlisker at gmail.com> 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.
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/openjfx-dev/attachments/20240125/1da911fb/attachment-0001.htm>