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

[John Hendrikx]

On Thu, 12 Jan 2023 12:51:36 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.
>
> @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.

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

@FlorianKirmaier I'm sorry for burdening your PR with this; weak references are something I think make a framework hard to work with and unpredictable, and every time I see more added I cringe. It's the old "now you have two problems" saying:

- We had memory leak problems (not unexpected when you forgot to clean something up)
- So we added weak references.  
- Now we still have memory leak problems, but we've got hard to debug concurrency problems now as well thanks to interactions with GC background threads.

The fix didn't fix it, and introduced a new, far more illusive, category of problems. How anyone can view this as a win is beyond me.

In the spirit of the "current" way JavaFX does things, I guess this PR is fine.  Perhaps it's too late to do anything about it now.  I still had hopes.

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

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