On Thu, 12 Feb 2026 23:03:31 GMT, Florian Kirmaier <[email protected]> 
wrote:

> Add `synchronized(lock)` around array mutations in 
> `addPulseReceiver`/`removePulseReceiver`/`addAnimationTimer`/`removeAnimationTimer`
>  and around snapshot-taking in `timePulseImpl`. Iteration remains outside the 
> lock. `updateAnimationRunnable()` is also called outside the lock to avoid 
> nested locking.
> 
> This preserves the existing copy-on-write design - the lock just ensures it 
> works correctly across threads. Performance impact is minimal: the lock only 
> covers field reads/writes, not the per-frame iteration.
> 
> **Testing:**
> New `AbstractPrimaryTimerThreadSafetyTest` with 
> `testConcurrentAddAnimationTimer` - 8 threads add timers simultaneously, 
> repeated 100 times. Fails 100% without fix. Reuses `AbstractPrimaryTimerStub` 
> from existing tests.
> 
> All existing animation tests pass.
> 
> In JPro, this often caused a Deadlock during startup.
> This might have caused many bugs, which are very hard to reproduce.

At first glance, this doesn't seem to be the correct solution for the bug. 
`AbstractPrimaryTimer` is not thread-safe under concurrent modification, and 
assumes that it is only ever used on the FX thread. The `addAnimationTimer()` 
method is only called by `AnimationTimer.startImpl()`, which itself is only 
called by `Utils.runOnFxThread(this::startImpl)`.

So the question is: why do you observe a background thread calling out to 
`addAnimationTimer()`, when the only caller tries to ensure that this won't 
ever happen?

-------------

PR Comment: https://git.openjdk.org/jfx/pull/2074#issuecomment-3901951536

Reply via email to