Hi Jurgen,
There is a very short window for making any changes like this that
require a CSR in JavaFX 22. If we were to do what you suggest, we would
need to:
1. File a new Enhancement request to:
-- revert JDK-8159048 (except for the doc changes about Animation being
run on the FX application thread, which is correct and useful even after
the revert)
-- update the documentation to indicate that the play/pause/stop methods
may be called on any thread
-- fix the implementation to wrap the implementation of play/pause/stop
in Platform.runLater, if not on the FX app thread
-- we may or may not also want to make the additional fix you suggest
in AbstractPrimaryTimer, but this might not be needed if we wrap the
call in a runLater, which I think should be done anyway
2. File a CSR for the spec changes for the above
All of this would need to be agreed upon and proposed within the next
few days to allow time for the CSR to be considered and approved while
we are still in RDP1. I would not consider such a change once we hit
RDP2 on Feb 1.
Before even considering this, I'd like to hear from Johan Vos, Jose
Pereda (who implemented JDK-8159048), Nir Lisker (who has done a lot of
work on animation), and Michael Strauß (who has proposed in JDK-8324219
to update the documentation to remove the reference to the fact that
stop is asynchronous), and any others who might have an interest. Note
that if we go down the route of reverting JDK-8159048, then we will
close JDK-8324219 as "not an issue".
If there is general agreement, then it would seem reasonable to shoot
for JavaFX 22. I note that it would be a low- risk change -- basically
going back to the behavior we have in JavaFX 21, which is the latest GA
release.
-- Kevin
On 1/19/2024 4:06 AM, Jurgen Doll 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
On 8/18/2023 4:17 PM, Kevin Rushforth wrote:
As a heads-up for app developers who use JavaFX animation (including
Animation, along with any subclasses, and AnimationTimer), a change
went into the JavaFX 22+5 build to enforce that the play, pause, and
stop methods must be called on the JavaFX Application thread.
Applications should have been doing that all along (else they would
have been subject to unpredictable errors), but for those who aren't
sure, you might want to take 22+5 for a spin and see if you have any
problems with your application. Please report them on the list if you
do.
See JDK-8159048 [1] and CSR JDK-8313378 [2] for more information on
this change.
Thanks.
-- Kevin
[1] https://bugs.openjdk.org/browse/JDK-8159048
[2] https://bugs.openjdk.org/browse/JDK-8313378
