On Mon, 21 Sep 2026 23:12:15 GMT, Kevin Rushforth <[email protected]> wrote:
> The updated docs look good with one comment inline. > > There are a few more corner cases in the implementation to consider. Only the > first one is confirmed. Addressing the others in follow-up issues is probably > OK even if they are reproducible: > > Confirmed cases: > > 1. If the sole change listener of an ObservableValue replaces itself with > another change listener and makes a nested change, it will not work correctly. > > A) If L1 first removes itself, then adds L2, and finally makes a change, that > second listener will be notified during that cycle. > > B) If L1 first adds L2, then removes itself, and finally makes a change, it > will lose track of L2 and not notify it on a subsequent change. I've written an extensive test case that checks various nasty behaviors that listeners may do, that catches this one and most of the others below. I'll make that available + fixes soon. The impact is primarily more edge-case handling code (the various axis with two variants caching/non-caching + allowing single listeners in direct fields, and the locking behavior is resulting in quite a lot of potential code paths). > Unconfirmed (potential) cases: > > 2. Two nested changes from the same change listener callback can cause the > change event to be wrong > > Given L1 and L2 and an initial value of 0; set value to 1; In L1 if newval == > 1, change the value to 2 and then to 3; L1 will see a nested 1->2 (and miss > 2-> 3), while L2 will see 0->3 (as expected); then make a top-level change to > set value to 4; L1 sees 3->4 never having seen 2->3 Confirmed -- this is the one I checked first as I figured it would have the most impact; it actually simplified the notification loop (no more two way communication up the stack and down the stack in the `progress` field, just one way only). The cost is that we call `getValue` more often, but I think that's not really a cost (unobserved lazy properties don't cache their value, but if they're unobserved we won't be in the notify loop) > 3. Adding a second change listener from a nested change listener callback can > unlock the list twice > > Given L1 and an initial value of 0; set value to 1; in L1 if newval == 1, > change val to 2, if newval == 2, add L2. The list may be unlocked twice, > leading to an exception or erroneous behavior. This should be solved in the next update. > 4. Adding a single change listener when there are two invalidation listeners > can leave the wrong cached value if the first invalidation listener vetoes > the change > > Given IL1 and IL2 and an initial value of 0; set val to 1; In IL1, the first > time it is called, add CL1 and set the property back to 0 (vetoing it); CL1 > will not be notified (which is correct), but the property will remain > invalid, further the ListenerManager's cached value will be incorrectly left > at 1, meaning that it will miss a subsequent change to 1 if that later change > is not vetoed. > > This is an admittedly bizarre thing to do in an invalidation listener. For sure, my "nastyListener behavior" test case will catch that now. > 5. Scalar Bindings capture the cached value before calling `onInvalidating`, > which can misbehave if called reentrantly > > Given a binding observing a value of 0; if its dependency changes to 1, and > an override of onInvalidating reads the binding and changes the dependency to > 2, the nested invalidation will report 1->2 and then 0->2 after the original > binding resumes. > > This would be a very uncommon corner case. This turns out to be the most annoying one, and I therefore want to defer doing anything about it. The problem is that `onInvalidating` should behave basically like any other `InvalidationListener` would now, but as it is completely outside the `ListenerManager` system none of the guarantees it provides will hold there. Some observations: - In JavaFX, `onInvalidating` is only overridden by `SelectBinding` - Nothing "nasty" happens there, so FX provided classes are not affected by this corner case - It is equivalent to `invalidated()` on the property tree with a different name (and is overridden 800+ times) - Many of those do nasty stuff - Because those use the old value caching variant, there is no issue when a nasty `invalidated()` messes up the value, as the cached old value will be the correct one to use still - If you **do** do nasty stuff in `onInvalidating` it can break the contract of a `ChangeListener`, so at some point it does need addressing IMHO Potential solutions: - Forget about the OldValueCaching/Normal variants and use the first one everywhere -- I would personally find that a shame, as I think we should push towards the non-caching variants not the other way around - Although a shame, it would cut down on a lot of code, at the cost of more memory use for bindings that use a change listener (lazy properties chain with invalidation, so it wouldn't affect those) - More special casing in the normal `ListenerManager` and pass in the `onInvalidating` hook to the manager so it can do the correct thing -- this will make it diverge further from the old value caching variant - Add (yet another) `boolean` field, but this time for the 7 binding types only -- the boolean would track if there is a nested `onInvalidating` happening, and if so skip calling `fireValueChanged` for those -- as before, the extra `boolean` will essentially be free as the bindings have space for it; we would not need to add extra methods to access it via the listener manager, it is local to the binding (so lighter than the `notifying` boolean). I don't like any of them very much, but starting to lean towards that last option. TLDR; I will address all of these in my next update, except for that last one that I want to think over a bit more ------------- PR Comment: https://git.openjdk.org/jfx/pull/1081#issuecomment-5833127621
