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

[John Hendrikx]

On Mon, 10 Mar 2025 08:48:53 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

>> This provides and uses a new implementation of `ExpressionHelper`, called `ListenerManager` with improved semantics.
>> 
>> See also #837 for a previous attempt which instead of triggering nested emissions immediately (like this PR and `ExpressionHelper`) would wait until the current emission finishes and then start a new (non-nested) emission.
>> 
>> # Behavior
>> 
>> |Listener...|ExpressionHelper|ListenerManager|
>> |---|---|---|
>> |Invocation Order|In order they were registered, invalidation listeners always before change listeners|(unchanged)|
>> |Removal during Notification|All listeners present when notification started are notified, but excluded for any nested changes|Listeners are removed immediately regardless of nesting|
>> |Addition during Notification|Only listeners present when notification started are notified, but included for any nested changes|New listeners are never called during the current notification regardless of nesting|
>> 
>> ## Nested notifications:
>> 
>> | |ExpressionHelper|ListenerManager|
>> |---|---|---|
>> |Type|Depth first (call stack increases for each nested level)|(same)|
>> |# of Calls|Listeners * Depth (using incorrect old values)|Collapses nested changes, skipping non-changes|
>> |Vetoing Possible?|No|Yes|
>> |Old Value correctness|Only for listeners called before listeners making nested changes|Always|
>> 
>> # Performance
>> 
>> |Listener|ExpressionHelper|ListenerManager|
>> |---|---|---|
>> |Addition|Array based, append in empty slot, resize as needed|(same)|
>> |Removal|Array based, shift array, resize as needed|(same)|
>> |Addition during notification|Array is copied, removing collected WeakListeners in the process|Appended when notification finishes|
>> |Removal during notification|As above|Entry is `null`ed (to avoid moving elements in array that is being iterated)|
>> |Notification completion with changes|-|Null entries (and collected WeakListeners) are removed|
>> |Notifying Invalidation Listeners|1 ns each|(same)|
>> |Notifying Change Listeners|1 ns each (*)|2-3 ns each|
>> 
>> (*) a simple for loop is close to optimal, but unfortunately does not provide correct old values
>> 
>> # Memory Use 
>> 
>> Does not include alignment, and assumes a 32-bit VM or one that is using compressed oops.
>> 
>> |Listener|ExpressionHelper|ListenerManager|OldValueCaching ListenerManager|
>> |---|---|---|---|
>> |No Listeners|none|none|none|
>> |Single InvalidationListener|16 bytes overhead|none|none|
>> |Single ChangeListener|20 bytes overhead|none|16 bytes overhe...
>
> John Hendrikx has updated the pull request incrementally with three additional commits since the last revision:
> 
>  - Small fixes from review comments
>  - Use switch expression where reasonable
>  - Update docs regarding NullPointerExceptions

> Is it possible to ignore the listener that incompatibly changed the value from Y back to X?

@mstr2 I'm not entirely sure what you mean here. But let me give a few options.

When there's two listeners that can't reach agreement, the sequence of events looks like this:

1. Value was changed from W to X (old value is set to W, and current value is X at this level)
2. First listener called with W -> X -- listener code **modifies** value to Y
    1. Nested loop starts that notifies max 1 listener (as we only notified one so far). Old value is set to X.
    2. First listener is called again but now with X -> Y -- listener does nothing this time
    3. Nested loop ends
3. Nested loop completion is detected; current value is fetched which now becomes Y for this level, old value stays at W
4. Second listener is called with W -> Y -- listener code **modifies** value to X
    1. Nested loop starts that notifies max 2 listeners (as we notified two so far). Old value is set to Y.
    2. First listener is called for a third time but now with Y -> X -- listener code **modifies** value to Y
        1. Another nested loop starts with max 1 listener. Old value is set to X
        2. First listener is called for a fourth time but now with X -> Y -- listener does nothing this time
        3. Nested loop ends
    3. Nested loop completion is detected; current value is fetched which now becomes Y for this level, old value is also Y **(!!)**
    4. Can't call second listener (it would be Y -> Y) ... second listener (that previous set value to X) may now think value is still X...
    5. This resulted in a `StackOverflowError` in old code, so we throw that...
5. We never get here, so any further listeners are now in the dark as to the current value...

So as to your question, it seems that you're asking if we could ignore what the listener is doing in step 4.ii -- here the listener is changing the value back that eventually leads to a problem.  However, we only notice that this happened in step 4.iii -- the property has already been modified by then.  Are you asking if we could change the property back again (using `setValue(X)`)?  That would be really tricky, as it would just trigger another nested notification loop...

Or perhaps you are asking if we can just ignore 4.iv and not do 4.v ?  That's what the code did a few changes ago, but which would leave you in the dark that there is conflicting changes happening (whereas with `ExpressionHelper` this would lead to the `StackOverflowError` -- not perfect, but at least you're informed...)

Or perhaps you meant something else?

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

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