<!DOCTYPE html><html><head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body>
And that's why the "be careful" option is not a satisfying solution.<br>
<br>
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.<br>
<br>
-- Kevin<br>
<br>
<br>
<div class="moz-cite-prefix">On 1/25/2024 5:46 AM, Nir Lisker wrote:<br>
</div>
<blockquote type="cite" cite="mid:CA+0ynh8z-PzEhJ58tWhLMeAfj7J-zS=CR7wXF6ksPsiK+f=tpA@mail.gmail.com">
<div dir="ltr">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Option
3 is basically document it as "be careful" and remove the
thread restriction recently introduced, am I correct?<br>
</blockquote>
<div><br>
</div>
<div>Yes :)</div>
<div><br>
</div>
<div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">IMHO
that can simply can't work at all, because this is what
(theoretically) can happen:<br>
1. Non-FX thread starts animation<br>
- Fields are manipulated in AbstractPrimaryTimer<br>
- There is no synchronization, so no guarantee anything
is flushed (it may all reside in CPU caches)</blockquote>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
2. FX Thread becomes involved to actually process the
animation<br>
- Sees partially flushed state fields, and acts on
garbage information (ie. number of animations > 0, but
array contains only null's)<br>
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).</blockquote>
</div>
<div> </div>
<div>What about something like a PauseTransition that doesn't
manipulate properties?</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Thu, Jan 25, 2024 at
11:02 AM John Hendrikx <<a href="mailto:john.hendrikx@gmail.com" target="_blank" moz-do-not-send="true" class="moz-txt-link-freetext">john.hendrikx@gmail.com</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div>
<p><br>
</p>
<div>On 24/01/2024 22:06, Nir Lisker wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">This is the ballpark of what I meant with
"the downside might be some surprise when these methods
behave differently".</div>
</blockquote>
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).<br>
<blockquote type="cite">
<div dir="ltr">
<div><br>
</div>
<div>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?</div>
</div>
</blockquote>
<p>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.<br>
</p>
<p>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.<br>
</p>
<br>
<blockquote type="cite">
<div dir="ltr">
<div>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?</div>
<div><br>
</div>
<div>If you want to be able to run an animation from a
background thread with no race conditions, why not opt
for option 3?</div>
</div>
</blockquote>
<p>Option 3 is basically document it as "be careful" and
remove the thread restriction recently introduced, am I
correct?</p>
<p>IMHO that can simply can't work at all, because this is
what (theoretically) can happen:</p>
<p>1. Non-FX thread starts animation<br>
- Fields are manipulated in AbstractPrimaryTimer<br>
- There is no synchronization, so no guarantee
anything is flushed (it may all reside in CPU caches)<br>
<br>
2. FX Thread becomes involved to actually process the
animation<br>
- Sees partially flushed state fields, and acts on
garbage information (ie. number of animations > 0, but
array contains only null's)</p>
<p>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).</p>
<p>--John<br>
</p>
<blockquote type="cite">
<div dir="ltr">
<div><br>
</div>
<div>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.</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Wed, Jan 24, 2024
at 9:04 PM Michael Strauß <<a href="mailto:michaelstrau2@gmail.com" target="_blank" moz-do-not-send="true" class="moz-txt-link-freetext">michaelstrau2@gmail.com</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">I
am not in favor of option 2 if the implementation was
simply "wrap<br>
the implementation in runLater", as this would be a
surprising change.<br>
Consider the following code:<br>
<br>
var transition = new FadeTransition();<br>
transition.play();<br>
<br>
// Will always print "RUNNING" currently, but
might print "STOPPED"<br>
// if we go with the proposed change:<br>
System.out.println(transition.getStatus());<br>
<br>
Regardless of the race condition in
AbstractPrimaryTimer, this code<br>
always seemed to be working quite fine (albeit
superficially), because<br>
the play/stop/etc. methods change that status of the
animation as one<br>
would expect.<br>
<br>
You are proposing to change that, such that calling
these methods will<br>
not predictably change the status of the animation.
Instead, these<br>
methods now act more like "requests" that may be
fulfilled at some<br>
later point in time, rather than statements of fact.<br>
This is not a change that I think we should be doing
on an ad-hoc<br>
basis, since the same considerations potentially apply
for many<br>
methods in many places.<br>
<br>
If we were to allow calling play/stop/etc. on a
background thread, I<br>
would be in favor of keeping the semantics that these
methods<br>
instantly and predictably affect the status of the
animation. Only the<br>
internal operation of adding the animation to
AbstractPrimaryTimer<br>
should be posted to the FX thread. If that is not
possible, this<br>
suggests to me that we should choose option 1.
Introducing new and<br>
surprising semantics to an existing API is not the way
to go.<br>
<br>
<br>
On Wed, Jan 24, 2024 at 7:27 PM Nir Lisker <<a href="mailto:nlisker@gmail.com" target="_blank" moz-do-not-send="true" class="moz-txt-link-freetext">nlisker@gmail.com</a>>
wrote:<br>
><br>
> These are the options I see as reasonable:<br>
><br>
> 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.<br>
> 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.<br>
> 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.<br>
><br>
> 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.<br>
> 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.<br>
</blockquote>
</div>
</blockquote>
</div>
</blockquote>
</div>
</blockquote>
<br>
</body>
</html>