Hi Jurgen,
the start, stop, pause, etc. methods on the Animation class cannot
reasonably work in the way you suggest.
While it is generally possible to construct a JavaFX object graph on a
thread other than the JavaFX application thread, this relies on the
fact that JavaFX will not interfere with your newly constructed object
graph unless you attach it to a scene. An object graph is
fundamentally single-threaded, which means that if you don't provide
external synchronization, you're going to have undefined behavior
under concurrent access.
But that's exactly what would happen if we were to allow calling
animation methods on a background thread. Wrapping the call in
Platform.runLater doesn't help, because as soon as the animation call
is dispatched to the FX thread, the animation can affect your scene
graph in all sorts of unpredictable ways. Every attempt to access or
modify a node after calling an animation method is an access under
potential race, and you can't provide any external synchronization to
make the FX thread wait until you're finished doing whatever needs to
be done on the background thread.
The fact that this sometimes seems to work when concurrent
modifications of the node are truly orthogonal in their effects
doesn't mean that it can work in general, and there's no way we can
make it work in general.
There isn't even a known set of preconditions where we are sure that
concurrent modifications are okay, so we can't include documentation
to that effect.
Since we can't make it work in general, and we can't know the
preconditions that may allow it to work in some cases, I think the
best course of action is to disallow this dangerous and unpredictable
use of this API entirely.
On Fri, Jan 19, 2024 at 1:06 PM Jurgen Doll <jav...@ivoryemr.co.za> wrote:
>
> Hi Kevin
>
> I was hoping that others would way in on this fix (PR #1167), but now that
> we're in RDP1 with no other input coming in I decided to looked into this
> matter again and have found that this is not the correct solution for the
> following two reasons:
>
> 1. The current solution doesn't actually fix the bug, but merely avoids it:
>
> Specifically with regards to bug JDK-8159048 which reports a NPE occuring
> under some conditions in AbstractMasterTimer (subsequently renamed to
> AbstractPrimaryTimer). The reporter suggests that this is as a result of
> some concurrent modification occurring and suggests a workaround of
> wrapping the start/stop animation code in a Platform.runLater() call.
>
> Looking at the AbstractPrimaryTimer code it is apparent that the original
> author made an effort to isolate array modifications from array access.
> However this code has a bug in it which then manifests as a NPE under the
> right timing conditions. So the correct solution is to make the array
> modification code more robust, as apposed to forcing changes to occur on a
> single thread.
>
> The safest (and proper) solution is to convert the two arrays ("receivers"
> and "animationTimers") to be CopyOnWriteArrayList(s) which is an ideal JDK
> data structure for this use case as it replicates the intended behavior of
> the current implementation. (I've tried this out and it does solve the NPE
> problem.)
>
> 2. The current solution is based on the misconception that start, stop,
> and pause must occur on the FX thread:
>
> Specifically with regards to the CSR JDK-8313378 which makes this claim.
>
> Firstly the Animation API says explicitly that these methods are
> asynchronous, which means that the thread that invokes these methods and
> the actual execution of the animation occurs on different threads, and
> looking at AbstractPrimaryTimer code it can be seen that this is indeed
> the case.
>
> Secondly JavaFX had tests, as noted in JDK-8314266, that have run reliably
> for years without invoking these methods on the FX thread. FWIW I've also
> had code and used libraries for years now, where these methods have been
> invoked on a background thread (for example while loading FXML) without
> issue.
>
>
> In conclusion then I request permission to submit a new PR containing the
> following changes:
>
> 1. Revert PR #1167 (if this is the appropriate place, however the test
> case will require it)
> 2. Changing the arrays in AbstractPrimaryTimer to be
> CopyOnWriteArrayList(s) and removing previously supporting array code.
> 3. Adding a test based on the one supplied in JDK-8159048 to check that a
> NPE isn't thrown anymore.
>
> Hope this meets with your approval.
> Regards,
> Jurgen
>
