Here are my quick thoughts on the things to watch out for when
converting a (strong) listener to a WeakListener, but I would love to
hear from others.
The main thing you need to do is make sure that the listener doesn't go
out of scope, and thus become subject to garbage collection, too soon.
At a minimum, it means that the object that creates the listener needs
to hold a strong reference to the listener. In the case of the two PRs
referred to above, that minimum bar was cleared (good). By way of
example, blindly turning the following into a WeakReference would lead
to a listener almost certainly being released too early:
Before:
class SomeClass {
...
void someMethod() {
someOtherObject.addListener((o, oldVal, newVal) ->
doSomethingUseful(o, oldVal, newVal));
}
}
After (wrong) :
class SomeClass {
...
void someMethod() {
someOtherObject.addListener(new WeakChangeListener(
(o, oldVal, newVal) -> doSomethingUseful(o, oldVal, newVal)));
}
}
So at a minimum, it would need to do this:
After (better) :
class SomeClass {
...
// hold listener in an instance variable
private ChangeListener myListener = (o, oldVal, newVal) ->
doSomethingUseful(o, oldVal, newVal);
void someMethod() {
someOtherObject.addListener(new
WeakChangeListener(myListener));
}
}
As for whether the above is sufficient, it depends on what the listener
does (what its purpose is).In this simple example, it seems unlikely
that removing the listener when the instance of SomeClass goes out of
scope will cause any problems. It's worth looking at what
"doSomethingUseful" does to see if unregisters anything that ought to be
unregistered (and now maybe won't be if the listener goes away early).
Bringing this back to the two cases in question, I think they are both
fine, although I pointed Ambarish at one thing to look at in the
ButtonSkin case (I don't see anything similar for your ChoiceBox leak
fix that ought to be looked at more carefully).
-- Kevin
On 3/25/2020 3:13 AM, Jeanette Winzenburg wrote:
Currently, there are two open pull requests (#147 and #148) for fixing
memory leaks in controls/skin with the same comment from Kevin:
"Using a WeakListener is certainly easier, but runs the risk of the
listener being removed too early and not cleaning up after itself."
Looks like I can't come up with a scenario where this might happen -
as long as the owner (f.i. a control) of the instance that mananges
the listener (f.i. a selectionModel) on a property of the owner keeps
a strong reference to the manager.
Questions:
- what to watch out for to safely exlude any unwanted self-removal?
- how to test?
-- Jeanette
