jfx 8277848: Binding and Unbinding to List leads to memory leak

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.

Progress

[x] Change must not contain extraneous whitespace
[x] Commit message must refer to an issue
[ ] Change must be properly reviewed (2 reviews required, with at least 2 Reviewers)

Issue

JDK-8277848: Binding and Unbinding to List leads to memory leak (Bug - P3)

Reviewers

Michael Strauß (@mstr2 - Committer) 🔄 Re-review required (review applies to d9fccd22)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/689/head:pull/689
$ git checkout pull/689

Update a local copy of the PR:
$ git checkout pull/689
$ git pull https://git.openjdk.org/jfx.git pull/689/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 689

View PR using the GUI difftool:
$ git pr show -t 689

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/689.diff

Webrev

Link to Webrev Comment

Dec 07 '21 14:12 FlorianKirmaier

:wave: Welcome back fkirmaier! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

Dec 07 '21 14:12 bridgekeeper[bot]

Webrevs

Dec 07 '21 15:12 mlbridge[bot]

/reviewers 2

Dec 07 '21 15:12 kevinrushforth

@kevinrushforth The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

Dec 07 '21 15:12 openjdk[bot]

Why are the new listener imlementations called BaseChangeListener and BaseInvalidationListener, i.e. why the Base?

Also, if you're going to the trouble of refactoring the existing listener implementation, have you considered merging the very similar implementations into a single class? You can then re-use the listener instance and save another object allocation in this way:

private static class Listener<E> extends WeakReference<ListPropertyBase<E>>
        implements InvalidationListener, ListChangeListener<E>, WeakListener {
    Listener(ListPropertyBase<E> ref) {
        super(ref);
    }

    @Override
    public boolean wasGarbageCollected() {
        return get() == null;
    }

    @Override
    public void onChanged(Change<? extends E> change) {
        ListPropertyBase<E> ref = get();
        if(ref != null) {
            ref.invalidateProperties();
            ref.invalidated();
            ref.fireValueChangedEvent(change);
        }
    }

    @Override
    public void invalidated(Observable observable) {
        ListPropertyBase<E> ref = get();
        if (ref == null) {
            observable.removeListener(this);
        } else {
            ref.markInvalid(ref.value);
        }
    }
}

Dec 31 '21 12:12 mstr2

@mstr2 Great point. The chosen name was just because I needed a name. I switched now to the name "Listener".

I've now merged the ChangeListener with the Invalidation Listener, as you've suggested. The PR should now improve the performance while fixing a bug. It might even be quite relevant because these classes are used very often.

Jan 05 '22 18:01 FlorianKirmaier

I've added the 3 requested whitespaces! It sill would be great if CI could catch these minor problems.

Jan 11 '22 19:01 FlorianKirmaier

How to proceed to get this PR reviewed?

Jun 09 '22 07:06 christianheilmann

Fundamentally, the problem does not arise from bindings, but from the fact that ListPropertyBase adds a ListChangeListener to its wrapped ObservableList.

Since the added ListChangeListener is a capturing lambda, the ListPropertyBase is now tied to the lifetime of the wrapped ObservableList. This failing test illustrates the problem:

JMemoryBuddy.memoryTest(checker -> {
    ObservableList<Object> list = FXCollections.observableArrayList();
    ListProperty<Object> listProperty = new SimpleListProperty<>(list);

    checker.setAsReferenced(list);
    checker.assertCollectable(listProperty); // --> AssertionError: listProperty not collected
});

This behavior may or may not be a bug. I'm inclined to think that it is, because I would be astonished to learn that an object contained in a Property<T> would hold a strong reference to the property instance itself.

Jun 22 '22 04:06 mstr2

Fundamentally, the problem does not arise from bindings, but from the fact that ListPropertyBase adds a ListChangeListener to its wrapped ObservableList.

Yes, I would agree. The cause is the eager setup of listeners on other properties, resulting in the target being tied to the lifecycle of the observed property. The listener itself serves no purpose until the ListProperty itself is observed in some fashion. A ListProperty could be processing thousands of "changes" to update its internal state, but if it is not observed or called directly, nobody cares. The end result is that the property cannot be garbage collected and is processing every change event, even though everything has lost interest in it.

This is basically another case where it is better to only listen (or bind) to other targets when absolutely necessary (when the source itself is being observed) and to remove the listener (or binding) when no longer observed, basically lazy versus eager observation. Lazy observation would completely remove the need for using weak references, simplifying the logic and making the whole system much easier to reason about.

Jun 22 '22 10:06 hjohn

This is basically another case where it is better to only listen (or bind) to other targets when absolutely necessary (when the source itself is being observed) and to remove the listener (or binding) when no longer observed, basically lazy versus eager observation. Lazy observation would completely remove the need for using weak references, simplifying the logic and making the whole system much easier to reason about.

While I agree that lazy observation would be a good improvement, it just shifts the problem one indirection away. You could still have an entire object graph that is needlessly kept alive by the property wrapper, which is a problem.

Jun 22 '22 12:06 mstr2

Discussed Property Leak @mstr2 I've added your test to the PR and it's green. I think you mixed something up when testing this. I've added the same test for the other property types.

Lazyness I don't know whether it's a good idea to add laziness to an existing API. But it's not the scope of my PR. I only try to fix a fundamental bug. Honestly, I'm not a big fan of laziness, because of the complexity, it tends to introduce.

Jul 11 '22 08:07 FlorianKirmaier

This will require careful review and testing to make sure that we don't introduce any cases where the binding disappears too soon. The main thing we need to be careful about is that we don't take any cases that work reliably today (albeit at the cost of a memory leak) and make them subject to premature garbage collection. I also agree that it seems out of scope to introduce laziness into this existing API. Changing the implementation to use lazy bindings under-the-covers would be acceptable, but only if it caused no app-visible change in behavior.

Jul 12 '22 14:07 kevinrushforth

@kevinrushforth Yes, premature collection is quite a problem with weak binding, but luckily this can also be checked with unit tests. So I've added a unit test to verify, that the binding doesn't get collected prematurely.

What we can't test, is whether some applications (or the internal JavaFX code) rely on the memory leak itself. But I guess that is how one can argue with any bug.

Jul 14 '22 10:07 FlorianKirmaier

I've just merged it with master for easier codereview.

Aug 19 '22 10:08 FlorianKirmaier

@FlorianKirmaier this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-8277848-list-binding-leak
git fetch https://git.openjdk.org/jfx master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

Jan 10 '23 21:01 openjdk[bot]

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 ObjectPropertys being ListPropertys. Should the expected behavior change?

I think there is a far more fundamental issue with how ListPropertys 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.

Jan 11 '23 11:01 hjohn

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.

Jan 11 '23 13:01 FlorianKirmaier

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:

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

Jan 11 '23 15:01 hjohn

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:

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

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.

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.

Jan 11 '23 16:01 mstr2

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

Jan 12 '23 09:01 FlorianKirmaier

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

Jan 12 '23 12:01 hjohn

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

Jan 12 '23 13:01 hjohn

To my knowledge, many memory leaks, are not really possible to implement elegantly without weak references. For that reason, I usually come from the opposite direction, and ask myself, how can I test code with WeakReferences properly.

Jan 13 '23 09:01 FlorianKirmaier

@FlorianKirmaier

To my knowledge, many memory leaks, are not really possible to implement elegantly without weak references. For that reason, I usually come from the opposite direction, and ask myself, how can I test code with WeakReferences properly.

They aren't leaks when you asked for them to be created. Unexpected memory leaks are the real problem, and JavaFX is the culprit because it does lots of unexpected things:

It stems from the standard classes JavaFX gives us. A ListProperty registers eagerly upon construction with an ObservableList. If I create a 100 of them in a loop, none of them disappear (assuming no weak listeners are used). Why does it do this when those properties are unused? The problem of memory leaks is not so much that they're real memory leaks, the problem is that the user doesn't expect there to be leaks from just the construction of a component, because that's not how Java works (even new Thread requires start before the thread "leaks"):

 new ListProperty<>(observableList);   // -> memory leak, eager listener registration occurred

The above causing a leak is indeed unexpected. But now take this:

 new ListProperty<>(observableList).emptyProperty().addListener( ... );

This is not a leak. The listener was added by the user, and it should keep working until the user removes it. The user expects this to work, and can reasonable expect there to be some memory allocated to keep it working, until such time the user undoes this action.

Now, the first example can be fixed in two ways:

Use weak listeners.
Don't register listeners until the user actually needs them

Option 1 breaks the second example and leads to new problems; those new problems are even harder to debug than a simple memory leak, as all the elaborate tests all over the JavaFX code base that rely on System.gc() clearly show. It doesn't solve the problem fully either, so now we have two problems: Unexpected memory leaks are still a problem (as JavaFX keeps doing things that are "unexpected" from a user perspective), and we now have to deal with unexpected memory reclamation because some vague rule was broken that you must keep a reference to some things, but not others (see other post that you can't rely on this at all when dealing with API's that provide properties).

Option 2 leads to several benefits; no listeners being registered until needed means less memory use, less "events" being send between components unnecessarily and the main benefit, no memory leaks that the user didn't specifically ask for. This is how a major problem was fixed in Node where listeners were registered on Scene and Window for each and every node in a scene. Those listeners weren't necessary as they weren't actually used by the user, yet they were eagerly registered causing tens of thousands of listeners (on a single property) for large scenes.

Use of weak listeners are a cure worse than the disease, because the second case now breaks randomly. We've traded a fairly tractable problem with a far worse problem. Weak references have their place, but using them to second guess the users idea of what should be in memory and what shouldn't be is not one of them. Their use cases are limited to associating data with objects whose life times or fields you do not control, and to respond to memory pressure. Using them to clean up eagerly registered listeners to fix unexpected coupling between components is beyond ridiculous. Memory leaks have been replaced with overzealous memory reclamation, to great surprise of the user.

Jan 13 '23 12:01 hjohn

@FlorianKirmaier This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

Mar 31 '23 23:03 bridgekeeper[bot]

I would like to keep this PR. It's already been approved by one person.

Apr 03 '23 07:04 FlorianKirmaier

@FlorianKirmaier This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

May 01 '23 09:05 bridgekeeper[bot]

@FlorianKirmaier This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

May 29 '23 10:05 bridgekeeper[bot]

jfx jfx copied to clipboard

8277848: Binding and Unbinding to List leads to memory leak

Progress

Issue

Reviewers

Reviewing

Webrev

Webrevs

jfx
jfx copied to clipboard