(draf): ChangeListener events are incorrect or misleading when a nested change occurs #837
Conversation
|
👋 Welcome back jhendrikx! A progress list of the required criteria for merging this PR into |
modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/skin/WebColorFieldSkin.java
Outdated
Show resolved
Hide resolved
Webrevs
|
|
/reviewers 2 |
|
@kevinrushforth |
There was a problem hiding this comment.
I've re-read the discussion on the mailing list. The approach as implemented in this PR is a good solution, as it has a very low implementation and runtime overhead, and gets us to having consistent histories for change listeners.
|
@hjohn this pull request can not be integrated into git checkout feature/delayed-nested-emission
git fetch https://git.openjdk.org/jfx master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push |
openjdk
bot
added
the
merge-conflict
…ayed-nested-emission
openjdk
bot
removed
the
merge-conflict
|
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). 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)). I will re-summarize what I think needs to be defined before implementing a behavior change like the one proposed here: Listener orderWhen 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? Event dispatch orderThe 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. 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. Nested listener modificationsIf 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 modificationsIf 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? 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). |
modules/javafx.base/src/test/java/test/javafx/beans/ObservableValueTest.java
Show resolved
Hide resolved
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
I don't think this is a behavior change, it is a bug fix, unless we document that
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 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:
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).
I'll consider this the same problem, with the same risks, see above.
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.
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
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:
But not:
This PR does not make listeners more/less dependent than they are. It fixes one major bug that can cause 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:
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. |
…ayed-nested-emission
There was a problem hiding this comment.
I did test this well and looks good to me. I tested changing the property value in more than one listener and combination of Invalidation and Change Listener. The behavior looks good and did not seem like it will cause regression(of course depends on specific scenarios)
I think the change may need a doc update, to explain this behavior.
Coming to the broad level of discussion, I think this change can still go in and other aspects can be worked on later whenever we get to them.
modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/skin/WebColorFieldSkin.java
Outdated
Show resolved
Hide resolved
|
@arapte and others
Did you have a specific place in mind for a documentation update? I just re-read the documentation for
Note that JavaFX itself does this sometimes to restore values if they don't match requirements. It's a bit of a tough requirement. If you do any work in a Perhaps it could be reworded to:
I added it in already. If there are any other places where you think documentation should be update, please let me know. |
I'll be more concrete. Here is my test program: public class ListenersTest {
private static int inv = 0;
public static void main(String[] args) {
with1Change();
}
static void with1Change() {
inv = 0;
var property = new SimpleIntegerProperty(0);
ChangeListener<? super Number> listenerA = (obs, ov, nv) -> {
inv++;
String spaces = IntStream.range(1, inv).mapToObj(i -> " ").reduce(String::concat).orElse("") + inv;
System.out.println(spaces + " bA " + ov + "->" + nv + " (" + property.get() + ")");
property.set(5);
System.out.println(spaces + " aA " + ov + "->" + nv + " (" + property.get() + ")");
};
property.addListener(listenerA);
ChangeListener<? super Number> listenerB = (obs, ov, nv) -> {
inv++;
String spaces = IntStream.range(1, inv).mapToObj(i -> " ").reduce(String::concat).orElse("") + inv;
System.out.println(spaces + " bB " + ov + "->" + nv + " (" + property.get() + ")");
System.out.println(spaces + " aB " + ov + "->" + nv + " (" + property.get() + ")");
};
property.addListener(listenerB);
property.set(1);
System.out.println("---------\n");
}
}With the patch: With your patch, each event finishes its run and only then the next event happens. This is the "breadth-first" approach. This approach starts events before the previous ones finished, and goes back to the original event later. This is the "depth-first" approach. I don't think that either is wrong. This one makes sense and it's a behavior I can reason about: the listener is loyal to the event at the time it happened (and the "real" value is accessible with Without the patch: I agree that at step 4 the 0->5 event is wrong because the events are only 0->1 and 1->5. If you comment out the line while with delaying nested events I would expect: So this looks inconsistent to me. The fix for the lock being released is good regardless, it's the behavioral change that I'm not sold on. |
| * In general, it is considered bad practice to modify the observed value in | ||
| * this method. | ||
| * Changing the observed value in this method will result in all listeners | ||
| * being notified of this latest change after the initial change | ||
| * notification (with the original old and values) has completed. | ||
| * The listeners that still needed to be notified may see a new value that | ||
| * differs from a call to {@link ObservableValue#getValue}. All listeners are | ||
| * then notified again with an old value equal to the initial new value, | ||
| * and a new value with the latest value. |
There was a problem hiding this comment.
This seems like a right place.
It is still a bad practice to change the value in this method, so I would recommend to keep the warning.
I did little rewording, please check how does this look:
* In general, it is considered a bad practice to modify the observed value in
* this method. But if still modified, then the notifications for this change
* are deferred until all the listeners are notified of current change.
* This results in a minor inconsistency of new value with the listeners
* that are still pending to be notified of current change that the listeners
* would receive a new value that differs from a call to {@link ObservableValue#getValue}.
Old text was uppercased while new text is always lowercase.
I should have said "this one makes sense too". The point was that while your fix is good, it's not the only good fix, and I wasn't sold on choosing the one in this PR just yet. I'm not at all sure that we need so specify this behavior, but we need to be consistent with it. @arapte Why do you think we should say what order the nested event is dispatched at? I'm also investigating the effect of this patch in conjunction with addition/removal of listeners in conjunction with a nested event. |
Confusing me again here :-) Did you mean to say "breadth-first" where you said "depth-first" ? Breadth first is for sure a lot easier, as the old values are much easier to get correct for it. I've given depth first some more thought, but every approach I think of requires some kind of stack or tracking of which listeners weren't notified yet of the original set that we wanted to notify (and I think we'll need to remember even more if there is another change before the first and second one finishes).
I've been wondering how often it happens -- I may be able to put a breakpoint or println in the nested code and see if this happens in a large FX application (in layout code perhaps).
Perhaps we could limit the explanation to just mentioning that its possible the "new value" may not be the same as
Yes, perhaps this may require an additional unit test. @nlisker if we're going for breadth-first, shall I fix the cases for single listeners as well (finish the notification before calling the same listener again, instead of recursively calling into it?) |
Yes, sorry, I meant breadth-first. We can go with that one.
I think that we need to be consistent, so barring any special reason for excepting single listeners from this change, I would say that we need to bring them in line with the generic listener.
I'll think about what doc changes to make. We'll see what Ambarish thinks too. |
|
I tried this test: static void withRemovalAnd2Changes() {
inv = 0;
var property = new SimpleIntegerProperty(0);
ChangeListener<? super Number> listenerA = (obs, ov, nv) -> {
inv++;
String spaces = spaces();
System.out.println(spaces + " bA " + ov + "->" + nv + " (" + property.get() + ")");
property.set(5);
System.out.println(spaces + " aA " + ov + "->" + nv + " (" + property.get() + ")");
};
ChangeListener<? super Number> listenerB = (obs, ov, nv) -> {
inv++;
String spaces = spaces();
System.out.println(spaces + " bB " + ov + "->" + nv + " (" + property.get() + ")");
property.removeListener(listenerA);
property.set(6);
System.out.println(spaces + " aB " + ov + "->" + nv + " (" + property.get() + ")");
};
property.addListener(listenerA);
property.addListener(listenerB);
property.set(1);
}which removes a listener in a nested event. With this patch I got the following: Because listener A is removed in B, we hit the inconsistency with 1 listener again. I need to work through this, but I'm not convinced this is what the output should be. One glaring question is, are nested listener additions/removals also deferred, or do they take effect immediately, in which case they shouldn't receive deferred nested events I think. |
|
With this test static void with2Changes() {
inv = 0;
var property = new SimpleIntegerProperty(0);
ChangeListener<? super Number> listenerA = (obs, ov, nv) -> {
inv++;
String spaces = spaces();
System.out.println(spaces + " bA " + ov + "->" + nv + " (" + property.get() + ")");
property.set(5);
System.out.println(spaces + " aA " + ov + "->" + nv + " (" + property.get() + ")");
};
ChangeListener<? super Number> listenerB = (obs, ov, nv) -> {
inv++;
String spaces = spaces();
System.out.println(spaces + " bB " + ov + "->" + nv + " (" + property.get() + ")");
property.set(6);
System.out.println(spaces + " aB " + ov + "->" + nv + " (" + property.get() + ")");
};
property.addListener(listenerA);
property.addListener(listenerB);
property.set(1);
}I get I think that we are missing a 1->5 event originating in listener A, and maybe a 5->6 event. I'm honestly not sure what the behavior should be here. |
If you add the missing events, this would be an infinite loop, so that's probably not the best way to go. I think you really need to look at the emissions as a whole, and then this doesn't look too bad. First, the indents and counts you use are a bit confusing. The first emission is: As changes occurred during the first emission, another is done afterwards. The changes are collapsed; the value is only re-evaluated after a full emission. I think that's probably the best thing we could do (or you'd get an infinite loop). The second emission is: The third emission is empty, as there is a I think it would be best to note that if there are multiple change listeners modifying the values, that it is up to the user to ensure they're not conflicting. Even though this case doesn't result in an infinite loop, it could easily have been (and under the old code it actually results in |
The logic only has three states, but using two booleans for this allows for four states.
|
(about removing listener during event emission)
I've fixed the 1 listener case so it doesn't do the call recursively. I'll now take a look at listener addition/removal part. I think that a follow up emission triggered by a nested change should use the latest set of listeners and not continue using the initial set (that's how the old code behaved). |
|
I see that the same unit test has failed on all three platforms after your latest change. |
|
Yes, that's correct (well not correct). The cause is that this new notification variant collapses nested changes into a single change (by not recursively calling into the same listener). After I applied the same treatment to the single listener variant in The reason it breaks is that it makes multiple changes while within a So it seems that there is at least some JavaFX code that relies on the behavior that even nested changes should result in separate change notifications. I'm now investigating possible solutions to this; as far as I can see, there are three options:
I'm currently looking into the first option to see if it can be done depth-first without having to do extensive tracking of which listeners were notified already and which didn't get any yet, to keep old values correct. |
|
I've included a fix which prevents nested changes from being collapsed into a single change. This should fix the failing test in There is still two open issues that I'm aware of, and which I'm looking into:
|
|
I've also come to the conclusion that a depth first approach is not really feasible unless you either allow for:
Given three listeners, A, B, C where B changes values to even values, it's really hard to give the last listeners in the chain (C) correct values. The implementation before this PR does: A correct depth first approach would have to avoid sending out The only option I still see that may work is to do this: For comparison, this PR does: |
|
@hjohn This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
|
@nlisker @mstr2 I'm going to update this soon with an (IMHO) much better solution, that handles all the edge cases.
The implementation has the following characteristics:
Here's a sample output with 5 listeners, where 0, 2 and 4 just register changes, and 1 uppercases a String and 3 will ensure the String contains 2 characters: As you can see, all changes make sense and the final listener only receives the full change. |
|
Converted this one to draft, I no longer think this is the way to go. See instead for a depth-first approach this PR: #1081 I think the new approach is much more solid, makes more sense, and is much better at avoiding unnecessary change listener calls. It's however a lot of new code... |
hjohn
changed the title
|
@hjohn This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
openjdk
bot
added
the
merge-conflict
|
@hjohn This pull request has been inactive for more than 16 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the |
This contains the following:
ConcurrentModificationExceptionif a nested change was combined with a remove/add listener callWebColorFieldSkinwhich triggered a nested change which used a flag to prevent an event loop (I've changed it now to match howDoubleFieldSkinandIntegerFieldSkindo itProgress
Integration blockers
issue number:message. (failed with the updated jcheck configuration)Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/837/head:pull/837$ git checkout pull/837Update a local copy of the PR:
$ git checkout pull/837$ git pull https://git.openjdk.org/jfx.git pull/837/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 837View PR using the GUI difftool:
$ git pr show -t 837Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/837.diff
Webrev
Link to Webrev Comment