> > 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.hendr...@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ß <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. >> >
