<!DOCTYPE html><html><head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body>
Thanks, Nir.<br>
<br>
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.<br>
<br>
-- Kevin<br>
<br>
<br>
<div class="moz-cite-prefix">On 1/19/2024 2:06 PM, Nir Lisker wrote:<br>
</div>
<blockquote type="cite" cite="mid:CA+0ynh_jL0xFodxDwEFc9sFXQJ1eaoZsFDiHBxVmyN6KpJKq=Q@mail.gmail.com">
<div dir="ltr">I will look at this tomorrow due to the short time
window. Haven't kept up with this thread.</div>
<br>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Fri, Jan 19, 2024 at
10:34 PM Kevin Rushforth <<a href="mailto:kevin.rushforth@oracle.com" moz-do-not-send="true" class="moz-txt-link-freetext">kevin.rushforth@oracle.com</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi
Jurgen,<br>
<br>
There is a very short window for making any changes like this
that <br>
require a CSR in JavaFX 22. If we were to do what you suggest,
we would <br>
need to:<br>
<br>
1. File a new Enhancement request to:<br>
-- revert JDK-8159048 (except for the doc changes about
Animation being <br>
run on the FX application thread, which is correct and useful
even after <br>
the revert)<br>
-- update the documentation to indicate that
the play/pause/stop methods <br>
may be called on any thread<br>
-- fix the implementation to wrap the implementation of
play/pause/stop <br>
in Platform.runLater, if not on the FX app thread<br>
-- we may or may not also want to make the additional fix you
suggest <br>
in AbstractPrimaryTimer, but this might not be needed if we
wrap the <br>
call in a runLater, which I think should be done anyway<br>
<br>
2. File a CSR for the spec changes for the above<br>
<br>
All of this would need to be agreed upon and proposed within
the next <br>
few days to allow time for the CSR to be considered and
approved while <br>
we are still in RDP1. I would not consider such a change once
we hit <br>
RDP2 on Feb 1.<br>
<br>
Before even considering this, I'd like to hear from Johan Vos,
Jose <br>
Pereda (who implemented JDK-8159048), Nir Lisker (who has done
a lot of <br>
work on animation), and Michael Strauß (who has proposed in
JDK-8324219 <br>
to update the documentation to remove the reference to the
fact that <br>
stop is asynchronous), and any others who might have an
interest. Note <br>
that if we go down the route of reverting JDK-8159048, then we
will <br>
close JDK-8324219 as "not an issue".<br>
<br>
If there is general agreement, then it would seem reasonable
to shoot <br>
for JavaFX 22. I note that it would be a low- risk change --
basically <br>
going back to the behavior we have in JavaFX 21, which is the
latest GA <br>
release.<br>
<br>
-- Kevin<br>
<br>
<br>
On 1/19/2024 4:06 AM, Jurgen Doll wrote:<br>
> Hi Kevin<br>
><br>
> I was hoping that others would way in on this fix (PR
#1167), but now <br>
> that we're in RDP1 with no other input coming in I
decided to looked <br>
> into this matter again and have found that this is not
the correct <br>
> solution for the following two reasons:<br>
><br>
> 1. The current solution doesn't actually fix the bug, but
merely <br>
> avoids it:<br>
><br>
> Specifically with regards to bug JDK-8159048 which
reports a NPE <br>
> occuring under some conditions in AbstractMasterTimer
(subsequently <br>
> renamed to AbstractPrimaryTimer). The reporter suggests
that this is <br>
> as a result of some concurrent modification occurring and
suggests a <br>
> workaround of wrapping the start/stop animation code in a
<br>
> Platform.runLater() call.<br>
><br>
> Looking at the AbstractPrimaryTimer code it is apparent
that the <br>
> original author made an effort to isolate array
modifications from <br>
> array access. However this code has a bug in it which
then manifests <br>
> as a NPE under the right timing conditions. So the
correct solution is <br>
> to make the array modification code more robust, as
apposed to forcing <br>
> changes to occur on a single thread.<br>
><br>
> The safest (and proper) solution is to convert the two
arrays <br>
> ("receivers" and "animationTimers") to be
CopyOnWriteArrayList(s) <br>
> which is an ideal JDK data structure for this use case as
it <br>
> replicates the intended behavior of the current
implementation. (I've <br>
> tried this out and it does solve the NPE problem.)<br>
><br>
> 2. The current solution is based on the misconception
that start, <br>
> stop, and pause must occur on the FX thread:<br>
><br>
> Specifically with regards to the CSR JDK-8313378 which
makes this claim.<br>
><br>
> Firstly the Animation API says explicitly that these
methods are <br>
> asynchronous, which means that the thread that invokes
these methods <br>
> and the actual execution of the animation occurs on
different threads, <br>
> and looking at AbstractPrimaryTimer code it can be seen
that this is <br>
> indeed the case.<br>
><br>
> Secondly JavaFX had tests, as noted in JDK-8314266, that
have run <br>
> reliably for years without invoking these methods on the
FX thread. <br>
> FWIW I've also had code and used libraries for years now,
where these <br>
> methods have been invoked on a background thread (for
example while <br>
> loading FXML) without issue.<br>
><br>
><br>
> In conclusion then I request permission to submit a new
PR containing <br>
> the following changes:<br>
><br>
> 1. Revert PR #1167 (if this is the appropriate place,
however the test <br>
> case will require it)<br>
> 2. Changing the arrays in AbstractPrimaryTimer to be <br>
> CopyOnWriteArrayList(s) and removing previously
supporting array code.<br>
> 3. Adding a test based on the one supplied in JDK-8159048
to check <br>
> that a NPE isn't thrown anymore.<br>
><br>
> Hope this meets with your approval.<br>
> Regards,<br>
> Jurgen<br>
><br>
><br>
>> On 8/18/2023 4:17 PM, Kevin Rushforth wrote:<br>
>> As a heads-up for app developers who use JavaFX
animation (including <br>
>> Animation, along with any subclasses, and
AnimationTimer), a change <br>
>> went into the JavaFX 22+5 build to enforce that the
play, pause, and <br>
>> stop methods must be called on the JavaFX Application
thread. <br>
>> Applications should have been doing that all along
(else they would <br>
>> have been subject to unpredictable errors), but for
those who aren't <br>
>> sure, you might want to take 22+5 for a spin and see
if you have any <br>
>> problems with your application. Please report them on
the list if you <br>
>> do.<br>
>><br>
>> See JDK-8159048 [1] and CSR JDK-8313378 [2] for more
information on <br>
>> this change.<br>
>><br>
>> Thanks.<br>
>><br>
>> -- Kevin<br>
>><br>
>> [1] <a href="https://bugs.openjdk.org/browse/JDK-8159048" rel="noreferrer" target="_blank" moz-do-not-send="true" class="moz-txt-link-freetext">https://bugs.openjdk.org/browse/JDK-8159048</a><br>
>> [2] <a href="https://bugs.openjdk.org/browse/JDK-8313378" rel="noreferrer" target="_blank" moz-do-not-send="true" class="moz-txt-link-freetext">https://bugs.openjdk.org/browse/JDK-8313378</a><br>
<br>
</blockquote>
</div>
</blockquote>
<br>
</body>
</html>