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

[John Hendrikx]

On Thu, 13 Oct 2022 15:52:46 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> John Hendrikx has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains ten commits:
>> 
>>  - Merge remote-tracking branch 'origin/master' into
>>    feature/conditional-bindings
>>    
>>    # Conflicts:
>>    #	modules/javafx.base/src/test/java/test/javafx/beans/value/LazyObjectBindingTest.java
>>  - Fix review comments
>>  - Add missing new line
>>  - Small wording fixes
>>  - Update javadoc of "when" with better phrasing
>>  - Rename showing property to shown as it is already used in subclasses
>>  - Add conditional bindings to ObservableValue
>>  - Change explanatory comment to block comment
>>  - Fix bug where ObjectBinding returns null when revalidated immediately
>>    
>>    Introduced with the fluent bindings PR
>
> modules/javafx.base/src/main/java/com/sun/javafx/binding/ConditionalBinding.java line 19:
> 
>> 17: 
>> 18:         // condition is always observed and never unsubscribed
>> 19:         Subscription.subscribe(nonNullCondition, current -> {
> 
> do I understand this correctly - if the transient Nodes are created and hooked up with ConditionalBinding and then discarded, we'll have a memory leak?

I'm not sure I fully understand what you mean here with the transient nodes, so if you have an example where you think this doesn't work as you'd like, I'd be happy to take a look at it.  However, I'll try explain though what's going on as best I can:

There are two important things at play here:

1) The bindings resulting from `map`, `when`, etc. are lazy bindings because they're created with the fluent system.  This means that if they're not observed themselves, they remove their listeners to their source.  For example, a chain of values (with arrows pointing from referrer to referent):

          A <-- B <-- C <-- D

Or in Java:

          b = a.map(...); c = b.map(...); d = c.map(...);

Here B observes A, C observes B and D observes C.  These will only create listeners on their source when they are observed themselves.  In their unobserved state, D can be GC'd, then C, then B, then A. When the listeners are present, the chain looks like:

         A <-> B <-> C <-> D <----> something is observing D

Now nothing can be GC'd as long as whatever is observing D is strongly referenced. However, as soon as the observer on D disappears, the chain reverts back to single directional pointers because all listeners are removed when unobserved.

2) The condition for `when` acts as an additional requirement for an observable value to create a listener on its source.  If it is `false`, no listener on its source is created, even when observed itself.  If it is `true`, then a listener is only created on its source if it is observed itself, otherwise still no listener is created.

This means when you have:

      ObservableValue<Boolean> active = new SimpleBooleanProperty(true);
 
And then use it as the condition for another property:

      ObservableValue<String> x = label.textProperty().when(active);

      x.addListener(xyz);

Then the `ObservableValue` that results from the call to `when` ("x") will always be subscribed to the `active` value. That means as long as `active` is referred, that `x` cannot be GC'd -- this is what you want, as when I toggle `active` back and forth, the thing we're toggling should not disappear.

In this case, there is a listener AND the condition is `true`, so label's text property is referencing `x` as `x` will have added a listener to label's text property.

         label.text <-> x <-----> listener
         x <-> active

If the condition is `false`, the graph instead looks like:

         label.text <-- x <-----> listener
         x <-> active

This is a very subtle difference, but it does mean that the `label` no longer has any influence on keeping these objects alive.  If only `label` is strongly referenced, then toggling `active` to `false`, and then purposely removing all references to `active` would mean the system can freely remove all these observables, including the listener as there is no way you could turn the switch back on again...

If you however kept a reference to one of these (either `listener`, `x` or `active`) then they all are referenced, as they should be, as I could toggle `active` back to `true` and expect the whole chain to work again as before.

The fact that the lifecycle of the condition and its resulting observable value are tied to each other should not matter, and in fact, has to work that way or switching on/off wouldn't do what you'd expect anymore if the properties being switched could be GC'd.

In an earlier example I showed this:

    ObservableValue<Boolean> active = new SimpleBooleanProperty(true);

    MySkin() {
         getSkinnable().textProperty().when(active).addListener(this::updateSkinStuffs);
         getSkinnable().fontProperty().when(active).addListener(this::updateSkinStuffs);
         getSkinnable().wrapTextProperty().when(active).addListener(this::updateSkinStuffs);
    }

    void dispose() {
         active.set(false);  // kills all listeners/bindings/etc in a single line
    }

Assuming the Skin itself is not strongly referenced at some point, the listeners in the above example are all still referenced by the control resulting from `getSkinnable` and can't be GC'd.  Skinnable refers to its text property, the text property refers to the `when` observable, the `when` observable refers to the `updateSkinStuffs` method, and that method pointer (but also the `when` observable which refers to `active`) keeps the entire Skin and all its listeners alive.

However, if you set `active` to false, that chain is broken.  Specifically, the text property no longer references the `when` observable, because the `when` observable will unsubscribe itself.  Since there are no other references to the skin anymore, the entire skin, including the `active` observable boolean is GC'd.  The fact that `when` and `active` reference each other makes no difference.

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

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