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

[Michael Strauß]

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

> 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 test fails (or doesn't) because it relies on side-effects of a weakly reachable object. I don't think that this is a JavaFX-specific issue, it's a general issue with garbage-collected environments. More to the point: I think it's more surprising to find out that this test would actually consistently pass. Java developers know that they can't rely on weakly-reachable objects, so why would they suddenly expect such an object to have well-defined side effects?

> 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.

I don't think this is how ´ListProperty` should work.

Fundamentally, we're talking about this scenario:

ObservableList<Object> list = FXCollections.observableArrayList();
ListProperty<Object> listProperty = new SimpleListProperty<>(list);


Under no circumstance should `list` ever hold a strong reference to `listProperty`. Your proposal would make a strong reference contingent upon registering listeners on `listProperty`.

But that's not relevant: why should `listProperty` be kept alive if it is only weakly reachable from its subscribed listeners (or anyone else, for that matter)? As a user, that's not what I would expect if I only added listeners to `listProperty` (note: I didn't add any listeners to `list` itself). `ObjectProperty` doesn't behave like this; instead, `ObjectProperty` can become weakly reachable even if the contained object doesn't.

The purpose of this PR is to break the strong reference from `list` to `listProperty`, and I think that's the right thing to do.

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

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