On Fri, 19 Aug 2022 10:40:01 GMT, Florian Kirmaier <[email protected]>
wrote:
>> Making the initial listener of the ListProperty weak fixes the problem.
>> The same is fixed for Set and Map.
>> Due to a smart implementation, this is done without any performance drawback.
>> (The trick is to have an object, which is both the WeakReference and the
>> Changelistener)
>> By implying the same trick to the InvalidationListener, this should even
>> improve the performance of the collection properties.
>
> Florian Kirmaier 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 seven additional
> commits since the last revision:
>
> - Merge remote-tracking branch 'origjfx/master' into
> JDK-8277848-list-binding-leak
> - JDK-8277848
> Added tests to ensure no premature garbage collection is happening.
> - JDK-8277848
> Added 3 more tests to verify that a bug discussed in the PR does not
> appear.
> - JDK-8277848
> Added the 3 requests whitespaces
> - JDK-8277848
> Further optimization based code review.
> This Bugfix should now event improve the performance
> - JDK-8277848
> Added missing change
> - JDK-8277848
> Fixed memoryleak, when binding and unbinding a ListProperty. The same was
> fixed for SetProperty and MapProperty.
My question is the following. Isn't this expected behavior?
Let's say I have this program:
X x = new X();
ObjectProperty<X> a = new SimpleObjectProperty<>(x);
ObjectProperty<X> b = new SimpleObjectProperty<>();
b.bind(a); // b takes on the value of a (x)
b.unbind(); // b keeps last value (x)
a = null;
x = null;
Should I expect `x` here to be unreferenced? No, that's how properties work,
`b` will keep its reference to `x`.
Now replace the above program with `X` being an `ObservableList` and the
`ObjectProperty`s being `ListProperty`s. Should the expected behavior change?
I think there is a far more fundamental issue with how `ListProperty`s behave
that's poorly defined and documented. While investigating, I found the main
culprit to the `bind`/`unbind` test (`testBindingLeak`) to be the copy that is
made in the `unbind` method:
@Override
public void unbind() {
if (observable != null) {
value = observable.getValue(); // <<< cause of "memory leak"
observable.removeListener(listener);
observable = null;
}
}
That line makes total sense from a property perspective; when a property is
unbound, it copies the value of its source as the last known value. Should
this perhaps make a deep copy? Perhaps it should, but these properties are so
poorly defined that it is anyone's guess at what it should be doing here.
IMHO there is a conflict between the property like behavior of `ListProperty`
and the list like behavior of the `ObservableList` that it wraps. On the one
hand, a list property is a reference to a list, and on the other hand we expect
it to behave as only referring to the contents of the list. The property like
behavior that it inherits (perhaps it shouldn't have done that) has been
subverted in that adding a change listener will pretend that a change to the
list is the same as replacing the entire list object, so it is not quite a
property any more. You can see this in the fact that the "old value" of a
change listener is incorrect:
ObservableList<Object> list = FXCollections.observableArrayList();
SimpleListProperty<Object> simpleListProperty = new
SimpleListProperty<>(list);
simpleListProperty.addListener((obs, old, current) ->
System.out.println("Changed: " + old + " -> " + current));
list.add("B");
simpleListProperty.set(FXCollections.observableArrayList("C"));
Prints:
Changed: [B] -> [B] // huh? so nothing changed then? but it did...
Changed: [B] -> [C]
This is frustrating as change listener has very specific semantics that code
would like to rely on, but here, again, the old value is just a best effort
guess (there are other bugs in this area).
I think that because of these poorly defined semantics, one will keep finding
surprises in the behavior of these properties, depending on your perspective
(property or list). I think therefore that the first thing that's need to be
done is to clearly define how these observable wrapper properties are supposed
to work before fixing what may or may not be bugs in this area. I fear it may
not be possible to have `ListProperty` behave property and list like at the
same time, and if so this should be clearly marked in the documentation that
`ListProperty` does not obey the general contract of `Property`.
-------------
PR: https://git.openjdk.org/jfx/pull/689
