When a ChangeListener modifies its associated property during its callback, other listeners on the same property may receive incorrect or plain misleading events, depending on the call order (registration order) of the listeners in relation to the listener performing a nested change.
Nested changes may at first glance seem to be bad practice, but with many interconnected properties (for example, layout properties) avoiding (indirect) nested changes will be hard if not impossible to eliminate completely. The practice is discouraged, but not prevented, and it is widely used in JavaFX itself
An example that demonstrates current behavior when a nested change occurs:
IntegerProperty property = new SimpleIntegerProperty(9);
property.addListener((obs, o, n) -> System.out.println("1: " + o + " -> " + n));
property.addListener((obs, o, n) -> {
System.out.println("2: " + o + " -> " + n);
if(n.intValue() > 9) {
property.set(9);
}
});
property.addListener((obs, o, n) -> System.out.println("3: " + o + " -> " + n));
property.set(10);
Outputs:
1: 9 -> 10
2: 9 -> 10
1: 10 -> 9
2: 10 -> 9
3: 10 -> 9
3: 9 -> 9
From the perspective of listener 3, the value, which was initially 9, changes from 10 to 9, then from 9 to 9.
Even though the final value was correct, the event is not consistent with what actually happened. Only listener 1 and 2 received events that accurately reflected the changes to the property. If listener 3 had cached its own old value, it would not be consistent with the event. In fact, it may sometimes be necessary to do this to work around this behavior.
This can have wide ranging implications for code built on top of this basic system, from subtle bugs which are getting "fixed" in the wrong location, or are fixed by triggering yet more events to get the system consistent. The old value provided as it is, is currently not very reliable and doing any kind of computation with it (like a diff) would be incorrect if there ever was a nested change.
JavaFX itself uses nested changes in some of its classes, SpinnerValueFactory is one such class. This behavior with incorrect old values can be observed there. It is however likely (given that nested changes may be hard to avoid in complex interconnected systems) there are many more locations.
Expected:
The output for the above program should accurately reflect the actual changes made to the property, regardless of the order of listeners. One such valid output could be:
1: 9 -> 10
2: 9 -> 10
3: 9 -> 10
1: 10 -> 9
2: 10 -> 9
3: 10 -> 9
Also valid would be:
1: 9 -> 10
2: 9 -> 10
1: 10 -> 9
2: 10 -> 9
3: 9 -> 10
3: 10 -> 9
Dubious, not reflecting what occurred, but at least consistent could be:
1: 9 -> 10
2: 9 -> 10
1: 10 -> 9
2: 10 -> 9
(no events for listener 3)
The last example however again shows that order of registration could have an effect on the events received.
If listeners always receive the same events irrespective of their registration order, then order becomes irrelevant enabling potential future optimizations with regards to listener list management.
Tests done:
A preliminary exploration shows that fixing this issue breaks no tests in JavaFX, nor is it in conflict with current documentation. From the perspective of ChangeListener, this is an actual bug fix, as the presented old and new value combination is not what one would expect when implementing such a listener.
Nested changes may at first glance seem to be bad practice, but with many interconnected properties (for example, layout properties) avoiding (indirect) nested changes will be hard if not impossible to eliminate completely. The practice is discouraged, but not prevented, and it is widely used in JavaFX itself
An example that demonstrates current behavior when a nested change occurs:
IntegerProperty property = new SimpleIntegerProperty(9);
property.addListener((obs, o, n) -> System.out.println("1: " + o + " -> " + n));
property.addListener((obs, o, n) -> {
System.out.println("2: " + o + " -> " + n);
if(n.intValue() > 9) {
property.set(9);
}
});
property.addListener((obs, o, n) -> System.out.println("3: " + o + " -> " + n));
property.set(10);
Outputs:
1: 9 -> 10
2: 9 -> 10
1: 10 -> 9
2: 10 -> 9
3: 10 -> 9
3: 9 -> 9
From the perspective of listener 3, the value, which was initially 9, changes from 10 to 9, then from 9 to 9.
Even though the final value was correct, the event is not consistent with what actually happened. Only listener 1 and 2 received events that accurately reflected the changes to the property. If listener 3 had cached its own old value, it would not be consistent with the event. In fact, it may sometimes be necessary to do this to work around this behavior.
This can have wide ranging implications for code built on top of this basic system, from subtle bugs which are getting "fixed" in the wrong location, or are fixed by triggering yet more events to get the system consistent. The old value provided as it is, is currently not very reliable and doing any kind of computation with it (like a diff) would be incorrect if there ever was a nested change.
JavaFX itself uses nested changes in some of its classes, SpinnerValueFactory is one such class. This behavior with incorrect old values can be observed there. It is however likely (given that nested changes may be hard to avoid in complex interconnected systems) there are many more locations.
Expected:
The output for the above program should accurately reflect the actual changes made to the property, regardless of the order of listeners. One such valid output could be:
1: 9 -> 10
2: 9 -> 10
3: 9 -> 10
1: 10 -> 9
2: 10 -> 9
3: 10 -> 9
Also valid would be:
1: 9 -> 10
2: 9 -> 10
1: 10 -> 9
2: 10 -> 9
3: 9 -> 10
3: 10 -> 9
Dubious, not reflecting what occurred, but at least consistent could be:
1: 9 -> 10
2: 9 -> 10
1: 10 -> 9
2: 10 -> 9
(no events for listener 3)
The last example however again shows that order of registration could have an effect on the events received.
If listeners always receive the same events irrespective of their registration order, then order becomes irrelevant enabling potential future optimizations with regards to listener list management.
Tests done:
A preliminary exploration shows that fixing this issue breaks no tests in JavaFX, nor is it in conflict with current documentation. From the perspective of ChangeListener, this is an actual bug fix, as the presented old and new value combination is not what one would expect when implementing such a listener.
- links to
-
Review openjdk/jfx/837
-
Review(master) openjdk/jfx/1081