I will look at this tomorrow due to the short time window. Haven't kept up
with this thread.
On Fri, Jan 19, 2024 at 10:34 PM Kevin Rushforth <kevin.rushfo...@oracle.com>
wrote:
> 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
>
>
