On Fri, 11 Sep 2026 23:57:03 GMT, Kevin Rushforth <[email protected]> wrote:
> In addition to the spec comments, I discovered one implementation bug. If a > property has a single `ChangeListener`, and that listener first adds a second > `ChangeListener` and then changes the value, the second listener will be > notified of the change. If there already were two or more change listeners on > the property, it works as expected. This is quite a rabbit hole that was uncovered here, that requires a fix with some real trade-offs. The problem in a nutshell is that when there is only a single listener, which adds in its callback a new listener, that the shape changes from single listener to a list of listeners. This new `ListenerList` is unaware it was constructed in the middle of a notification, and its mechanism to ensure correct change events (locking) is inactive at that point. The solution is to construct the `ListenerList` in a locked state (basically constructing it with the single listener, then locking it, then adding the 2nd listener that required the construction of a listener list). The problem is how do we know that we must construct a locked list? When just adding a listener in the non-notifying case, it should be unlocked. It should be constructed locked in the notifying case only. I have dug deep here, and I see 3 possible solutions to track whether a property is currently in the middle of a notification (in order of preference I think): 1. We add a `boolean` flag to every property type. The property is set to true when a notification starts, and reset to previous value when it ends. The flag is then used to see if we're in the middle of a notification, allowing us to decide whether the listener list should be created locked or unlocked. 2. We always use wrappers around single listeners (currently only 1 in 4 cases need a wrapper to track the old value which was a nice memory saving win over `ExpressionHelper` that we would need to drop now). The wrapper can then carry the `boolean` flag from option 1, and used in the same way. 3. We use a `TheadLocal` stack of active notifications; this probably requires the least amount of memory overall, but ties "simple" properties to thread state. The `ThreadLocal` would contain a stack of active notifications, and to check if we need to construct a listener list locked or unlocked, we check if the property instance involved is part of that stack. Table summary: |Solution|Memory implications|Allocations during notification|CPU cost|Threading blast radius(**)| |---|---|---|---|---| |Extra `boolean` field next to `listenerData`|No cost in JDK27+|None|Set/Reset flag before/after notification|Current property only| |Always use wrapper|Lose cheap bare listeners, but on par with `ExpressionHelper`|None|Set/Reset flag before/after notification|Current property only| |ThreadLocal stack|Ammortizes to 0|Ammortizes to 0(*)|AddLast/RemoveLast to List before/after notification|All properties of same type| (*) Ties property system to thread locals, allocation cost is only zero if we don't clean up this list per thread... (**) Properties aren't thread-safe, but knowing how bad things can get or if it may affect unrelated properties is still valuable I recommend we go for the first option to solve this (a new `boolean` flag on properties that use the new `listenerData` mechanism), as it seems to win on all counts. We can even change our minds between all of these options later if the trade-offs turned out to be incorrect or developments in the JDK allow for a different better trade-off. For now I've assumed we'll soon all be using JDK27+ (compact object headers) which makes the extra boolean field free on all current property types (it was free for all types before JDK27 as well, except for the `ReadOnly*` variants). Always using wrappers makes a different trade-off which I think comes out slightly worse. The second option is also acceptable (it simplifies the code a bit as well, no more wrapper and bare cases, wrappers always). The third option I've included as the only other alternative I've found to solve this, but I would recommend against it. @andy-goryachev-oracle @kevinrushforth @Maran23 @mstr2 @nlisker -- what do you think? ------------- PR Comment: https://git.openjdk.org/jfx/pull/1081#issuecomment-5657584101
