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.
Thanks @hjohn for the detailed explanation. However, I'm not convinced that
this is the right model. Many applications will use a layered architecture
where a short-lived property is bound to a long-lived property:
var label = new Label(); // scoped to the lifetime of a dialog window
var caption = model.captionProperty(); // scoped to the lifetime of the
application
label.textProperty().bind(caption);
This works fine, and doesn't require users to think about unbinding the
property before it goes out of scope. I would argue that this is a fundamental
feature of the JavaFX property system which makes it easy to reason about code.
You give the following example as the "odd one out":
// #4 Baseline JavaFX listener on concatted result:
JMemoryBuddy.memoryTest(test -> {
StringProperty labelText = new SimpleStringProperty(this, "labelText");
StringProperty model = new SimpleStringProperty(this, "model");
ObservableValue<String> expression = model.concat("%");
expression.addListener((obs, old, current) -> labelText.set(current));
test.setAsReferenced(labelText);
test.setAsReferenced(model);
test.assertCollectable(expression); // !!!! -- listener stops working,
expression was collected!
});
I don't agree that there is anything wrong with this code, other than the fact
that a listener was added to a transient value (`expression`). In fact, I would
argue that this is a good model: the lifetime of a property is determined by
its reachability from user code. If it's not reachable, then it's eligible for
collection.
With this model in mind, it's also immediately clear why the code works then
using `bind` instead of `addListener`:
// #3 Baseline JavaFX bind on concatted result:
JMemoryBuddy.memoryTest(test -> {
StringProperty labelText = new SimpleStringProperty(this, "labelText");
StringProperty model = new SimpleStringProperty(this, "model");
ObservableValue<String> expression = model.concat("%");
labelText.bind(expression);
test.setAsReferenced(labelText);
test.setAsReferenced(model);
test.assertNotCollectable(expression); // expression is being used, please
don't collect
});
Now, `expression` is not a transient value because it is logically reachable
from `labelText`.
Any alternative to this model involves some form of explicit lifetime
management (`conditionOn`, `Subscription`, etc). However, as we can see with
listeners (that need to be manually added and removed), explicit lifetime
management is one of the most abundant sources of bugs.
Coming back to my first example, what I would like to see is the following code
work as expected:
var label = new Label(); // scoped to the lifetime of a dialog window
var caption = model.captionProperty(); // scoped to the lifetime of the
application
// I shouldn't have to worry about this code:
label.textProperty().bind(caption.map(String::toUpperCase));
However, not only does this code leak memory, it also makes it far too easy to
leak memory. The API looks and feels natural and easy to use, but hides a bug
that will not be visible to most users. I would argue that the potential for
bugs is on the same order of magnitude as with explicitly added listeners, but
much more subtle as it's hidden in a nice and fluent API.
Here's my conclusion: I think the "reachability" model is superior to manual
and explicit lifetime management, and I think we should make it work before
integrating this PR. As currently proposed, the new `map` and `flatMap` APIs
have the potential to cause lots of bugs, or force users to use weird
constructs to prevent memory leaks.
-------------
PR: https://git.openjdk.org/jfx/pull/675
