RFR: 8290310: ChangeListener events are incorrect or misleading when a nested change occurs [v5]
John Hendrikx
jhendrikx at openjdk.org
Wed Feb 19 03:51:03 UTC 2025
On Mon, 17 Feb 2025 15:37:56 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:
>> This provides and uses a new implementation of `ExpressionHelper`, called `ListenerManager` with improved semantics.
>>
>> # 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 overhead|
>> |Multiple listeners|57 + 4 per listener (excluding unused slots)|57 + 4 per listener (excluding unused slots)|61 + 4 per listener (excluding unused slots)|
>>
>> # About nested changes
>>
>> Nested changes are simply changes...
>
> John Hendrikx has updated the pull request incrementally with one additional commit since the last revision:
>
> Fix fix for regression :)
> > I'm going to assume that A and B are aware of each other, and are somehow working together, as this scenario makes little sense otherwise. The code that is using these two listeners is then likely coming from the same implementation (how else would A have a reference to B).
>
> Yes, here is an example:
>
> ```java
> var property = new SimpleIntegerProperty(0);
>
> ChangeListener<? super Number> listenerB = (obs, ov, nv) -> {
> };
>
> ChangeListener<? super Number> listenerA = (obs, ov, nv) -> {
> property.set(5);
> property.removeListener(listenerB);
> };
>
> property.addListener(listenerA);
> property.addListener(listenerB);
>
> property.set(1);
> ```
>
> > So, given that, if an implementation decides to remove B, no matter when that happens exactly (outside a notification call, or within one), would it be reasonable to expect a callback still?
>
> Here is how I walked through this in my head:
>
> ```
> A gets 0->1; sets 5
> A gets 1->5; setting 5 does nothing
> B gets 0->5
> A removes B (in the 1->5 event)
> A removes B (in the 0->1 event)
> ```
I see, so from reading the code, you see `property.set(5)` and then assume (not unjustified) that everyone will first see the value change to `5`. Then the removal of B happens. I can see how that is a logical thought process. What exactly happens however is more complicated, and I guess that's why a warning to not modify the same property within a callback is still justified even with this new implementation.
There may be a way to make this a bit more logical, I think I could offer an alternative solution. I've added a passive listener C after B and a passive listener Z before A to ensure they don't receive bad values:
- (`progress` is set to 0 and we call Z)
- Z gets 0 -> 1
- (`progress` is set to 1 and we call A)
- A gets 0 -> 1; sets 5 triggering a new notification
- (nested level starts, but continues with next listener instead of from start, communicated down with the `progress` field)
- (`progress` is set to 2 and we call B)
- B gets 0 -> 5
- (`progress` is set to 3 and we call C)
- C gets 0 -> 5
- (no more listeners, set progress to `-1` to indicate a nested notification occurred and exit)
- (we now arrive back in the listener A code)
- A removes B
- (listener callback A has now completed and returned control back to the top-level notification loop; this loop notices there was a nested notification as `progress` variable now contains `-1` instead of the index; it is going to assume that every subsequent listener was notified already by the nested level, and so breaks off its loop not continuing with B or C)
- (an explicit check is done if the last called listener from its perspective (A) resulted in the value changing; normally yes, otherwise there was no nested call, but in the nested call it could have been modified again back to the value A last received)
- (if the current value (5 in this case) is not the same as what A received (1) then start a partial loop containing only the listeners notified so far in this loop)
- (`progress` is set to 0 and we call Z)
- Z gets 1 -> 5
- (`progress` is set to 1 and we call A)
- A gets 1 -> 5
I've looked at this for a few hours now, but can't say yet for sure if this is a better alternative or if it would work correctly if more listeners make changes; I think it may work. It's sort of the opposite of what the PR implementation is doing now.
The PR basically will restart from the first listener in a nested level and stop when reaching the last listener notified in the parent level. The parent level then continues where it left off. Assuming we have listeners Z, A, B, C, D, and that listeners A and C make a change once it would look like:
Z (0 -> 1)
A (0 -> 1) wants >1 so sets 2
Z (1 -> 2)
A (1 -> 2)
B (0 -> 2)
C (0 -> 2) wants >2 so sets 3
Z (2 -> 3)
A (2 -> 3)
B (2 -> 3)
C (2 -> 3)
D (0 -> 3)
The alternative idea above sort of inverts this; a nested level continues where the parent level left off and the parent will renotify all listeners it notified at its level if the current value isn't what it thinks it should be:
Z (0 -> 1)
A (0 -> 1) wants >1 so sets 2
B (0 -> 2)
C (0 -> 2) wants >2 so sets 3
D (0 -> 3)
(check if value changed, renotify anyone notified in this level, in this case B and C)
B (2 -> 3)
C (2 -> 3)
(check if value changed, renotify anyone notified in this level, in this case Z and A)
Z (1 -> 3)
A (1 -> 3)
Now, whether this will be more logical from a user perspective remains to be seen. It calls listeners in a different order, and doesn't always end with the "last" listener (this could have consequences for added listeners, but its a niche case as said before).
One thing that jumps out is that it needs even less calls to achieve the same goal.
It's really late here, and I've been thinking about this for a few hours now; I see some potential for this alternative, it may indeed be more logical, and I think it wouldn't require more information to be passed between nested levels than I do currently (currently that's the `progress` field only).
I will have to get back to you on this.
> That is, after A reacts to the new value 5, it's B's turn to react _before_ it's removed. It could be just me though.
As you can see from the above, I think you may have a valid point here.
> > Here is a comparison table:
>
> It looks good, but it doesn't strictly contradict the example. A listener should stop receiving notifications immediately when it's removed, but here it's not removed yet and still doesn't receive notifications.
It's the nature of the PR implementation, but may be odd from a user perspective.
> > I feel I also should point out that such dependencies between listeners (where listener A removes B, or adds C or whatever) can only happen in code that is aware of these other listeners. There is no way to obtain a reference to a listener from the system, and so all of these scenario's have perfect knowledge of what each listener does and their life cycles. There can be no surprises here, listeners can't be added or removed by some other class or 3rd party dependency without a reference to them.
>
> Yes, this is just another case where I'm not sure we're doing the right thing, not that I'm sure we're not.
It would still be nice if the system makes sense for implementations that manage multiple listeners in this way, however, we're basically discussing scenario's where 1 implementation for some inexplicable reason has multiple listeners on the **same** property, and is actively adding/removing them at random times, and then hopes it all still makes some sense (for varying definitions of sense... :))
Perhaps the proposed alternative would improve these extremely exotic scenario's, but we should really only consider such an alternative if it doesn't make the far more common cases harder to grasp for users...
-------------
PR Comment: https://git.openjdk.org/jfx/pull/1081#issuecomment-2667438500
More information about the openjfx-dev
mailing list