8290310: ChangeListener events are incorrect or misleading when a nested change occurs

[hjohn]

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 nulled (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 that are made to a property that is currently in the process of notifying its listeners. This all occurs on the same thread, and a nested change is nothing more than the same property being modified, triggering its listeners again deeper in the call stack with another notification, while higher up the call stack a notification is still being handled:

       (top of stack)
       fireValueChangedEvent (property A)  <-- nested notification
       setValue (property A)  <-- listener modifies property A
       changed (Listener 1)  <-- a listener called by original notification
       fireValueChangedEvent (property A)  <-- original notification

How do nested changes look?

Let's say we have three listeners, where the middle listener changes values to uppercase. When changing a property with the initial value "A" to a lowercase "b" the listeners would see the following events:

ExpressionHelper

Nesting Level	Time	Listener 1	Listener 2	Listener 3	Comment
0	T1	A -> b
0	T2		A -> b		Value is changed to B
1	T3	b -> B			A nested loop started deeper on the call stack
1	T4		b -> B
1	T5			b -> B
0	T6			A -> B	Top level loop is resumed

Note how the values received by the 3rd listener are non-sensical. It receives two changes both of which changes to B from old values that are out of order.

ListenerManager (new)

This how ListenerManager sends out these events:

Nesting Level	Time	Listener 1	Listener 2	Listener 3	Comment
0	T1	A -> b
0	T2		A -> b		Value is changed to B
1	T3	b -> B			A nested loop started deeper on the call stack
1	T4		b -> B		The nested loop is terminated early at this point
0	T5			A -> B	Top level loop is resumed

Note how the 3rd listener now receives an event that reflects what actually happened from its perspective. Also note that less events overall needed to be send out.

About Invocation Order

A lot of code depends on the fact that an earlier registered listener of the same type is called before a later registered later of the same type. For listeners of different types it is a bit less clear. What is clear however is that invalidation and change listeners are defined by separate interfaces. Mixing their invocations (to conserve registration order) would not make sense. Historically, invalidation listeners are called before change listeners. No doubt, code will be (unknowingly) relying on this in today's JavaFX applications so changing this is not recommended. Perhaps there is reason to say that invalidation listeners should be called first as they're defined by the super interface of ObservableValue which introduces change listeners.

About Listener Add/Remove performance

Many discussions have happened in the past to improve the performance of removing listeners, ranging from using maps to ordered data structures with better remove performance. Often these solutions would subtly change the notification order, or increase the overhead per listener significantly.

But these discussions never really looked at the other consequences of having tens of thousands of listeners. Even if listeners could be removed in something approaching O(1) time (additions are already O(1) and so are not the issue), what about the performance of notifying that many listeners? That will still be O(n), and so even if JavaFX could handle addition and removal of that many listeners comfortably, actually using a property with that many listeners is still impossible as it would block the FX thread for far too long when sending out that many notifications.

Therefore, I'm of the opinion that "fixing" this "problem" is pointless. Instead, having that many listeners should be considered a design flaw in the application. A solution that registers only a single listener that updates a shared model may be much more appropriate.

About Old Value Correctness

...and why it is important.

A change listener provides a callback that gives the old and the new value. One may reasonably expect that these values are never the same, and one may reasonably expect that the given old value is the same as the previous invocation's new value (if there was a previous invocation).

In JavaFX, many change listeners are used for important tasks, ranging from reverting changes in order to veto something, to registering and unregistering listeners on properties. Many of those change listeners do not care about the old value, but there are a significant number that use it and rely on it being correct. A common example is the registering of a listener on the "new" value, and removing the same listener from the "old" value in order to maintain a link to some other property that changes location:

    (obs, old, current) -> {
          if (old != null) {
               old.removeListener(x);
          } 
          if (current != null) {
               current.addListener(x);
          }
    }

The above code looks bug free, and it would be if the provided old values are always correct. Unfortunately, this does not have to be the case. When a nested change is made (which can be made by a user registered listener on the same property), ExpressionHelper makes no effort to ensure that for all registered listener the received old and new values make sense. This leads to listeners being notified twice with the same "new" value for example, but with a different old value. Imagine the above listener receives the following change events:

      scene1 becomes scene3
      scene2 becomes scene3

The above code would remove its listeners from scene1 and scene2, and register two listeners on scene3. This leads to the listener being called twice when something changes. When later the scene changes to scene4, it receives:

      scene3 becomes scene4

Because it registered its listener twice on scene3, and only removes one of them, it now has listeners on both scene3 and scene4.

Clearly it is incredibly important that changes make sense, or even simple code that looks innocuous becomes problematic.

The PR

The ListenerManager differs from ExpressionHelper in the following ways:

• Provides correct old/new values to ChangeListeners under all circumstances
• Unnecessary change events are never sent
• Single invalidation or change listeners are inlined directly into the observable class (in other words, properties with only a single listener don't take up any extra space at all)
• Performance is slightly worse when calling change listeners (but remember that ExpressionHelper is not following the contract).
• Removed listeners are never called after being removed (even if they were part of the initial list when the notification triggered)
• Added listeners are only called when a new non-nested (top level) notification starts
• Locking and maintaining the listener list works a bit differently -- the main takeaway is that the list indices remain the same when modified during nested modifications, which allows using the same list no matter how deep the nesting
• No reference is stored to the ObservableValue and no copy is kept of the current value
• Memory use when there is more than 1 listener should be similar, and better when not
• Although complicated, the code is I think much better separated, and more focused on accomplishing a single task:
  • ListenerManager manages the listener storage in property classes, and the transformation between the listener variants (it either uses listeners directly, or uses a ListenerList when there are multiple listeners).
  • ListenerListBase handles the locking and compacting of its listener lists.
  • ListenerList which extends ListenerListBase is only concerned with the recursion algorithm for notification.
  • ArrayManager handles resizing and reallocating of arrays.
  • There are variants of ListenerList and ListenerManager which can cache their old values when its not possible to supply these (this has a cost, and is what ExpressionHelper does by default).

The notification mechanism deals with nested notifications by tracking how many listeners were notified already before the nested notification occurs. For example, if 5 listeners were notified, and then listener 5 makes a nested change, then in that nested change only the first 5 listeners are notified again (if they still exist). The nested loop is then terminated early, at which point the top level loop resumes: it continues where it left of and notifies listener 6 and so on. This ensures that all notifications are always correct, and that listeners that "veto" changes can effectively block later listeners from seeing those at all.

For example, if the first listener always uppercases any received values, then any succeeding listeners will always see only uppercase values. The first listener receives two notifications (X -> a and a -> A), and the second receives only X -> A. Contrast this with the old ExpressionHelper, which sends odd notifications to the second listener (a -> A and X -> A, in that order).

Unfortunately, due to a somewhat weird design choice in the PropertyBase classes, the strategy of not having to cache the "current value" (or old value) can't be used (it can only be used for Bindings for now). So there is another variant of this helper, called OldValueCachingListenerHelper, with some slight differences:

• Has an extra field for storing the old value when there are any ChangeListeners active
• Can't store a ChangeListener inline; a minimal wrapper is needed to track the old value (ExpressionHelper does the same)

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

• JDK-8290310: ChangeListener events are incorrect or misleading when a nested change occurs (Enhancement - P2)

Reviewers

• Michael Strauß (@mstr2 - Reviewer) 🔄 Re-review required (review applies to 139c2aa4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1081/head:pull/1081
$ git checkout pull/1081

Update a local copy of the PR:
$ git checkout pull/1081
$ git pull https://git.openjdk.org/jfx.git pull/1081/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1081

View PR using the GUI difftool:
$ git pr show -t 1081

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1081.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back jhendrikx! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[nlisker]

John and I discussed this off-list. I will write a short review of this change.

Behavior

The solution implements has the following behaviors, which are compared to the current (partially-flawed) ones:

• Listeners are invoked in the order they were registered, with invalidation listeners being invoked before change listeners. This is also the current behavior. The behavior of invoking all listeners according to the order of registrations will be investigated.
• Listeners that are removed during event propagation will be removed immediately, and will not receive the event (if they hadn't already). This differs from the current behavior of removing the listeners only after the event finished (buggy implementation).
• Listeners that are added during event propagation will be effectively added after the event finishes, and will not receive the event during which they were added. This is also the current behavior (buggy implementation). The behavior of adding the listeners immediately, as is done with removal, will be investigated.
• Nested events are invoked "depth-first", meaning that the parent event propagation is halted until the nested event finishes (see below). This differs from the current behavior that takes the "breadth-first" approach - each event finishes before the nested one starts (buggy implementation).
• Nested events are only handled by listeners who received the parent event already so that they can react to the new change. Listeners that did not receive the parent event will only get a single (updated) event so that they don't react to irrelevant values. This allows vetoing, This differs from the current behavior that sends all events to all listeners (buggy implementation).

Examples

Suppose 5 change listeners are registered when an event starts.
Removal during an event

L1 gets the event
L2 gets the event and removes L4
L3 gets the event and removes L2 (L2 already got the event)
L4 does not get the event (removed by L2)
L5 gets the event
final listeners: L1, L3, L5

Addition during an event

L1 gets the event
L2 gets the event and adds L6
L3-L5 get the event
L6 does not get the event (added by L2)
final listeners: L1 - L6

Nested event (value change during an event)

The observable value changes from 0 to 1
L1 gets 0->1
L2 gets 0->1
L3 gets 0->1 and sets the value to 2 (vetoing)
L1-L3 get 1->2 (nested event - listeners can react to the new change)
L4-L5 get 0->2 (parent event continues with the updated value)

Recursive change (see https://continuously.dev/blog/2015/02/10/val-a-better-observablevalue.html)
The code

IntegerProperty p = new SimpleIntegerProperty(0);
// L1
p.addListener((obs, old, val) -> {
    if (val.intValue() > 0) {
        p.set(val.intValue() - 1);
    }
});
// L2
p.addListener((obs, old, val) -> System.out.println(old + " -> " + val));
p.set(2);

will trigger

L1 0->2 (set 1)
L1 2->1 (set 0)
L1 2->0
L2 is not triggered because the updated event is 0->0
Nothing is printed

instead of the current behavior that will print

1->0
2->0
0->0

Equality check

Change events require a comparison method for the old and new value. The 2 candidates are reference equality (==) and object equality (Objects#equals). There is some inconsistency in JavaFX about how this equality check is made (it is made in a few places on a few different types). It makes sense to do == with primitive types, and equals with String and the primitive wrappers. For other types, it depends on their characteristics. The "safer" option is == because a change that is triggered by != can then be tested for !oldValue.equals(newValue) in the listener and be vetoed; the opposite is not possible. This might mean that the user will have to give the comparison method that is desired.

Currently, == is used except for String. The behavior is preserved in this change, but will be investigated further in order to allows for more sensible change events.

Performance

Performance both in memory and speed is either equal or slightly worse than the current one. This is because the current behavior is wrong and fixing it entails more complications. In practice, the difference should be small. Registering many listeners on the same observable is not recommended and has caused issues in the past as well. Performance is a WIP and benchmarks will be posted later.

[hjohn]

John and I discussed this off-list. I will write a short review of this change.

I have some small corrections I think.

• Nested events are invoked "depth-first", meaning that the parent event propagation is halted until the nested event finishes (see below). This differs from the current behavior that takes the "breadth-first" approach - each event finishes before the nested one starts (buggy implementation).

Current behavior in ExpressionHelper is also depth first.

Equality check

Change events require a comparison method for the old and new value. The 2 candidates are reference equality (==) and object equality (Objects#equals). There is some inconsistency in JavaFX about how this equality check is made (it is made in a few places on a few different types). It makes sense to do == with primitive types, and equals with String and the primitive wrappers. For other types, it depends on their characteristics. The "safer" option is == because a change that is triggered by != can then be tested for !oldValue.equals(newValue) in the listener and be vetoed; the opposite is not possible. This might mean that the user will have to give the comparison method that is desired.

Currently, == is used except for String. The behavior is preserved in this change, but will be investigated further in order to allows for more sensible change events.

Just to add here, there are actually two checks involved. When you "set" the value on a property, all properties do a reference equality check (apart from String) to determine whether to fire their listeners. This means that InvalidationListeners always fire in these circumstances. Change listeners however are protected by a 2nd check that is part of ExpressionHelper. This check uses equals for all property types. This means that the behavior of an InvalidationListener + get is sometimes subtly different from using a ChangeListener.

When looking only at change listeners, this behavior makes sense for any type that is either primitive, immutable or mutable without overriding equals. For types that are mutable and override equals, the odd situation can occur that no change fires because the two instances are equal, but that the instance reference did change. When such a type is mutable, any further mutations could be missed. Simple example:

  List a = new ArrayList<>();
  List b = a.clone();
  ObjectProperty<List> prop = new SimpleObjectProperty<>(a);
  ObjectProperty<List> copy = new SimpleObjectProperty<>(a);

  // keep properties in sync:
  prop.addListener((obs, o, n) -> copy.set(n));
  prop.get().equals(copy.get());  // true :-)

  // change first property:
  prop.set(b);   // no change fired, they're equals!
  
  b.add("Hello");

  prop.get().equals(copy.get());  // false, as prop has reference B, while copy has reference A still...

It doesn't happen too often that properties are used with a type that is mutable with its own equals implementation, so this usually works correctly; for cases where you do want to use a mutable type with its own equals in an ObjectProperty though, I think having a variant of ObjectProperty with a reference equality check for its change listeners call may be sufficient.

Performance

Performance both in memory and speed is either equal or slightly worse than the current one. This is because the current behavior is wrong and fixing it entails more complications. In practice, the difference should be small. Registering many listeners on the same observable is not recommended and has caused issues in the past as well. Performance is a WIP and benchmarks will be posted later.

The implementation is able to avoid using a wrapper for single invalidation/change listeners, which improves memory use a bit for the second most common state properties are in (having 1 listener only -- the most common state being having no listeners at all).

As for adding/removing many listeners, I've have changed my stance on this and I don't think we should cater to situations that can have 10.000's of listeners -- even if add/remove performance was much improved, that won't make notifying such high amounts of listeners any better. Having such high amounts of listeners on a single property is a sign that something is wrong with the design, and even if it were to perform reasonably (which it won't due to the sheer amount of listener calls), it would be better to investigate how to avoid adding so many listeners, perhaps by adding a single listener and distributing the requested information more directly (via a shared model for example).

This implementation will have similar performance when it comes to adding/removing listeners as the current implementation. It grows and shrinks the listener list on demand, with the major difference being that when the list is locked (due to an ongoing notification) it will avoid modifying the lists in such a way that indices of current listeners change (removals are nulled out). After the list unlocks, the list is compacted if there were any listener additions/removals, and nulls (and weak listeners) are removed at that time. For this reason it may win out in some cases where listeners are added/removed during notifications, as it does not need to make copies of the listener list, but that is going to be a very rare occurrence.

[nlisker]

I have done some more tests with nested value changes converging and diverging. Overall I've tested over 20 scenarios of nested changes (including addition/removal of listers). Looks good. Great job!

I'll finish going over the implementation with the new changes.

Note that the non-convergence detection logic is pretty smart, as the test case where multiple listeners are trying to agree upon a value that is divisible by 3, 5, 7 and 11 still works fine

And what about non-integer values, like custom objects or strings?

I fail to see how this would make a difference. The test case demonstrates...

I missed that you were talking about a specific test.

---

Do you mind adding somewhere in the top comment a link to #837 for bookkeeping purpose?

[hjohn]

I have done some more tests with nested value changes converging and diverging. Overall I've tested over 20 scenarios of nested changes (including addition/removal of listers). Looks good. Great job!

Thanks, and thank you for putting in so much testing effort for this. Anything I should add as test-cases?

I missed that you were talking about a specific test.

Ah, okay, that explains my confusion as well.

Do you mind adding somewhere in the top comment a link to #837 for bookkeeping purpose?

I added a link. I almost forgot about that implementation which took a different route. I seem to remember there being issues when emissions weren't started nested, but queued up instead.

[nlisker]

Anything I should add as test-cases?

When I do the review of the tests I'll compare with my scenarios and see if there's anything worth adding.

[nlisker]

First part of the review.

There are several class (and their methods) that are public, but are only used in their package and can just have package-access:
OldValueCachingListenerList
ListenerManagerBase
ListenerListBase
ListenerList
ArrayManager

If they are public because of tests, please add a comment like "public for testing purpose".

[nlisker]

I suggest adding a line that says that this class is used by properties.

[hjohn]

That doesn't seem like something you should be adding to the docs I think?

[nlisker]

Up to you. I thought it would make it easier on anyone learning the implementations if they understood why 2 are needed (one that stores the value and one that relies on the value being stored elsewhere).

[hjohn]

I can always put a comment outside the docs. I think the docs (in OldValueCachingListenerManager) are quite clear already though why you'd use one or the other. Note that in an ideal world, we would never need OldValueCachingListenerManager. I mean, it is currently used for properties the exact thing you'd expect to have a readily available old value. However, a big design flaw in properties makes it impossible to do this:

The design flaw is the use of a protected fireValueChangedEvent method within the property, that has an implementation and is not final. This is poor design, because:

• You can't change the implementation, as subclasses may call super.fireValueChangedEvent
• You can't change when you call it, as subclasses may be overriding it to be "aware" of changes
• You can't even provide the default implementation somewhere else, then call fireValueChangedEvent as subclasses may be purposely disabling or altering the default implementation

So, theoretically, I can easily provide the old value for properties, but it means I have to do one of the following:

• Execute the default implementation (but now with old value support) regardless of whether fireValueChangedEvent was overridden -- would break code that overrides this method to block the default implementation
• Change the signature of fireValueChangedEvent to accept a T oldValue -- can't do that, its protected
• Modify the default implementation to fetch the old value from somewhere to provide it -- can't do that as subclasses may be calling it at random, and they won't have the mechanism in place to provide the old value via something other than a parameter

You should never do all three of these for protected methods, as it makes the providing class fragile and tightly coupled with subclasses:

• Provide an implementation in a protected method.
• Make it non-final.
• Call it internally when its implementation is essential for correct operation.

Doing only two of these is fine:

• Providing an implementation and making it overridable is fine if the base class doesn't rely on it (i.e., it's just a helper).
• Providing an implementation that is final and using it internally is fine because subclasses can't alter the base class's behavior.
• Calling a non-final protected method with no expected implementation in the base class is also fine.

If we choose to address this at some point, to make properties more performant (with regards to listeners) and use less memory (when using a single change listener) it would mean a backwards incompatible change. The change will mostly affect FX classes (as they rely on being able to both call fireValueChangedEvent as well as being able to override it) but it may affect 3rd parties as well as they can call and/or override it as well.

[nlisker]

I think the docs (in OldValueCachingListenerManager) are quite clear already though why you'd use one or the other.

Alright, no need to add a note about where it's used.

The design flaw is the use of a protected fireValueChangedEvent method within the property...

The "old value for properties" is interesting, but another discussion. Might be worth bringing it up on the list if there's more to it than just this extra implementation.

You should never do all three of these for protected methods...

Yes, well, same for public and package methods. This falls under "design for inheritance or prevent it" - don't rely on an implementation that can be overridden.

[hjohn]

First part of the review.

There are several class (and their methods) that are public, but are only used in their package and can just have package-access: OldValueCachingListenerList ListenerManagerBase ListenerListBase ListenerList ArrayManager

If they are public because of tests, please add a comment like "public for testing purpose"

I understand that making a method public that shouldn't be should be documented as such, but what's against having public classes in non-exported packages even if just for testing purposes? This is done widely through out FX already.

Also these classes are well documented, and there's no reason they could not be used outside their package, even if they currently are not.

[hjohn]

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... if we did call it anyway, it likely would do the same thing (set value to X) and we'd have an infinite loop...
   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?

[mstr2]

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...)

What if we didn't call back the second listener (as you said, it would probably result in an infinite loop), and logged an error instead (that's not without precedence, bindings also log errors)? It seems like this would still allow other listeners to receive notifications, while the "defective" listener would not.

[hjohn]

What if we didn't call back the second listener (as you said, it would probably result in an infinite loop), and logged an error instead (that's not without precedence, bindings also log errors)? It seems like this would still allow other listeners to receive notifications, while the "defective" listener would not.

I'm fine with logging a warning/error instead. If Nir also agrees, I can make the modifications.

[nlisker]

If we can know that the values are going to diverge then logging an error instead of throwing it is fine. However, if they are trying to find a common divisor (as in John's example above), it could take many callbacks for the final value to settle.

[nlisker]

First part of the review.
There are several class (and their methods) that are public, but are only used in their package and can just have package-access: OldValueCachingListenerList ListenerManagerBase ListenerListBase ListenerList ArrayManager
If they are public because of tests, please add a comment like "public for testing purpose"

I understand that making a method public that shouldn't be should be documented as such, but what's against having public classes in non-exported packages even if just for testing purposes? This is done widely through out FX already.

Also these classes are well documented, and there's no reason they could not be used outside their package, even if they currently are not.

I tend to restrict visibility unless necessary, especially when some classes function as helper classes, but it's fine to leave as is.

[hjohn]

If we can know that the values are going to diverge then logging an error instead of throwing it is fine. However, if they are trying to find a common divisor (as in John's example above), it could take many callbacks for the final value to settle.

I'm unsure what you are saying here. The common divisor example would not log a warning if I place the warning log in the same location as the SOE.

As for the behavior of multiple listeners, we can never really know for certain if they will eventually agree. I mean, even the examples with ExpressionHelper can avoid a SOE if they can change their minds after 100 back-and-forth attempts, and do something else. The same applies for this implementation. If listeners are non-deterministic (same input does not lead to same output, by using randomness for example) then we can never be sure if they will reach agreement.

So, aside from letting this "escalate" into a SOE (where the JVM simply gives up) there is no real way to be sure that the values won't converge at some point. The check I added is making the assumption that listeners are deterministic, but they don't have to be. Then again, we're again approaching super rare exotic edge cases, that ExpressionHelper is not handling either (if it is even possible to handle this). A non-deterministic listener that also modifies the value of the very property it is monitoring is not something we should have to worry about.

[nlisker]

I'm unsure what you are saying here. The common divisor example would not log a warning if I place the warning log in the same location as the SOE.

If you replace the thrown error/exception with logging then the code path will continue. What behavior will we get instead? The one before the modification where "first listener wins"?

[hjohn]

I'm unsure what you are saying here. The common divisor example would not log a warning if I place the warning log in the same location as the SOE.

If you replace the thrown error/exception with logging then the code path will continue. What behavior will we get instead? The one before the modification where "first listener wins"?

Yeah, if there's disagreement, then the listener earliest in the chain (if it is persistent) will win, and one of the disagreeing listeners (after setting a value) will not have been notified of that change (as it was changed back). Note that there's no inconsistency here; after all notification loops complete, the last received new value will match the current value of the property. This goes for all listeners, regardless of disagreements. In other words:

     (obs, ov, nv) - {
           // new value received here is ALWAYS going to be correct after the full notification completes

           // There is no guarantee that after calling set(X) that the value actually will become X (there
           // never was such a guarantee, also not with ExpressionHelper as other listeners can interfere
           // at any time).  So if you didn't receive a new value X, it hasn't actually become X.
           property.set(X);  
           
           // Calling this afterwards could allow you to see that your change was "rejected"
           if (property.get()  != X) {}
     }

Realistically, there's not really that much wrong with this. It is just that listeners should not make assumptions that a value passed to property.set will become the final state. The only way to be sure is to have received a call back with that value as the new
value.

[nlisker]

Finished the 2nd part of the implementation review. I didn't delve into the logic of the listener management, but it looks sane :)

I'll review the tests as the final part.