[External] : Re: HEADS-UP: Threading restriction for Animation play, pause, stop now enforced USE CASE
Kevin Rushforth
kevin.rushforth at oracle.com
Wed Jan 24 19:00:27 UTC 2024
Thanks!
I just filed https://bugs.openjdk.org/browse/JDK-8324658
It should mostly revert JDK-8159048 (although the sentence added to the
class docs about animations running on the JavaFX app thread is still
valid), and some of the unit tests might still be valid.
-- Kevin
On 1/24/2024 10:50 AM, Nir Lisker wrote:
> If there's an agreement, I can do it tomorrow. It will effectively
> revert JDK-8159048 and supersede JDK-8324219.
>
> On Wed, Jan 24, 2024 at 8:42 PM Kevin Rushforth
> <kevin.rushforth at oracle.com> wrote:
>
> I also now favor option 2 and was in the process of writing
> something up recommending that we do that. Phil and I (and a
> couple others) had an offline discussion and think this is the way
> to go.
>
> Given the short amount of time to get this into 22, I will file
> the JBS issue in the next hour or so.
>
> Nir: if you want to take it, that would be great. Otherwise, I
> will do it. We need the PR and CSR filed before the end of this week.
>
> Regarding other methods that choose option 1, many of them are
> more complicated (e.g., scene mutation can be done on a background
> thread as long as the scene is not "showing") or must be
> synchronous. Animation starts something that conceptually happens
> later on the FX animation thread anyway, so wrapping it in a
> runLater (if not already on the right thread) doesn't really
> change the semantics in an appreciable way.
>
> -- Kevin
>
>
> On 1/24/2024 10:26 AM, Nir Lisker wrote:
>> These are the options I see as reasonable:
>>
>> 1. Document that these methods *must* be run on the FX thread and
>> throw an exception otherwise. This leaves it to the
>> responsibility of the user. However, this will cause the
>> backwards compatibility problems that Jugen brought up. As a side
>> note, we do this in other methods already, but I always
>> questioned why let the developer do something illegal - if
>> there's only one execution path, force it.
>> 2. Document that these methods *are* run on the FX thread (the
>> user doesn't need to do anything) and change the implementation
>> to call runLater(...) internally unless they are already on the
>> FX thread. This will be backwards compatible for the most part
>> (see option 3). The downside might be some surprise when these
>> methods behave differently.
>> 3. Document that it's *recommended* that these methods be run on
>> the FX thread and let the user be responsible for the threading.
>> We can explain that manipulating nodes that are attached to an
>> active scenegraph should be avoided.
>>
>> I prefer option 2 over 1 regardless of the backwards
>> compatibility issue even, but would like to know if I'm missing
>> something here because in theory this change could be done to any
>> "must run on the FX thread" method and I question why the user
>> had the option to get an exception.
>> Option 3 is risky and I wager a guess that it will be used
>> wrongly more often than not. It does allow some (what I would
>> call) valid niche uses. I never did it.
>>
>> On Wed, Jan 24, 2024 at 4:21 PM Kevin Rushforth
>> <kevin.rushforth at oracle.com> wrote:
>>
>> I'd like to hear from the others on this. I don't see any
>> fundamental problem with having the play/pause/stop methods
>> wrap their implementation in a runLater (if not on the FX
>> Application thread already), and documenting that it does so,
>> if we can get general agreement.
>>
>> -- Kevin
>>
>>
>> On 1/24/2024 5:29 AM, Jurgen Doll wrote:
>>> Hi Kevin
>>>
>>> If I may make one more final appeal then to an alternative
>>> solution please.
>>>
>>> Could we then instead of throwing an Exception rather invoke
>>> runLater if needed inside play, stop, and resume.
>>>
>>> Putting the onus on the developer is fine if it is the
>>> developer that is invoking the call, but if it's in a
>>> library then it's a no go.
>>>
>>> In my application I have two libraries that I know of where
>>> this happens. The major problem is that with FX22 as it now
>>> stands my application just crashes because play() does an FX
>>> thread check and throws an Exception which it never did
>>> before. There are bound to be other applications out there
>>> that are going to find themselves in a similar position.
>>>
>>> PLEASE !
>>>
>>> Regards
>>> Jurgen
>>>
>>>
>>>
>>> On Wed, 24 Jan 2024 15:15:31 +0200, Kevin Rushforth
>>> <kevin.rushforth at oracle.com>
>>> <mailto:kevin.rushforth at oracle.com> wrote:
>>>
>>> Thank you to Jurgen for raising the question and to Nir,
>>> John, and Michael for evaluating it.
>>>
>>> I conclude that there is insufficient motivation to
>>> revert the change in behavior implemented by JDK-8159048
>>> to allow calling the play/pause/stop methods of
>>> Animation on a background thread. Doing so without
>>> making it fully multi-thread-safe would be asking for
>>> problems, and making it fully multi-thread-safe would be
>>> a fair bit of work to do it right without a clear benefit.
>>>
>>> We will proceed with the current approach and let
>>> JDK-8159048 stand. Further, we will proceed with
>>> https://bugs.openjdk.org/browse/JDK-8324219 which is
>>> under review in https://github.com/openjdk/jfx/pull/1342
>>> <https://urldefense.com/v3/__https://github.com/openjdk/jfx/pull/1342__;!!ACWV5N9M2RV99hQ!PLtaZAvaobOk-fe_DRSy2AHiPOFUaK683TVXWIvmjnTiUGhmBp3x_GIy_wlMuEV3IHTUMx37HxJHiVuWiFRR$>
>>>
>>> -- Kevin
>>>
>>> On 1/24/2024 12:30 AM, Nir Lisker wrote:
>>>> After playing around with the code sample, I think that
>>>> this is not the right way to use the animation. The
>>>> reason is that there is no point in starting the
>>>> animation before the control is attached to the
>>>> scenegraph, or even made visible. A small refactoring
>>>> where, e.g., the controller class exposes a method to
>>>> start the animation in onSucceeded or just calls it on
>>>> the FX thread is enough. I never start an animation as
>>>> part of the construction because it's not the right
>>>> time. John suggested tying the lifecycle of the
>>>> animation to the showing of the node, which also solves
>>>> the problem.
>>>>
>>>> There are animations like PauseTransition or other
>>>> non-interfering Timelines that could reasonably be run
>>>> on a background thread. Or maybe just on an unconnected
>>>> control. This could be a reason to not limit animation
>>>> methods to the FX thread at the expense of possible
>>>> user errors, but document the pitfall.
>>>>
>>>> I don't see a good use case for modifying controls in a
>>>> background thread while still interacting with the
>>>> scenegraph, hence for adding multithread support.
>>>>
>>>> - Nir
>>>>
>>>> On Mon, Jan 22, 2024, 12:59 Jurgen Doll
>>>> <javafx at ivoryemr.co.za> wrote:
>>>>
>>>> Here's an example as requested by Nir:
>>>>
>>>> publicclassFxTimeLineTest extendsApplication
>>>>
>>>> {
>>>>
>>>> privateBorderPane bp= newBorderPane(
>>>> newLabel("Loading") );
>>>>
>>>> publicstaticvoidmain( String[] args) {
>>>>
>>>> launch( FxTimeLineTest.class, args);
>>>>
>>>> }
>>>>
>>>> @Override
>>>>
>>>> publicvoidstart( Stage primaryStage) throwsException {
>>>>
>>>> newThread( newLoadScene() ).start();
>>>>
>>>> primaryStage.setScene( newScene( bp, 300, 200 ) );
>>>>
>>>> primaryStage.setTitle( "Memory Usage");
>>>>
>>>> primaryStage.show();
>>>>
>>>> }
>>>>
>>>> privateclassLoadScene extendsTask<Parent> {
>>>>
>>>> @OverrideprotectedParent call() throwsException {
>>>>
>>>> Parent p= FXMLLoader.load( getClass(
>>>> ).getResource("TestView.fxml") );
>>>>
>>>> Thread.sleep( 1000 );
>>>>
>>>> returnp;
>>>>
>>>> }
>>>>
>>>> @Overrideprotectedvoidsucceeded() {
>>>>
>>>> bp.setCenter( getValue() );
>>>>
>>>> }
>>>>
>>>> @Overrideprotectedvoidfailed() {
>>>>
>>>> getException().printStackTrace();
>>>>
>>>> }
>>>>
>>>> }
>>>>
>>>> }
>>>>
>>>> ------------------------------------------------------------------------------------------------------
>>>>
>>>>
>>>> publicclassTestView
>>>>
>>>> {
>>>>
>>>> @FXMLprivateLabel memory;
>>>>
>>>> privatestaticfinaldoubleMEGABYTE= 1024 * 1024;
>>>>
>>>> @FXMLprivatevoidinitialize()
>>>>
>>>> {
>>>>
>>>> varupdater= newTimeline
>>>>
>>>> (
>>>>
>>>> newK eyFrame( Duration.seconds(2.5), event->
>>>>
>>>> {
>>>>
>>>> varruntime= Runtime.getRuntime();
>>>>
>>>> doublemaxMemory= runtime.maxMemory() / MEGABYTE;
>>>>
>>>> doubleusedMemory= (runtime.totalMemory() -
>>>> runtime.freeMemory()) / MEGABYTE;
>>>>
>>>> memory.setText( (int) usedMemory+ " MB / "+ (int)
>>>> maxMemory+" MB");
>>>>
>>>> })
>>>>
>>>> );
>>>>
>>>> updater.setCycleCount(Animation.INDEFINITE); //
>>>> This FXML is being loaded on a background thread
>>>>
>>>> updater.play();
>>>>
>>>> }
>>>>
>>>> }
>>>>
>>>> ------------------------------------------------------------------------------------------------------
>>>>
>>>>
>>>> TestView.fxml
>>>>
>>>> <?xml version="1.0" encoding="UTF-8"?>
>>>>
>>>> <?import javafx.scene.control.Label?>
>>>>
>>>> <?import javafx.scene.layout.StackPane?>
>>>>
>>>> <StackPane xmlns:fx="http://javafx.com/fxml/1
>>>> <https://urldefense.com/v3/__http://javafx.com/fxml/1__;!!ACWV5N9M2RV99hQ!PLtaZAvaobOk-fe_DRSy2AHiPOFUaK683TVXWIvmjnTiUGhmBp3x_GIy_wlMuEV3IHTUMx37HxJHia1cBAoi$>"
>>>> fx:controller="TestView">
>>>>
>>>> <children>
>>>>
>>>> <Label fx:id="memory" text="Current / Max MB" >
>>>>
>>>> <properties hashCode="12345" />
>>>>
>>>> </Label>
>>>>
>>>> </children>
>>>>
>>>> </StackPane>
>>>>
>>>>
>>>>
>>>>
>>>> On Sat, 20 Jan 2024 17:08:41 +0200, Nir Lisker
>>>> <nlisker at gmail.com> wrote:
>>>>
>>>> Hi Jurgen,
>>>>
>>>> What I'm confused about the most is what it is
>>>> you are actually trying to do that
>>>> necessitates the use of animations outside of
>>>> the FX thread. You said that you need to
>>>> initialize controls on another thread, and that
>>>> you are using Task (both of which are fine),
>>>> but how does playing animations relate? Playing
>>>> an animation is something that is done
>>>> explicitly, usually in order to manipulate
>>>> data. Can you give a real use case, like a
>>>> minimized version of what you're doing?
>>>>
>>>> - Nir
>>>>
>>>
>>>
>>>
>>>
>>> --
>>> Using Opera's mail client: http://www.opera.com/mail/
>>> <https://urldefense.com/v3/__http://www.opera.com/mail/__;!!ACWV5N9M2RV99hQ!PLtaZAvaobOk-fe_DRSy2AHiPOFUaK683TVXWIvmjnTiUGhmBp3x_GIy_wlMuEV3IHTUMx37HxJHifb0G69Z$>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/openjfx-dev/attachments/20240124/8fed2f22/attachment-0001.htm>
More information about the openjfx-dev
mailing list