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