On Wed, 11 Jan 2023 15:54:25 GMT, John Hendrikx <jhendr...@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
