RFR: 8290040: Provide simplified deterministic way to manage listeners [v6]

[John Hendrikx]

On Thu, 13 Oct 2022 23:13:25 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>>> Also, using your example above, it might be too easy to create a memory leak by inadvertently referring to a long lived object:
>>> 
>>> ```
>>> getSkinnable().textProperty().when(active).addListener((src,prev,current) -> {
>>>   getSkinnable().setSomething(window.getSomething() + "..."); // window reference makes the listener lambda uncollectable, resulting in a memory leak
>>> });
>>> ```
>> 
>> I'm not sure I follow your meaning.  Why would referring to `window` make the lambda uncollectable? The lambda is referring to `window`, not the other way around.
>> 
>> The current "alternative", which is weak listeners, is not what it is cracked up to be.  A weak listener that is not strongly referenced will be collected at a random time, including never, (almost) immediately or in 5 minutes.  All that time, the listener still functions, responds to potential events and property changes, even though the reason for its existence has since disappeared.
>> 
>> When a weak reference is collected depends on many things, including the chosen GC, its parameters, JDK versions, memory pressure, etc.  This proposal is specifically tailored to make the process of removing listeners deterministic without having to manually unregister listeners.  Weak listeners are completely unpredictable and could potentially live much longer than expected on certain GC's (or future improvements of GC's) that have different strategies for cleaning weak references.
>> 
>> A weak reference specifically is NOT something that gets cleaned up immediately upon the last reference disappearing. This can cause all kinds of phantom effects in JavaFX applications, if your listener does something like persisting state or printing something to the console. For example, a long lived model gets a weak listener attached to it when a dialog opens; the listener persists the last selected item.  The dialog is closed and reopened.  Now there might be two listeners, both persisting the last selected item. This may show up on your console as two database calls where one is expected.
>> 
>> You can argue this is bad programming and that those listeners should have been cleaned up specifically with a dispose method, and I agree, you get predictable behavior that way. And that's exactly what this proposal also brings, predictable behavior, using a syntax that doesn't require you to keep track of your listeners explicitely:
>> 
>>        longLivedModel.indexProperty().when(dialog::shownProperty).addListener(persistPosition);
>> 
>> This detaches the listener immediately after the dialog is closed.  Not in 3 seconds or 5 minutes, but as soon as it is no longer visible.  Opening a new dialog will not reinstate this listener. Reusing the same dialog however would, and it would start working again exactly as you'd expect as soon as the dialog is visible again.
>
> let me try to create a unit test (and I could be wrong, of course).

> In the context of this PR, I can see how a memory leak can be created when a developer has the `active` property as long lived and that would prevent the whole chain from being collected. An example would be trying to attach to the Window.showingProperty(), adding listeners that reference Nodes that are supposed to be GC'd when removed from the said window (dynamically creating detail panes as response to selection event, for example).

What would be the purpose of having a long lived `active` property?  If it has any purpose, then the properties it is controlling must stay referenced, so yes, that would prevent the chain from being GC'd otherwise it could suddenly break your application.

Just like this application breaks currently because of weak listeners:

         label.textProperty().concat("A").addListener((src, old, current) -> System.out.println(current));

The above code will stop working at a random time due to the weak listener created by `concat`.  The same code without `concat` will keep working... this is very hard to explain to users.

Now, if you had used `map`, this wouldn't happen.  This is because `map` uses a normal listener, since it is observed itself.  If the above is what the user wants to do, then who are we to break this when a GC happens?

You also mentioned the `Window.showingProperty`. You have to use the correct path to access this property.  You don't want to make it depend on whether the window is showing.  No, you want to make it depend on whether the label has a scene which is part of a showing window:

       ObservableValue<Boolean> showing = label.flatMap(Node::sceneProperty())
             .flatMap(Scene::windowProperty)
             .orElse(false);

Or short version:

       ObservableValue<Boolean> showing = label.shownProperty();

Now, the listener on your label is added like this:

       label.textProperty().when(showing).addListener( xyz );

Or inlined:

       label.textProperty().when(label::shownProperty).addListener( xyz );

When you remove this label, it's Scene is cleared. `showing` will therefore become `false`, and in turn the listener will stop functioning.

-------------

PR: https://git.openjdk.org/jfx/pull/830