Listeners behavior discussion

[Michael Strauß]

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 <john.hendrikx at gmail.com> 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
>