WeakXXListener - when not to use?

[Kevin Rushforth]

I realized I didn't answer the "how to test" part of it. The only thing 
I can think of is running all of the existing tests, including the 
systemTests and any other manual tests that use the object in question, 
with a limited heap size to make GC more likely to happen. There might 
be additional GC flags that one could pass to make the GC more 
aggressive, but I haven't tried that.

-- Kevin


On 3/25/2020 9:25 AM, Kevin Rushforth wrote:
> 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
>>
>