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ß
<michaelstr...@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 <nlis...@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.
