On Wed, 11 Jan 2023 13:09:37 GMT, Florian Kirmaier <fkirma...@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.
-------------
PR: https://git.openjdk.org/jfx/pull/689
