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:
>
> ```java
> 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.
I'm not entirely in agreement here, but also not entirely disagreeing. I mean,
if there was a way to make this work perfectly, then I agree that this
"automatic" unsubscribing is a great mechanism. However, because you are
relying on the GC the code is actually subtly broken right from the start --
you just don't notice it so easily:
1) Listeners which are considered to be "gone" (closed the UI) may still be
working in the background listening to long lived models, which can trigger all
kinds of things. For example, I've seen a UI bound to a long lived model that
would store a "last selected" item when the selection changes. When that UI is
closed and reopened you can sometimes see this storage happening twice, or
three or four times, because the old listeners weren't GC'd yet. Explicit
management is required to avoid such problems.
2) Code that may seem to work fine during a quick testing session may stop
working when exercised a bit longer and a GC occurs. This is quite a common
thing that JavaFX newcomers encounter. They're playing around using the
expression mechanism, code seems to work, but after using a UI for a while, it
suddenly stops updating something. They forgot to make a hard reference
somewhere.
3) The issues I highlighted in my earlier post. Why is there a difference in
how listeners work when it is a property vs an expression? Why does it work
differently when using a binding? Replacing a binding with a listener and vice
versa can have surprising consequences.
The GC issues are some of the hardest to debug, as the problem may appear or
disappear (depending on the type) at any time during debugging if the GC kicks
in. Bug reports from users are going to be even harder to analyze as they're
basically going to be describing something that occurs infrequently and without
any pattern to it, and you can't reproduce it. You'll have to ask things like
the exact GC used, any GC parameters, all things that are normally completely
irrelevant but now may have serious repercussions. Conservative GC's may not
even be suitable to use in combination with JavaFX (when not using explicit
unbinding/unsubscribing). With a regular memory leak I can make a heap dump and
see that a piece of UI has 500 copies in memory. While not fun, finding a GC
root then is often sufficient to find the offending reference and solve the
problem.
> 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.
I don't disagree that is a big source of bugs, however the trade off made
introduces a new category of bugs that is fleeting and an order of magnitude
harder to reproduce and reason about.
> Coming back to my first example, what I would like to see is the following
> code work as expected:
>
> ```java
> 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.
Just to make sure we are on same page, it leaks a bit more memory than it would
if it was using `concat` or a regular property, as the listener stub is leaked
+ the mapping itself. The label is not leaked. The leak "fixes" itself (in both
cases) once caption is updated: the left over listener stub which `label` added
gets removed, which in turn causes an unsubscribe on the mapping, which in turn
unsubscribes itself from `caption`. The mapping is then eligible for GC.
> 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.
I think something might be possible. I personally however find that for
anything but the most trivial FX apps, explicit management (although not for
the case you highlighted above) is preferable to avoid hard to pin down bugs
related to untimely GC's. Explicit management could be far simpler than it
currently is, with a little bit of help from JavaFX.
An `isShowing` property on `Node` in combination with `conditionOn` (since
`while` is unfortunately a keyword) would make binding/listeners between
properties with different lifecycles a lot simpler:
label.textProperty().bind(caption.conditionOn(label::isShowing)); // the
mapping is optional, this already has value
Note that if caption never changes, just binding it regularly will leave stubs
behind with the new and old mechanism. Not a big memory leak, but it may add up
if the binding is in something like a frequently occurring dialog or progress
indicator or something.
-------------
PR: https://git.openjdk.org/jfx/pull/675
