WeakXXListener - when not to use?

[Kevin Rushforth]

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
>