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

[Jurgen Doll]

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


On Tue, 23 Jan 2024 01:52:42 +0200, John Hendrikx  
<john.hendrikx at gmail.com> wrote:
>
> This is not a good idea, the NPE is a symptom of the problem, but there  
> can be many more subtler problems (and new ones may surface after  
> >fixing the NPE) that can affect other animations making use of the  
> shared structures involved.  For one thing, absent a method of  
> >synchronization, Java doesn't even guarantee that fields (or arrays)  
> contain the same value for all threads.  So your thread may modify some  
> >state, but the FX thread wouldn't even know about it (or only  
> partially), and may make assumptions about an incorrect state, and then  
> make >further changes based on incorrect assumptions.
>
> A good example is that even a much simpler class like HashMap when  
> accessed by multiple threads can get into a really bad state.  It will  
> >usually throw a "ConcurrentModificationException" (or perhaps a weird  
> NPE or IllegalStateException) to alert you that you're using it wrong  
> >-- but those are the **best** case scenarios... In the worst case  
> scenario, modifying a HashMap on multiple threads can result in an  
> infinite >loop -- this can happen when two modifications occur, both  
> updating the same bucket, and the threads end up changing things in such  
> a way >that the entries point to each other.  Iterating it then (or  
> accessing the wrong key) will cause an infinite loop (not hypothetical,  
> I've had this >happen).
>
> I'm afraid that even the synchronization option I proposed is not very  
> realistic due to the amount of work it will entail.  We'd need to trace  
> all >code paths that a call to play or stop could get to, and all those  
> code paths would need to be made thread safe.
>
> --John
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/openjfx-dev/attachments/20240123/c9e581d5/attachment.htm>