Feedback requested for infrastructure for properties that wish to delay registering listeners

[John Hendrikx]

On 11/02/2023 06:31, Michael Strauß wrote:
> I think that we are conflating two distinct questions in this discussion:
>
> 1. The first question is whether a listener that is registered on an
> ObservableValue should prevent the ObservableValue from being eligible
> for garbage collection.

This should never be the case, unless the listener does this by 
accident. The ObservableValue and listener can both be GC'd if the 
ObservableValue isn't referenced any more. Without a reference to the 
ObservableValue, you can't change it any more, and thus can't trigger 
the listener anymore.  At that point, there's no need to keep either around.

I've always advocated the opposite: the ObservableValue should prevent 
its listeners from being eligible for GC.  Yet, in JavaFX, with weak 
listeners, this can easily happen (even though the "stub" is still 
there).  This is what leads to hard to reproduce behavior as it relies 
on an unpredictable external event: garbage collection.

All I'm trying to do is to provide more tools to do timely removal of 
listeners and make it about as easy to do as weak listeners, but without 
the unpredictable behavior.  It is just jarring to see even more weak 
listeners being introduced considering all the downsides of this approach.

> 2. The second question is whether the object contained in an
> ObservableValue should hold a strong reference back to the
> ObservableValue.
>
> I'm only addressing the second question here. ObservableValue is a
> property system: it allows developers to declare named values (much
> like fields, but in addition to fields, it also allows observers to
> see when the value was changed). It seems obvious to me that, while a
> field refers to its value, the value of a field should not magically
> refer back to the field itself (or its declaring object). Unless, of
> course, the back-reference is manually added by the developer.
> Similarly, the value of an ObservableValue should not magically refer
> back to the ObservableValue; it is a one-way dependency by default.
>
> This one-way dependency makes a lot of sense: it binds the lifetime of
> the contained value to the lifetime of the property wrapper. The
> inverse makes no sense: why should the lifetime of the property
> wrapper be bound to the lifetime of its value? Plain fields don't work
> like this, neither do Beans, C#'s language properties, WPF's
> DependencyProperty, or any other property system that I've come
> across.
>
> And that's how most implementations in JavaFX work. Consider
> ObjectProperty: the contained value will not refer to ObjectProperty,
> regardless of whether listeners are registered on ObjectProperty.

I don't think I ever said the value should refer back to its property, 
so I think we're on the same frequency here.

However, if you are wrapping observable into observable you are actually 
doing what bindings are doing, as you can see these as wrappers as well. 
Handling the relationship between bindings with weak listeners is what 
results in the unpredictable semantics.

The dual nature of ListProperty is what is the root cause of the 
problems.  It is basically an ObjectProperty<ObservableList> that 
implements ObservableList itself and offers some convenience that could 
have been provided differently; but it doesn't act like an 
ObservableList like ObservableArrayList would, and it doesn't act like 
an ObservableValue like ObjectProperty would.

In a fluent binding chain like flatMap().map().orElse(), each part of 
the chain strongly refers to its source (ie. "orElse" refers to "map" as 
its source). When the chain is observed itself, then the opposite is 
also true ("map" refers to "orElse" as "orElse" is a party interested in 
"map"s invalidations/changes).

Replace "orElse" with "ListProperty" and "map" with "ObservableList" and 
it's incredibly similar.

> But ListProperty is an anomaly: it adds a ListChangeListener to its
> contained list, which causes the contained list to magically refer
> back to ListProperty without the intervention of the developer. This
> violates the expectation that there's a one-way dependency between an
> ObservableValue and its contained value.

It's not a normal value though, it's also an Observable, and with any 
binding, there are references back and forth.

In what situation would this be a problem though, assuming the 
ListProperty itself is actively in use? Remember that I'm proposing this 
strong reference from ObservableList to ListProperty is only present 
when the ListProperty itself is being observed.

> The anomaly is caused by the decision to have ListProperty also
> implement ObservableList: this makes it looks like ListProperty (this
> is our ObservableValue) is actually the same thing as its contained
> value (the ObservableList). But it's not, from the perspective of
> ObservableValue these are two very distinct things. One is the
> wrapper, the other is the contained value.
>
> Purely as an implementation detail, ListProperty needs the internal
> ListChangeListener to replay the changes of its contained list, in
> order to perfectly masquerade as its contained list. But
> implementation details shouldn't change the semantics of the
> fundamental API, and this is why the internal dependency between
> ListProperty and its contained list needs to be broken by a weak
> reference.
>
> Note that I'm not talking about listener semantics at all. This is
> just about the semantics that govern the relationship between an
> ObservableValue and its contained value. Florian's PR is a targeted
> fix for this very specific bug, and I remain convinced that this is a
> bug.
>
> Furthermore, fixing this bug has no bearing on whether and how we
> address question 1, which you are passionate about. Leaving the
> current behavior as-is does not bring us closer to having "sticky
> listeners". It seems to me that much of what you've written is an
> endorsement for sticky listeners, but does not address the very narrow
> problem of the PR in question.

The decision to have ListProperty implement both ObservableList and 
ObservableValue is indeed as you say an "anomaly".  These two interfaces 
can't be implemented correctly in a single class and I think this is 
where the problems originate -- it can only perfectly mimick one of 
these.  They both implement Observable for example, which specifies 
methods for invalidation listeners.  For ObservableValue this would need 
to trigger when its value (a reference to the list as returned by 
`getValue`) is invalidated, while for ObservableList it would trigger 
when any mutation occurs.

On top of that, the ObservableValue part doesn't behave like any other 
ObservableValue. Calling `get` gets you the ObservableList, but adding a 
ChangeListener triggers many times with nonsense old and new values when 
modifying the underlying list (ie. something like: "old value = [A], 
current value = [A]" when adding "A" to an empty list).

I understand the desire to have something better than 
"ObjectProperty<ObservableList<T>>" and so "ListProperty<T>" was born. 
But you can also just use the ObservableList directly (without wrapper); 
this is 99% effective already, with only a few downsides:

1) I can't bind to empty/size
2) I can't replace the entire list efficiently (requires clear/addAll)
3) There is no change listener (only list change listener)

1) can easily be fixed by just having a convenient way to construct 
these bindings as all that is needed is to add an invalidation listener 
to the observable list, and a computeValue that calls `size` or 
`empty`.  These can probably just be default methods in `ObservableList`.

2) could probably be done with a default method (but I haven't really 
looked much into it).  By default it can do a clear/addAll, allowing 
smarter more efficient implementations to override this. These 
implementations never implement `ObservableValue` -- it's already 
observable, and just as distinct as a boolean or double; there is no 
need to wrap it again.

3) there are already list change listeners; change listeners offer 
nothing over these or invalidation listeners, unless they could 
correctly return the original list as old value; as it is, they're worse 
than useless; not having them is actually better than having broken 
ones, less confusing too.

>
>>> But the test shouldn't ever fail, because the ObservableList that is
>>> wrapped by ListProperty should never keep a strong reference to the
>>> ListProperty, even if the ListProperty is itself observed.
>> Why should it work that way?  The alternative is that my listener is
>> discarded without my knowledge.
> You didn't add a listener to the contained list; you added a listener
> to the property wrapper. This listener is not discarded.
> It may seem like it was, but that's only because ListProperty fooled
> us into thinking it was the same thing as its contained list.

I added it to an ObservableList, an interface with a contract, which may 
or may not be a ListProperty; this contract is what ListProperty claims 
to implement.

If I have two ObservableList's, A and B, that I received from somewhere, 
the interface documentation tells me what to expect. For some reason 
though, if I add an invalidation listener to A it keeps working, but if 
I add it to B, it breaks after a GC.

A is an ObservableArrayList

B is a ListProperty

An interface is supposed to be solid contract that you can rely upon.  
ListProperty acts like they're just guidelines and does its own thing 
whenever the gap between ObservableList and ObservableValue is too hard 
to reconcile easily.

--John