HEADS-UP: Threading restriction for Animation play, pause, stop now enforced USE CASE
Kevin Rushforth
kevin.rushforth at oracle.com
Thu Jan 25 14:49:38 UTC 2024
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
> <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/96444abb/attachment-0001.htm>
More information about the openjfx-dev
mailing list