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

[John Hendrikx]

On 06/02/2023 06:00, Michael Strauß wrote:
> I like your proposal in general, since it may result in a performance
> improvement in some cases. If we can avoid to eagerly registering
> listeners that aren't needed, that's great.
Thanks. It would probably simplify some cases that now require custom 
property implementations.
> However, it is not an alternative to using weak listeners. Consider
> the following test, which is taken from Florian's PR
> (https://github.com/openjdk/jfx/pull/689):
>
>      @Test
>      public void testListPropertyLeak2() {
>         JMemoryBuddy.memoryTest(checker -> {
>             ObservableList<Object> list = FXCollections.observableArrayList();
>             ListProperty<Object> listProperty = new SimpleListProperty<>(list);
>
>             checker.setAsReferenced(list);
>             checker.assertCollectable(listProperty);
>         });
>      }
>
> This test only passes with your PR because `listProperty` doesn't
> register a listener on `list` (that's what you are proposing, after
> all).

Yes, when creating a new instance of something (in this case 
`SimpleListProperty`) there shouldn't be any weird side effects. 
Registering a listener to something else in the constructor is passing 
on a reference to the instance (SimpleListProperty) before it is fully 
constructed, which is a bad practice.  Because it is bad practice, it is 
very rare to see an object creation have side effects, like it being 
registered somewhere, that would block its immediate garbage collection.

A call to `new` without an assignment to a variable, will in almost all 
cases result in that object being immediately eligible for GC. This is 
what users expect, the only one that has a reference to the object 
resulting from a `new` operator is the caller.  The cases where this 
doesn't happen are likely bugs or not idiomatic Java.  This escapes are 
problematic for various reasons (see recent discussions in amber-dev and 
the new this-escape warning).  In the strictly threaded environment of 
JavaFX you can get away with it usually, but not always.  It is still a 
huge surprise for users that expect the `new` operator to be side effect 
free.

This is why I applaud the effort which we're doing for Skins at the 
moment, moving all listener registration to the `install` method, 
preventing the instance from escaping before it is fully constructed.  
It means that just constructing the Skin does not tie it to a control 
already.  Similarly, just constructing a ListProperty should not tie it 
to an ObservableList; it doesn't matter if that tie is strong or weak.

> As soon as you do that, for example by adding the following line, the
> test fails:
>
>      listProperty.addListener((ListChangeListener<? super Object>) change -> {});
>
> 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.  There is no warning in my IDE, in my 
logs, or anywhere.  It just seems to work, and then breaks at some point 
in the future. The fact that it seems to work, but then doesn't is a 
huge problem for debugging, testing and reproducing user problems. That 
last one is nasty, because it may work on my machine; when weak 
references get cleared can depend on JVM used, JVM parameters, memory 
available, hardware platform, etc.. -- they're fine for low level work 
like caching, but using them in a high level mechanism like a UI is 
making JavaFX far more complex than it should be.
> That's a bug in the current implementation, and hiding it by making it
> contingent upon the presence of observers doesn't fix the underlying
> issue.

I don't think it's a bug.  I took an action to register a listener.  It 
is on me to take action to unregister it.  And until that time it should 
keep working.

I mean, what if Java were to decide that a `Thread` which I started is 
not doing relevant work because it determined that the objects the 
thread is using are not referenced by any other threads?  Should it be 
allowed to kill this `Thread`? And if that's allowed, should it do this 
silently without warning?

What if I added an important listener like this:

permissionsListProperty.addListener(AuditingService::auditLogPermissionChanges);

Do you think JavaFX should decide that this is not relevant because only 
the property still has a reference to this listener, but nothing else has?

Also, do you think it should keep working until a garbage collection 
occurs, but then stop working after one occurs?  When will that be?

I feel there is fundamental misunderstanding of how listeners are 
normally working in Java. JavaFX is the only system that decides that a 
listener is irrelevant based on how it is being referenced.  Nothing 
else does this.  Beans don't do this (PropertyChangeListener), other UI 
frameworks don't do this (Swing, AWT, SWT), event frameworks I worked 
with don't -- in fact, I can't think of a single example that does 
listener management the way JavaFX does.

I'm trying to get JavaFX to a state where it is more predictable, and 
for this certain tools are needed.  ObservableValue#when was one of 
those, it allows users to do their listener management upfront (no need 
to save a reference to the listener for later removal).

Subscriptions are another useful tool so you don't have to save the 
lambda or method reference for later removal:

        Subscription combinedSubscription =
               propertyA.subscribe(xyz)
                   .and(propertyB.subscribe(abc));

        // dispose of all listeneres with one line:
        combinedSubscription.unsubscribe();

Method references are not unique (or don't have to be) so doing 
"property.removeListener(this::myMethod)" will not do anything (and 
removeListener will silently ignore listeners it doesn't know about 
making this problem worse).  Subscriptions solve this problem.

These (and other patterns) are currently hand rolled by many -- many 
however simply get it wrong, from which many "memory leak", 
"unpredictable" or "bug" complaints arise. ObservableValue#when 
addresses many of these already, and exposing Subscriptions is useful as 
they're a bit lighter (I think it would be useful for Skins, although 
you could use `when` if you're careful).

Note that weak listeners don't solve the problem adequately because the 
pattern breaks when the involved property has a different life cycle 
than its listener.  Weak listeners promise (or seem to promise) that 
manual removal of listeners is never needed, but nothing is further from 
the truth.  Since manual removal is still required, even with weak 
listeners, investing in API and patterns that make this burden easier 
and less error prone is a good thing.

With this proposal I'm trying to help property implementators to not 
hold onto resources when they're not needed.  The idea here being that 
you can override observed/unobserved to register/unregister listeners 
when there's an actual need.  It's also constructed in such a way that 
it is easy to do this for sub properties.

For example, if you have the subproperties "size" and "empty", then you 
can register listeners by having all of these sub properties forward to 
a general "observed" method:

       private void subPropertyWasObserved() {
              if (!isAnySubPropertyObserved()) {
                   // register listeners
              }
       }

       // reusable helper to determine observation state:
       private boolean isAnySubPropertyObserved() {
              return size.isObserved() || empty.isObserved();
       }

And it's counter part:

       private void subPropertyWasUnobserved() {
              if (!isAnySubPropertyObserved()) {
                   // unregister listeners
              }
       }

The observed/unobserved calls happen just before the first registration 
and just after the last unregistration, making this a very simple 
pattern to use when multiple properties are involved.

--John


>
>
> On Sun, Feb 5, 2023 at 10:46 AM John Hendrikx <john.hendrikx at gmail.com> wrote:
>> Any comments on this?
>>
>> I think having API to make it easier for both JavaFX and users to create
>> listeners that can delay registration to their own dependencies. This
>> would be an alternative to using weak listeners for these cases.
>>
>> --John