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

Reply via email to