<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">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">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">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>