RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v18]

[John Hendrikx]

On Tue, 5 Jul 2022 17:14:41 GMT, Michael Strauß <mstrauss at openjdk.org> wrote:

>> John Hendrikx has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 27 additional commits since the last revision:
>> 
>>  - Merge branch 'openjdk:master' into feature/fluent-bindings
>>  - Add null checks in Subscription
>>  - Update copyrights
>>  - Move private binding classes to com.sun.javafx.binding package
>>  - Add note to Bindings#select to consider ObservableValue#flatMap
>>  - Fix bug invalidation bug in FlatMappedBinding
>>    
>>    Also fixed a secondary issue where the indirect source of the binding
>>    was unsubscribed and resubscribed each time its value was recomputed.
>>    
>>    Add additional comments to clarify how FlatMappedBinding works.
>>    
>>    Added test cases for these newly discovered issues.
>>  - Fix typos in LazyObjectBinding
>>  - Rename observeInputs to observeSources
>>  - Expand flatMap javadoc with additional wording from Optional#flatMap
>>  - Add since tags to all new API
>>  - ... and 17 more: https://git.openjdk.org/jfx/compare/7472f0a3...d66f2ba6
>
> I have yet another question. The following test passes for `Bindings.select`, but fails for `ObservableValue.flatMap`:
> 
> 
> JMemoryBuddy.memoryTest(test -> {
>     class ValueHolder {
>         final StringProperty value = new SimpleStringProperty(this, "value");
>         StringProperty valueProperty() { return value; }
>     }
> 
>     ObjectProperty<ValueHolder> valueHolderProperty = new SimpleObjectProperty<>();
>     valueHolderProperty.set(new ValueHolder());
> 
>     // Map the nested property value
>     ObservableValue<String> mapped = valueHolderProperty.flatMap(ValueHolder::valueProperty);
>     
>     // Note: the test passes when using the following alternative to flatMap:
>     // ObservableValue<String> mapped = Bindings.selectString(valueHolderProperty, "value");
> 
>     // Bind the mapped value to a property that will soon be GC'ed.
>     ObjectProperty<String> otherProperty = new SimpleObjectProperty<>();
>     otherProperty.bind(mapped);
> 
>     test.setAsReferenced(valueHolderProperty);
>     test.assertCollectable(otherProperty);
>     test.assertCollectable(mapped); // expectation: the mapped value is eligible for GC
> });
> 
> 
> My observation is that a flat-mapped value that was once observed is not eligible for garbage-collection even when the observer itself is collected. This seems to be quite unexpected to me, because it means that a bound property that is collected without being manually unbound will cause a memory leak in the mapped binding.
> 
> Is this by design? If so, I think this can lead to subtle and hard to diagnose bugs, and should be documented at the very least.

Some more about the GC problem discovered by @mstr2 

### How to deal with this when using Fluent bindings (`conditionOn`)

In the initial proposal, there was a `conditionOn` mechanism and `Subscription` mechanism.  `conditionOn` can be used to make your bindings conditional on some external factor to ensure they are automatically cleaned up. The Fluent bindings only require their final result to be unsubscribed, as all intermediate steps will unsubscribe themselves from their source as soon as they themselves become unobserved:

> a listens to b, listens to c

If `a` becomes unobserved, it unsubscribes itself from `b`, which unsubscribes itself from `c`. `c` is now eligible for GC. With standard JavaFX listeners, such a chain must be unsubscribed at each step making it almost impossible to use in practice.

Using `conditionOn` the chain of mappings can be automatically unsubscribed:

      ObservableValue<Boolean> condition = ... ;

      longLivedProperty.conditionOn(condition)
           .map(x -> x + "%")
           .addListener((obs, old, current) -> ... );

The condition can be anything, like a `Skinnable` reference becoming `null`, a piece of UI becoming invisible, etc.

Note that even though `conditionOn` is currently not available as a nice short-cut, you can still do this with the current implementation:

      ObservableValue<Boolean> condition = ... ;

      condition.flatMap(c -> c ? longLivedProperty : null)
           .map(x -> x + "%")
           .addListener((obs, old, current) -> ... );

`longLivedProperty` will be unsubscribed as soon as `condition` becomes false.

### How to deal with this when using Fluent bindings (`Subscription`)

Although `conditionOn` is IMHO by far the preferred mechanism to handle clean-up, `Subscription` also could be very useful. It is less awkward to use than `addListener` / `removeListener` because the `Subscription` is returned:

     ChangeListener<ObservableValue<String>, String, String> listener = (obs, old, current) -> ... ;
     x.addListener(listener);
     x.removeListener(listener);

vs:

     Subscription s = x.subscribe((obs, old, current) -> ... );
     s.unsubscribe();

Subscriptions can also be combined:

     Subscription s = x.subscribe((obs, old, current) -> ... )
         .and(y.subscribe( ... ))
         .and(z.subscribe( ... ));
    
     s.unsubscribe();   // releases listeners on x, y and z

### Dealing with "stub" memory leak in current JavaFX

Relying on `invalidated` or `changed` being called to clean up dead listeners is perhaps not ideal. It may be an idea to start using a `ReferenceQueue` where all such stubs are registered when they get GC'd. As JavaFX is already continuously running its UI and render threads, it is no great leap to check this `ReferenceQueue` each tick and pro-actively clean up these stubs.  Alternatively, a single daemon thread could be used specifically for this purpose.  The FX thread would be more suitable however as listener clean-up must be done on that thread anyway.

This would solve the issue found by @mstr2 in any normal JavaFX application (one where the FX thread is running), and would also solve the issue I highlighted with stubs not being cleaned up in my test program.

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

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