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