On Wed, 6 Jul 2022 07:07:20 GMT, John Hendrikx <[email protected]> wrote:
>> 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.
@hjohn Thanks for your detailed analysis of the bindings GC situation. The
conclusion I take from all of this is that the current implementation is fine,
but we might consider a follow-up issue to clean up dead listener stubs. If so,
my initial take on that later cleanup issue is that we might use the Disposer
mechanism on a thread other than the JavaFX application thread, but that can be
a topic for later discussion.
@mstr2 What are your thoughts on this?
-------------
PR: https://git.openjdk.org/jfx/pull/675
