<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p>Hi Jurgen,</p>
<p>See my answers inline.<br>
</p>
<div class="moz-cite-prefix">On 24/01/2024 10:12, Jurgen Doll wrote:<br>
</div>
<blockquote type="cite" cite="mid:op.2h1nvi1q0ae2rt@admin-pc">
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
<style type="text/css">body { font-family:'Calibri'; font-size:13px}</style>
<div><big>Hi John<br>
<br>
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.<br>
<br>
Your perspective then is that AbstractPrimaryTimer is designed
for single threaded use only. <br>
</big></div>
</blockquote>
<p>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. <br>
</p>
<p>However, there are issues even with a single thread that must be
taken care of, see below.<br>
</p>
<blockquote type="cite" cite="mid:op.2h1nvi1q0ae2rt@admin-pc">
<div><big>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.<br>
</big></div>
</blockquote>
<p>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.<br>
</p>
<p>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. <br>
</p>
--John<br>
<p><br>
</p>
<p><br>
</p>
<blockquote type="cite" cite="mid:op.2h1nvi1q0ae2rt@admin-pc">
<div><big><br>
Thanks, regards<br>
Jurgen</big></div>
<div><big>
</big>
<div><big><br>
</big></div>
</div>
<div><br>
</div>
<div>On Tue, 23 Jan 2024 18:36:16 +0200, John Hendrikx
<a class="moz-txt-link-rfc2396E" href="mailto:john.hendrikx@gmail.com"><john.hendrikx@gmail.com></a> wrote:<br>
</div>
<br>
<blockquote style="margin: 0 0 0.80ex; border-left: #0000FF 2px
solid; padding-left: 1ex">
<div class="moz-cite-prefix">On 23/01/2024 16:56, Jurgen Doll
wrote:<br>
</div>
<blockquote type="cite" cite="mid:op.2h0bwcdn0ae2rt@admin-pc">
<style type="text/css">body { font-family:'Calibri'; font-size:13px}</style>
<div><big>Hi John and others,</big></div>
<div><big><br>
</big></div>
<div><big>I don't think we are entirely on the same page, so
here's the objective.<br>
<br>
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.<br>
</big></div>
</blockquote>
The number of threads is irrelevant really, it's either thread
safe or it isn't.<br>
<blockquote type="cite" cite="mid:op.2h0bwcdn0ae2rt@admin-pc">
<div><big><br>
Non Goal: Multi-threaded access of Animation play, stop,
and resume is NOT a goal.<br>
<br>
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.<br>
</big></div>
</blockquote>
<p>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.</p>
<p>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.</p>
<p>So a hypothetical scenario:</p>
<p>- AbstractPrimaryTimer has no receivers<br>
- 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)<br>
- You start your animation on another thread. Since you
didn't see the receivers array yet, you may see one of these
states:<br>
- The change from the FX thread was flushed to main
memory, and you see [X, null] and receiversLength = 1<br>
- The change from the FX thread was partially flushed, and
you see [X, null] and receiversLength = 0 (!!)<br>
- Nothing was flushed yet, and you see [null, null] and
receiversLength = 0</p>
<p>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...<br>
</p>
<p>--John</p>
</blockquote>
</blockquote>
</body>
</html>