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

[John Hendrikx]

Hi Jurgen,

See my answers inline.

On 24/01/2024 10:12, Jurgen Doll wrote:
> Hi John
>
> Thank you for the hypothetical receivers array scenario, I think it 
> explains the problem exactly and is why replacing the array with 
> CopyOnWriteArrayList removes the NPE.
>
> Your perspective then is that AbstractPrimaryTimer is designed for 
> single threaded use only.

Yes, it looks that way simply because I see none of the usual 
concurrency controls (Locks, synchronized, volatile), and of course it 
fits with the general design of FX where almost all FX code is assumed 
to be accessed from a single thread.

However, there are issues even with a single thread that must be taken 
care of, see below.

> If that is indeed so, then could you please explain the purpose of the 
> receiversLocked and animationTimersLocked flags, as well as the point 
> of receivers.clone() and animationTimers.clone() all of which indicate 
> to the contrary.

This is a pattern that we can also see in other areas of FX (see 
ExpressionHelper for example).  The clones and the use of a 
(non-synchronized) locking flag are to prevent issues with recursive 
calls on the same thread: During the notifying of all registered pulse 
receivers, the list is locked.  If one of those receivers decides to 
remove itself *during* the notification callback, then the list would be 
modified while iterating (as more receivers may need notifying still).  
If that happens, it will instead see that the list is currently locked, 
and make a clone.

Replacing the array with a CopyOnWriteArrayList would mean that copies 
will be made even when not needed.  Since the code seems to go through 
great lengths to even avoid the super fast ArrayList implementation 
(which IMHO would have been perfectly acceptable here) I can't help but 
wonder if that could have significant performance implications.  Perhaps 
pulse receivers are added/removed far more frequently than we would think.

--John



>
> Thanks, regards
> Jurgen
>
>
> On Tue, 23 Jan 2024 18:36:16 +0200, John Hendrikx 
> <john.hendrikx at gmail.com> wrote:
>
>     On 23/01/2024 16:56, Jurgen Doll 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.
>     The number of threads is irrelevant really, it's either thread
>     safe or it isn't.
>>
>>     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.
>
>     I'm afraid that is incorrect.  The fact that your animation is not
>     running doesn't mean that AbstractPrimaryTimer isn't in use by
>     other animations that are running.  These other animations are
>     using the receivers array, and may be modifying it or reading from
>     it from the FX thread.
>
>     When you start your animation on a different thread, you are
>     accessing these fields with a different thread, and modifying
>     them.  Since the JVM is free to cache values and do other fancy
>     things (like reordering read/writes) in the absence of
>     synchronized/locking, there is no guarantee that the FX thread
>     will see those modifications until these are flushed to main
>     memory (or caches are synced).  Even then, the FX thread may have
>     these values cached somewhere, and so it may not go all the way to
>     main memory to see if its assumptions are now incorrect.  The only
>     way to ensure this is with proper use of synchronization.
>
>     So a hypothetical scenario:
>
>     - AbstractPrimaryTimer has no receivers
>     - A receiver is added via the FX thread, slot 0 is now filled and
>     receiversLength is now 1.  Due to cache lines being large, it also
>     read slot 1 (which is null)
>     - You start your animation on another thread.  Since you didn't
>     see the receivers array yet, you may see one of these states:
>         - The change from the FX thread was flushed to main memory,
>     and you see [X, null] and receiversLength = 1
>         - The change from the FX thread was partially flushed, and you
>     see [X, null]  and receiversLength = 0 (!!)
>         - Nothing was flushed yet, and you see [null, null] and
>     receiversLength = 0
>
>     Now, you can see that it would be very dangerous to proceed to
>     modify the array based on half flushed information. Something
>     similar happens when you are the first to start an animation, and
>     then another is started later.  If the changes of your thread are
>     not flushed yet (or partially) then the FX thread will act on
>     partially flushed data, or even see no receivers yet at all...
>
>     --John
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/openjfx-dev/attachments/20240124/f8287cbd/attachment-0001.htm>