You are right: the weak references are not suitable in this case. These cases are clearly bugs in the skins, we should log them (unless already logged); I don’t think we need new APIs.
See https://bugs.openjdk.org/issues/?jql=text%20~%20%22skin%20cleanup%22%20AND%20project%20%3D%20JDK%20AND%20component%20%3D%20javafx%20AND%20resolution%20%3D%20Unresolved%20ORDER%20BY%20updated%20%20DESC There is an umbrella task https://bugs.openjdk.org/browse/JDK-8241364 which collects all these issues. Care to create JBS tickets? Thanks! -andy From: John Hendrikx <john.hendr...@gmail.com> Date: Monday, December 18, 2023 at 16:14 To: Andy Goryachev <andy.goryac...@oracle.com>, openjfx-dev <openjfx-dev@openjdk.org> Subject: [External] : Re: New API: Animation/Timeline improvement I don't think that's the correct solution, as it opens up a whole new avenue for subtle bugs, bringing GC/JVM/OS whims and settings into the mix. We want the clean-up to be easier to reason about, not harder. Even if it were a good idea, it's likely also going to be a breaking change to add weak references at this stage, without some kind of opt-in (which would then require new API, in which case we might as well go for a solution that has no need of weak references). I feel I have to keep repeating this, but there almost zero guarantees made by the JVM or Java with regards to references; they are fine for internal processes carefully designed to have no user visible side effects, but burdening the user with side effects (delayed clean-up, or early unexpected clean-up due to lack of a hard reference) without the user actually choosing to use a reference type themselves is not a good design. --John On 18/12/2023 17:18, Andy Goryachev wrote: Would making Timeline to use WeakReferences solve the issue without the need for a new API? -andy From: openjfx-dev <openjfx-dev-r...@openjdk.org><mailto:openjfx-dev-r...@openjdk.org> on behalf of John Hendrikx <john.hendr...@gmail.com><mailto:john.hendr...@gmail.com> Date: Friday, December 15, 2023 at 21:53 To: openjfx-dev <openjfx-dev@openjdk.org><mailto:openjfx-dev@openjdk.org> Subject: New API: Animation/Timeline improvement Hi list, I've noticed that Animations and Timelines are often a source of leaks, and their clean-up is often either non-existent or incorrect. The reason they cause leaks easily is because a running animation or timeline is globally referred from a singleton PrimaryTimer. The animation or timeline then refers to properties or event handlers which refer to controls (which refer to parents and the entire scene). For example: - ScrollBarBehavior uses a Timeline, but neglects to clean it up. If it was running at the time a Scene is detached from a Window, and that Scene is left to go out of scope, it won't because Timeline refers it; this can happen if the behavior never receives a key released event. - ScrollBarBehavior has no dispose method overridden, so swapping Skins while the animation is running will leave a Timeline active (it uses Animation.INDEFINITE) - SpinnerBehavior has flawed clean up; it attaches a Scene listener and disables its timeline when the scene changed, but the scene doesn't have to change for it to go out of scope as a whole... Result is that if you have a spinner timeline running, and you close the entire window (no Scene change happens), the entire Scene will still be referred. It also uses an indefinite cycle count. It also lacks a dispose method, so swapping Skins at a bad moment can also leave a reference. I think these mistakes are common, and far too easy to make. The most common use cases for animations revolve around modifying properties on visible controls, and although animations can be used for purposes other than animating UI controls, this is extremely rare. So it is safe to say that in 99% of cases you want the animation to stop once a some Node is no longer showing. For both the mentioned buggy behaviors above, this would be perfect. A spinner stops spinning when no longer showing, and a scroll bar stops scrolling when no longer showing. It is also likely to apply for many other uses of timelines and animations. I therefore want to propose a new API, either on Node or Animation (or both): /** * Creates a new timeline which is stopped automatically when this Node * is no longer showing. Stopping timelines is essential as they may refer * nodes even after they are no longer used anywhere, preventing them from * being garbage collected. */ Node.createTimeline(); // and variants with the various Timeline constructors And/or: /** * Links this Animation to the given Node, and stops the animation * automatically when the Node is no longer showing. Stopping animations * is essential as they may refer nodes even after they are no longer used * anywhere, preventing them from being garbage collected. */ void stopWhenHidden(Node node); The above API for Animation could also be provided through another constructor, which takes a Node which will it be linked to. Alternatives: - Be a lot more diligent about cleaning up animations and timelines (essentially maintain the status quo which has led to above bugs) - Use this lengthy code fragment below: Timeline timeline = new Timeline(); someNode.sceneProperty() .when(timeline.statusProperty().map(status -> status != Status.STOPPED)) .flatMap(Scene::windowProperty) .flatMap(Window::showingProperty) .orElse(false) .subscribe(showing -> { if (!showing) timeline.stop(); }); The `when` line ensures that the opposite problem (Nodes forever referencing Timelines) doesn't occur if you are creating a new Timeline for each use (not recommended, but nonetheless a common occurrence). --John
