[External] : Re: HEADS-UP: Threading restriction for Animation play, pause, stop now enforced

Fri Jan 19 22:10:03 UTC 2024

Thanks, Nir.

I see that Michael replied to this thread a short time ago with a pretty 
convincing argument for letting the change make in JDK-8159048 stand.

-- Kevin

On 1/19/2024 2:06 PM, Nir Lisker wrote:
> 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.rushforth at 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/openjfx-dev/attachments/20240119/86edab31/attachment.htm>