RFR: 8290310: ChangeListener events are incorrect or misleading when a nested change occurs [v3]

Mon Feb 17 23:50:17 UTC 2025

On Mon, 17 Feb 2025 16:19:29 GMT, Nir Lisker <nlisker at openjdk.org> wrote:

> > What part in the PR section? I found this:
> > > Added listeners are only called when a new non-nested (top level) notification starts
> 
> Maybe I misunderstood what that section says. When I look at the list of differences it's written from the point of view of the new code:
> 
> > * Provides correct old/new values to ChangeListeners under all circumstances
> > * Unnecessary change events are never sent
> > * Single invalidation or change listeners are inlined directly into the observable class (in other words, properties with only a single listener don't take up any extra space at all)
> >   ...
> > * Added listeners are only called when a new non-nested (top level) notification starts
> 
> But the table says "New listeners are never called during the current notification regardless of nesting". This is the mismatch I noticed. I think that that bullet point talks about the old code.

I don't quite see the mismatch (but I'm not a native speaker, so I may be missing something obvious).  Both are supposed to be  talking about the new code:

"Added listeners are only called when a new non-nested (top level) notification starts"

With that I mean that when you add a listener to a property during its own notification, that listener will not be called until a change occurs after the notification completes (ie. the change must be triggered by some 3rd party, not during the fire value changed callback).  

When the addition occurs in a nested notification for the same property, then it will not be called until the nested notifications have all completed, the top level notification completes, and **then** another change is made triggering a new notification.  You could see it as the new listener is put on hold in a pending list, and this list is only updated when the property is in an "idle" state (ie. no notifications are ongoing).

"New listeners are never called during the current notification regardless of nesting"

I think this sort of says the same.  Any new listeners added to a property are not called when a notification (nested or not) is still ongoing.  The property must become idle before the new listener will participate.

> > Both `ExpressionHelper` and this implementation will not notify an added listener within a callback for the same notification. `ExpressionHelper` will notify the newly added listener though if a _nested_ change occurs before the original notification completes, while this implementation considers this to be integral with the first change.
> 
> This is what I observed, along with the example you give later. I also agree that the only place to add the listener in this implementation would be at the end of the top level in order to avoid the incorrect old value nested notifications.
> 
> Let's look at 2 cases where a listener is added inside another, one before a nested change happens and one without a nested change. In the case where a nested change happens:
> 
> * old code: the new listener will receive an incorrect value in the nested level.
> * current new code: the new listener will not receive a notification.
> * prospective new code: the new listener will receive a correct value in the top-level.
> 
> In the case where an addition happens without a nested change:
> 
> * old code: the new listener will not receive a notification.
> * current new code: same as above.
> * prospective new code: the new listener will receive a correct value in the top-level.

Yes, this is pretty much spot on.

> So in the first case, the value is corrected. 

I'm unsure what you mean by corrected here,  A listener has no notion of a current value that must be corrected.  If you want your listener to immediately receive the current value, then use `subscribe(Consumer)` -- `addListener(ChangeListener)` does not do an immediate callback, and always required a 2nd piece of code that does its own `get` if that was important (which it usually is, which is why I was very happy to see `subscribe(Consumer)` make it into FX).

> I would say that the current code removing the (incorrect) notification of the new listener is a behavioral change. In the second case, the prospective code's new notification is a behavioral change. Looks like we'll have to decide where the divergence is.

But is it a specified behavior or just an implementation artifact of `ExpressionHelper`? `ExpressionHelper` could conceivably be modified to not copy lists either, and just track an index to avoid copies for "performance reasons"; the small behavioral change would be mostly dismissed as "within specification".  This change proves that it can be done, and with this concept as a template (especially the `progress` field trick) `ExpressionHelper` could be optimized to avoid copies (and I suppose with some more work, also provide correct old values). 

I think we can go either way here, as none of this is specified anywhere.  JavaFX is full of listeners, and yet it builds and all tests run, so it seems at least not much is relying on this (unspecified) behavioral change. `ExpressionHelper` itself has had several detectable behavioral changes over the course of its existence as well.

For me, the main question would be: "Does it make sense for a listener added during a notification to immediately partake in the notification that added it?".  

`ExpressionHelper` does not notify newly added listeners in the overwhelming majority of cases.  It only happens when adding a new listener during a notification, and then making another change (which is discouraged see `InvalidationListener#invalidated`) 

I see only two options:
- Notify new listeners near the end (because they're appended, not for any other reason) as part of a higher level (top level?) change in all cases, nested or not
   - This means a newly added listener even in the simple case gets the notification of the one that added it (this may be quite surprising, and we'll have to see if FX even builds if I do this :))
- Never notify new listeners when an ongoing change is going on (ie. while the property is "busy" in a callback)
  - This is the current implementation, and will probably do exactly what `ExpressionHelper` does in 99.9% of the cases

I've tried getting my head around an option that would more closely do what `ExpressionHelper` does, but I don't see how I could make it work without major fundamental changes (and without sacrificing old value correctness again).  We'd probably have to go to a system then where all listeners are notified of the first change first, and any further changes are queue'd up, to be executed in a 2nd (full) pass (and so on), so something like this:

Value is set to from 4 to 5.

A gets 4 -> 5
B gets 4 -> 5; adds listener F
C gets 4 -> 5; changes value to 6, but we finish the current notification first
D gets 4 -> 5
E gets 4-> 5
-- check is done if value was unchanged; we expected 5 but see 6, so another round is triggered --
A gets 5 -> 6
B gets 5 -> 6 (does not add another listener)
C gets 5 -> 6 (does not change value again)
D gets 5 -> 6
E gets 5 -> 6
F gets 5 -> 6

That can't be done with the current implementation though, it would basically be a new implementation, one that basically disallows nested changes of the same property by processing changes one by one (I'm not entirely sure what the consequences of that could be during say a complex layout, aside from shorter stacktraces)

One immediate problem however is that the `newValue` received may differ from a direct `get` (D receives 4 -> 5 while the value is already 6) -- it's not easily fixed, and the solution for this problem chosen by `ExpressionHelper` (always send the latest value) basically means you can't rely on old/new value combinations, which is the whole reason for this PR in the first place ;-)

-------------

PR Comment: https://git.openjdk.org/jfx/pull/1081#issuecomment-2664241252