Your explanation for why it's okay for a ChangeListener to get() a different value than the reported new value is quite convincing. We'd only need to clearly document that a change event should only ever be interpreted as a historical fact, not a description of current reality.
Of course, there are surely many cases where a ChangeListener directly or indirectly acts on the current state of the world, erroneously assuming that historical fact == current reality. Can you think of scenarios where this would constitute a bug, and if so, should we have our property implementations have a flag to choose whether nested changes are allowed? On the other hand, considering that writing code that recursively changes a value is very suspicious in almost all cases, maybe disallowing it entirely and having something like a "setLater" mechanism to break a cycle would be a solution that doesn't invite strange bugs. In this case, change notifications would always correspond exactly to the current state of the world. I have more thoughts on your implementation ideas, but it's probably best to hold back that discussion until there's agreement on the core concepts. On Sun, Jul 10, 2022 at 3:06 PM John Hendrikx <[email protected]> wrote: > > Hey, thanks for commenting, I have added my comments inline. > > > From what I understand, you are essentially suggesting to solve an > > existing bug in ExpressionHelper, where inconsistent change > > notifications can be produced when a listener modifies its > > ObservableValue. > Well, and the fact that it will prematurely release the lock when a > nested emission finishes (and there is another bug in there which I > didn't mention in my previous post, while the lock is held, each change > of the listener list as part of the same emission triggers another > unnecessary copy). > > At first, it seems like a good idea to defer a nested change > > notification until after all listeners have been invoked for the > > current change. But upon closer inspection, even with the proposed > > fix, there are more problems: > > Let's consider three listeners A, B, and C that are invoked in this > > order. If B changes the value, then C would seem to receive a correct > > change notification. However, if C tried to get() the value, it would > > not see the value that's reported in the change notification, but the > > value that B set. That's not good. > > I think relying on #get when you're using a ChangeListener is a bit odd > (since we're allowing threading, #get could report anything anyway). For > an InvalidationListener it wouldn't matter, since it's allowed to get 2 > invalidations where nothing changed. What I think is not good is that > the ChangeListener is receiving events that never happened, happened in > a different order or events where the current value didn't actually > change. This is not documented anywhere: "the old value can be > incorrect, don't rely on it" or "the change the event reports may never > have actually happened" or "you may receive two changes with no change > in the current value reported". All I would like to see is to formalize > this somewhat, so we know what the implementation *should* be doing, > instead of having to guess at what is a bug and what is intended. > > Also, I'm like 99% sure nested changes are already happening inside > JavaFX code itself, for example, during layout. The more complicated > layouts can take a few passes to "settle", and even though there may not > be a direct change of the same property, it can also be triggered by > changing another property which in turn changes the original property, > all occurring during the first emission still of the original property. > > I can do an experiment where I disallow this (by throwing an exception > if the list is already locked), but I'm pretty sure that tests are going > to fail. > > > We might be tempted to just add a similar deferred update mechanism to > > the value itself, so that C can still get() the old value even after B > > already set a new value. But that would create another problem: if the > > value is only applied after all listeners have run, B could observe > > that get() == "foo", then call set("bar"), and after that be > > astonished to learn that get() == "foo" (because the value is not yet > > applied). > Agreed, I don't think this would work at all, especially not for > invalidation listeners. > > I don't think there is a way to support nested value changes, deliver > > the same events to all listeners, and be consistent at the same time. > > Something has to give. The easiest option, quite obviously, is to > > disallow nested value changes entirely. > > I think what I described still stands -- it's not an uncommon problem in > event listening frameworks. The only argument against it at the moment > is that a direct #get could report something that is not the same as the > current value, which I think is not really a concern. You want to act on > the *change*, a historical event that should reflect what actually > happened so you can look at the old value and the new value and perhaps > even derive something from those two (like the difference). A > ChangeListener might even have logic to ignore the current change if it > notices that #get reports something different as another change will > follow soon after (guaranteed by the framework). > > > Another option may be the following algorithm: > > 1. When B changes the value, the scheduled change notification for C > > is updated to reflect the new value (but it still needs to retain the > > old value from the perspective of C). > > 2. After A, B and C have run, C is already up to date with the newest > > value. In the deferred phase, only A and B receive additional change > > notifications for the new value. > > It's not a bad idea, but I think it would be even harder to implement. > The problem here is that the listener list can also be modified during > emission, and so it would be very hard to track which listeners were > notified already, which weren't notified yet, etc. > > It also removes some of the guarantees that a listener should receive > all change events as they occurred (if we want to guarantee that). > Basically, the order of listeners becomes a serious influence here, > something that I think should be avoided. I mean, we are making an > effort to notify all *original* listeners that were part of the list, > but then are not concerned with giving all of them the same event > information? > > > As for your proposed specification: ExpressionHelper is an > > implementation detail and not public API. As far as I know, we don't > > specify the invocation order of listeners, and I see no compelling > > reason why we should. Listeners should not depend on their order of > > invocation with respect to other listeners. Of course, users can > > currently depend on that (because it's implemented as such), but we > > probably shouldn't invite them to write even more of such code. > > I may not have been clear, I'm not proposing to make ExpressionHelper > public. However, since it is used in the majority of our listener > registration and notification system, and there is no formal > specification as to how listeners should work in the face of concurrent > changes, nested changes and listener add/removal, the only place to find > what the system is currently doing is in ExpressionHelper. > Unfortunately, what ExpressionHelper is currently doing would not make > for a very good specification that users can rely and build complicated > logic on. Primitives which are hard to predict or are just plain > misrepresenting events are not a solid foundation to build upon. > > I know that currently we don't specific invocation order, but this is > another thing that probably would break JavaFX if changed. It is sort of > implied though; adding duplicate listeners is allowed, which will be > notified twice, and the docs mention that removal would remove the first > instance, implying there is somekind of ordering going on. The docs > could have stated that duplicates would only be notified once and that > removal would remove all matches, but it doesn't. > > I'll see what happens when I swap out ExpressionHelper for one that > ignores order and disallowes nested changes (one at a time) :) > > > Similarly, I see no compelling reason to include language about > > concurrency in ObservableValue's specification. ObservableValue > > doesn't have any thread-safety guarantee at all; any attempt to use it > > from multiple threads should be assumed to potentially result in > > corrupted state. > > I'm not that concerned with this part, although in light of perhaps > having background cleaning of listener stubs, it could be worthwhile to > just specify it. If JavaFX itself has a need to do background listener > removal, then so could users. All it requires is synchronization per > property while events are emitted. > > --John >
