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

Sun Feb 5 20:13:59 UTC 2023

On Tue, 3 Jan 2023 09:42:05 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

>> This contains the following:
>> - Nested changes or invalidations using ExpressionHelper are delayed until the current emission completes
>>   - This fixes odd change events being produced (with incorrect oldValue)
>>   - Also fixes a bug in ExpressionHelper where a nested change would unlock the listener list early, which could cause a `ConcurrentModificationException` if a nested change was combined with a remove/add listener call
>> - A test for ExpressionHelper to verify the new behavior
>> - A test for all *Property and *Binding classes that verifies correct listener behavior at the API level (this tests gets 85% coverage on ExpressionHelper on its own, the only thing it is not testing is the locking behavior, which is not relevant at the API level).
>> - A fix for `WebColorFieldSkin` which triggered a nested change which used a flag to prevent an event loop (I've changed it now to match how `DoubleFieldSkin` and `IntegerFieldSkin` do it
>
> John Hendrikx has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains two commits:
> 
>  - Merge branch 'master' of https://git.openjdk.org/jfx into feature/delayed-nested-emission
>  - Delay nested event emission

> The fix for the bug with nested events releasing the lock seems fine. I'm still not sure if the behavioral change is what we want the result to be even if it's better than what we have now. I have mentioned this in [#108 (comment)](https://github.com/openjdk/jfx/pull/108#issuecomment-590878643). we need to decide on a spec first, and there are several facets to it as discussed in the mailing list link above ([#837 (review)](https://github.com/openjdk/jfx/pull/837#pullrequestreview-1233910264)).

Is there any doubt as to what the old value should be?  I think this is a bug fix only, and although I think it is good to discuss the whole listener system, I think we shouldn't delay this fix too much.

After all, it is a very common pattern to use the old value for unregistering listeners when listening to a property chain (like node -> scene), and if the "old property" is ever incorrect, then the `removeListener` call will silently fail (shame we can't change that any more).

> I will re-summarize what I think needs to be defined before implementing a behavior change like the one proposed here:

I don't think this is a behavior change, it is a bug fix, unless we document that `ChangeListener` old value is a "best effort value" that cannot be trusted to contain anything useful when nested changes occur.  Nested changes can easily occur in the huge web of properties that are sometimes woven (take the layout code for example).

> ### Listener order
> When listeners are registered, they are added to some collection (an array, currently). The add/remove methods hint at an order, but don't guarantee one. Do we specify an order, or say it's unspecified?

I think it is too late to change anything significant here.  Also the primary reason we've been pushing for a change here is I think the poor (remove) performance when there are many listeners.  However, I think having many listeners on a single property is always an unwanted situation.  The remove performance is simply the first problem you'll encounter.  The next problem you'll encounter is when 10.000+ listeners need to be notified of a change... it will quickly become unworkable.

Furthermore, there are data structures that have `O(1)` add(end) + remove(T) performance and `O(n)` iterate performance, while still being sequential and allowing duplicates.  The remove "problem" could be solved that way without breaking existing code, but that will not speed up notification (which will be your next problem).  I've mostly abandoned fixes in this area as I think it is **never** a good idea to have so many listeners that remove performance becomes an issue.

So, if it were up to me, and I'm pretty convinced this is going to break quite some code out there, I'd say it should be:

- listeners are called in the order they're registered
- duplicates are allowed
- removing a listener removes the first instance of that listener (not the last or a random one)

Since there are data structures that can handle the requirements we need (we only need add-at-end, remove(T) and iteration) I think we're not locking ourselves into too much problems (the cost of such a data structure is slower get(index) and remove(index) performance, but we don't need these). 

> ### Event dispatch order
> The order in which listeners are triggered is not necessarily the order of the listeners above. Do we specify a dispatch order, or say it's unspecified? If it's specified, the listener order is probably also specified.

I'll consider this the same problem, with the same risks, see above.

> We are also dispatching invalidation events before change events arbitrarily. Do we dispatch the event to all listeners of one type and then to the other, or do we want to combine them according to a joint dispatch order? We either say that they are dispatched separately, together in some order (like registration), or that it's unspecified.

I think there may be good reasons to do invalidation first, but we don't need to specify it.  An experiment where these orders are changed and running all the tests probably can give some insights here. Either leave unspecified or specify as it works currently as.  I think here it is less likely things will break though.

> ### Nested listener modifications
> If a listener is added/removed during an event dispatch, do we specify it will affect ("be in time for") the nested event chain, the outer event chain, or that it's unspecified?
> 
> ### Nested value modifications
> If a listener changes the value of its property during an event dispatch, do we specify that the new event will trigger first, halting the current event dispatch (depth-first approach), or that the new event waits until current one finishes (breadth-first approach), or that it is unspecified? Do we guarantee that when a listener is invoked it sees the new value that was set by the latest nested change, or will it see the value that existed at trigger time, or is it unspecified?

I don't think we should care about depth-first, breadth-first. The only thing that I think is important here is that the contract of `ChangeListener` is respected.  I think that that contract should be:

- `oldValue` and `newValue` should always be different values
- a subsequent notification should have `newValue` as `oldValue` as that's almost a requirement to do anything sensible with `oldValue`, and also implied (it is the old value, aka. the previous value, the value that this property held since its last change)

So when I change something form A to B, and a nested listener changes it from B to C, then any of these change events are fine:

- A -> B + B -> C
- A -> C

But not:

- A -> B + A -> C [listener not unregistered on B]
- A -> B + C -> C [listener not unregistered on B]
- A -> C + B -> C [listener registered twice on C]

> If listeners affect each other with nested events, then the dispatch order matters.
> 
> If we answer "unspecified" to the questions above, it allows us more implementation freedom. It will also mean that listeners should be thought of as "lone wolves" - they are not intended to talk to each other or depend on each other; each should do something regardless of what the others are doing and assume its code "territory" is not affected by other listeners. The user takes responsibility for coupling them (nested modifications). On the other hand, no guarantees bring limited usage. The question is, what is the intended usage?

This PR does not make listeners more/less dependent than they are. It fixes one major bug that can cause `ConcurrentModificationException` and it ensures a useful old value in the face of nested changes.

Since we probably can't disallow duplicate listeners at this stage, any new implementation needs to support this. This makes simple map based solutions a lot trickier (they'd need a counter). We'd also be going against years of standard practices for UI frameworks that work with listeners and which are using the add/remove pattern (Swing, AWT).  They all are ordered and allow duplicates.

I think that at this stage, it would be best to document it as it works currently, leaving maybe a few small areas unspecified still.  Any performance problems can be tackled within these restrictions.  It is certainly possible to make a structure that has all of these qualities:

- sequential
- allows duplicates
- fast add-at-end
- fast remove(T), removing the first match
- fast iteration
- low memory use (no wrapper needed per listener), although you'll never beat `ArrayList`
- no need for copying

Again though, not sure what problem that will solve exactly... 10.000 listeners on a single property is going to perform badly no matter what you do.

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

PR: https://git.openjdk.org/jfx/pull/837