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

[John Hendrikx]

On Wed, 11 Jan 2023 16:41:04 GMT, Michael Strauß <mstrauss 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.
>> 
>> 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.
>
>> 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.

@mstr2 

> The test fails (or doesn't) because it relies on side-effects of a weakly reachable object. 

I know, because JavaFX made it so, other frameworks do not have this problem.  This problem does not happen with other Java code where things are handed off to be done in the background or via call back. Java threads for example don't need to be referenced to keep working, nor do Animations or Timelines, nor do other property based solutions (reactfx) nor do a hundred other examples where a callback is registered via some intermediate object. The intermediate object does not need to be referenced for it to keep working.

What's worse, this is everywhere. My example is just the most banal, but it happens unpredictably in JavaFX with little warning. Let's say I expose something that you might want to observe:

      ObservableValue<String> currentTimeOfDay() {  ...  }

And I add a listener to it:

      x.currentTimeOfDay().addListener((obs, old, current) -> System.out.println("Time now is: " + current));

You can say exactly **nothing** about the functioning of this code when working with JavaFX.  You **must** look at the implementation of `currentTimeOfDay` to infer anything about this code.  It may work forever, or it may break randomly.  To make it work for sure, you would need to store the reference you got from `currentTimeOfDay()`.  This is because it depends on the runtime type of what `currentTimeOfDay` returns -- the promises made by the `ObservableValue` interface are taken very lightly by JavaFX.

For example, does it return a property (`instanceof StringProperty`), like `SimpleStringProperty`, which must be a direct field somewhere so it can be updated?  It will keep working fine.  Does it perhaps build the time of day for you with a few `concat`s using separate fields?

     return hours.concat(":").concat(minutes).concat(":").concat(seconds);

Or perhaps we refactored it as such later?  Now the caller code breaks.  Or perhaps some of the callers break, but others don't.  Some of the callers may be in another module.  Perhaps used by another team.

In isolation this may all seem reasonable (certainly not trivial) and you should "keep in mind that JavaFX uses weak references", but in larger applications, there are more and more abstractions, properties are not always what they seem and may change.  There's nothing worse than bugs that will pass most unit tests and fail randomly in production (on user machines) when GC has time to run.  

We've traded potential memory leaks for concurrency issues.  Memory leaks which can be relatively easily and deterministically analyzed with heap dumps VS higher level application logic breaking randomly defying easy analysis, similar to concurrency issues (I've had to run JavaFX applications with `System.gc()` being called every second to find such bugs).  Such bugs can be so hard to debug for even experienced developers that they give up and blame bugs in the framework for the random behavior -- they may give up on JavaFX entirely after such an experience and see it as something to avoid for their next project.

> I don't think that this is a JavaFX-specific issue, it's a general issue with garbage-collected environments. 

I'm not sure what you're getting at, GC'd environments have nothing to do with this. GC is transparent, does not affect normal operations of code and doesn't generally act as a concurrent thread that can change the state of your objects randomly. It's weak reference use that is the culprit... most GC'd languages don't even support weak references. The test even proofs the problem is weak references, it worked before this change introduced additional weak references.

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

We will have to disagree here. The surprising part is definitely that this magically stops working (and unpredictably at that). How you could possibly think that "keeps working" vs "stops randomly due to background thread interaction with GC" is the more surprising outcome here I find hard to follow.

I tell the system:

- Wrap this list in a property
- Add a listener to the property to monitor the list
- Listener prints changes

In JavaFX, that can break if I didn't:

- Hold a strong reference to the property that wraps the list (because multiple other threads that I didn't create and run in the background and which normally never visibly interfere with any Java code may decide to "undo" the work I just did)

It's about the same as:

- I create a background function
- I wrap this in a thread
- I start the thread

It would be rather annoying if that stopped working randomly if I forgot to keep a strong reference to the thread.

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

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