RFR: 8277848 Binding and Unbinding to List leads to memory leak [v7]

[Florian Kirmaier]

On Wed, 11 Jan 2023 15:54:25 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

>>> Should I expect x here to be unreferenced? No, that's how properties work, b will keep its reference to x.
>> 
>> No, x is not dereferenced. But you should expect that a is no longer referencing b, but both a and b are referencing x.
>> 
>> But I don't see your point - it's worth mentioning, when you bind a ListProperty A, to another ListProperty B, then A doesn't get the value "B", but the value will be the value of the property B.
>> 
>> If you think the PR creates an unwanted behavior, I would appreciate seeing a **unit-test** for it, because it's way easier to discuss on the basis of a test.
>> 
>> Making tests for memory behavior (both for being collectible and being not-collectible) is easy with JMemoryBuddy, which I wrote exactly for this use case.
>> 
>> I'm also happy with an alternative solution - which makes the tests green, which I've provided. I'm not really opinionated.
>> 
>>> 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. 
>> 
>> Yes, I agree. I think - it's a complicated topic. I don't want to discuss any details, because I just want to add a fundamental bug, without discussing the whole Property/Collection design.
>
>> > Should I expect x here to be unreferenced? No, that's how properties work, b will keep its reference to x.
>> 
>> No, x is not dereferenced. But you should expect that a is no longer referencing b, but both a and b are referencing x.
> 
> I missed that the test was referencing only the properties, although I suppose the "list like behavior" people might find it surprising to see the previously bound property now suddenly referencing a different `ObservableList`. If that `ObservableList` is supposed to be short lived, then that might be a new kind of memory leak (but nothing that wasn't there before).
> 
> The above written as a test, people may find this surprising behavior:
> 
>     @Test
>     public void testListsDontCrossPolinate() {
>         ObservableList<Object> a = FXCollections.observableArrayList("A");
>         ObservableList<Object> b = FXCollections.observableArrayList("A", "B", "C");
> 
>         SimpleListProperty<Object> propA = new SimpleListProperty<>(a);
>         SimpleListProperty<Object> propB = new SimpleListProperty<>(b);
> 
>         propA.bind(propB);
> 
>         assertEquals(3, propA.get().size());  // Note: if you remove this line, the old code fails badly...
> 
>         propA.unbind();
> 
>         propA.get().add("A");
> 
>         assertEquals(4, propA.get().size());
>         assertEquals(4, propB.get().size());
>     }
> 
>> If you think the PR creates an unwanted behavior, I would appreciate seeing a **unit-test** for it, because it's way easier to discuss on the basis of a test.
> 
> Here is one that passed with the old version, which highlights the standard gotcha's of all uses of weak references:
> 
>     @Test
>     public void testWeakRefDoesntDisappearUnexpectedly() {
>         List<Object> changes = new ArrayList<>();
> 
>         ObservableList<Object> observableList = FXCollections.observableArrayList();
> 
>         SimpleListProperty<Object> simpleListProperty = new SimpleListProperty<>(observableList);
>         simpleListProperty.addListener((obs, old, current) -> changes.add(current));
> 
>         simpleListProperty = null;
> 
>         observableList.add("A");
> 
>         assertEquals(1, changes.size());
> 
>         System.gc();
> 
>         observableList.add("B");
> 
>         assertEquals(2, changes.size());
>     }
> 
> This test highlights the following issues:
> - Weak references don't disappear instantly; they may disappear at any time or never, at the digression of your JVM; the first change we do to `observableList` may be picked up or not, it's unpredictable.  In real applications, this exhibits itself as "works on my machine" or "seems to work for a while, but then breaks".
> - The second change doesn't get picked up, but it still might.  It depends on the mood of the garbage collector.  This can show up in real applications as phantom changes being triggered by objects that are supposedly no longer in use.
> 
> And finally, why would we expect that the outcome is anything but what the test says?  I registered a listener, I didn't unregister it, it's there, it's registering changes made to an observable list and putting those in another list for later use.  Why would this test fail at all?  Explaining what's happening here to JavaFX novices is impossible. We're relying here on a fundamental JVM system, that is largely unspecified in how it behaves exactly (and with good reason), for a very basic but crucial feature.
> 
> The way I see it, `ListProperty` should work as follows:
> 
> 1) When created, it registers nothing to anything; this will allow it to go out of scope (currently it can't, as it registers on `ObservableList` which will point back to it).
> 
> 2) When any listener is registered to it (or to its `empty` or `size` properties), it registers on the underlying `ObservableList`; when all listeners are gone, it unregisters from the underlying `ObservableList`.  This is what you would want; if I'm listening to something, I don't want this to disappear for any reason.
> 
> 3) `bind` / `unbind` can stay as they are; when you bind to another property, that other property gets a listener and registers with its underlying `ObservableList`.  When `unbind` gets called, that listener is removed again (and potentially, the other property unregisters from its underlying `ObservableList` if that was the only listener).  Before this would break, as the `ObservableList` would still be pointing to the other list property (as a listener is present) and the now unbound property, which also refers that `ObservableList` now (whether that's good or bad) will then keep an indirect reference to that other property (previously bound property -> observable list -> other property).
> 
> No weak listeners are needed anywhere now, and everything will be nice and predictable.
> 
> I'll attempt to make a PoC for this.

@hjohn 
I guess it would be possible to make "bind" use strong references. But then we must do the same for the "normal properties". And doing that would probably break half of JavaFX and many many projects. So for consistency, the bind in ListProperty has to be weak. And honestly, I prefer it to be weak - I guess it's a matter of opinion. But many things in JavaFX wouldn't work if bind wouldn't weak by default.

Since I'm writing unit tests for the "memory behavior" with my library JMemoryBuddy, getting WeakReferences correct is now also much easier! (It can write both tests to check whether something is collectible, or whether it's not collectible.) 




I've just merged the PR with master, so it's now possible again to merge it!

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

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