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.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
