HEADS-UP: Threading restriction for Animation play, pause, stop now enforced USE CASE

[Michael Strauß]

Hi Jurgen,

even assuming that it was as easy as you say it is, there are more
problems with this proposal.

1. If non-FX thread access to some methods is allowed, this will
become a permanent addition to the API and a complication we have to
deal with. There are proposals to refactor the animation framework,
and this will become harder as a result. This also has direct
implications on the implementation (you already pointed out two
instances where fields would have to be marked volatile).
Multithreaded code is never easy, even in easy cases. Mutation
visibility will become a permanent question for future development.

2. We will need to specify that the `onFinished` handler might be
invoked on the thread that calls play() or on the FX thread, or we
need to make sure that `onFinished` is always invoked on the FX
thread.

3. Any change that would expressly allow concurrent access to the FX
framework will require lots of testing to ensure that we don't
introduce regressions or race conditions. This competes with reviewer
time for other features and fixes.

4. That being said, all of the effort might still be a good investment
if the added value was significant. But I don't see the significance
of it, as you can simply wrap the play() call in Platform.runLater and
be done with it.


On Tue, Jan 23, 2024 at 4:56 PM Jurgen Doll <javafx at ivoryemr.co.za> wrote:
>
> Hi John and others,
>
> I don't think we are entirely on the same page, so here's the objective.
>
> The Goal: To determine if the FX animation thread and a SINGLE other thread can access Animation in a safe manner wrt play, stop, and resume.
>
> Non Goal: Multi-threaded access of Animation play, stop, and resume is NOT a goal.
>
> Wrt play() and resume(), it is ALWAYS safe to call them on a background thread because at this point the Animation isn't being processed by the FX thread as the "Animation" isn't contained in the AbstractPrimaryTimer receivers array.
>
> Wrt stop() from what I can tell there are no shared method calls that need synchronization (see audit below), however the following two boolean flags should be marked as volatile in order to ensure that animation is cut short if executing:
>
> TimelineClipCore.abort
> ClipEnvelope.abort
>
> This is simple enough to add to my original proposal of replacing the arrays in AbstractPrimaryTimer with CopyOnWriteArrayList which is thread safe and replicates the intended behavior in a clear and concise way.
>
> Here are the methods that are called when stop() is invoked:
>
> Timeline.stop()
>
> getStatus()
>
> clipCore.abort()
>
> isStopped()
>
> clipEnvelope.abortCurrentPulse()
>
> doStop()
>
> timer.removePulseReceiver(pulseReceiver);
>
> // After this point it doesn't matter
>
> setStatus(Status.STOPPED)
>
> doSetCurrentRate(0.0)
>
> jumpTo(Duration.ZERO)
>
>
> And for AbstractPrimaryTimer.timePulseImpl:
>
> AbstractPrimaryTimer.timePulseImpl
>
> Animation.PulseReceiver.timePulse
>
> Animation.doTimePulse
>
> clipEnvelope.timePulse
>
> animation.doPlayTo
>
> clipCore.playTo
>
> visitKeyFrame
>
> setTime
>
> animation.setCurrentTicks
>
> clipInterpolator.interpolate
>
> animation.doJumpTo
>
> sync(false)
>
> setCurrentTicks
>
> clipCore.jumpTo
>
> timeline.getStatus()
>
> clipInterpolator.interpolate
>
>
> Regards
> Jurgen