Listeners behavior discussion

[Nir Lisker]

There was a previous implementation discussion that touched on some of the
points here in https://github.com/openjdk/jfx/pull/108

I think there are a few (semi-)independent points here:
1. Order of listeners: as John said, there is no specification of the order
of listeners, but the add/removeListener methods imply that there is some
sort of order by saying that the "first occurrence" will be removed.
2. Dispatch order: the docs don't specify (from what I could find) the
order in which listeners receive events. That is, if listener1 and
listener2 are registered in that order, I didn't find anything that
specifies that they will be notified in this order, although It makes sense
that they are. From the API perspective, there is no `getListeners()`
method or the like that allows me to see the order, so I have to "remember"
that order somehow, and that's an indication that the order is actually not
important.
Furthermore, the order is separate for invalidation listeners and change
listeners: first all invalidation listeners are notified, then all change
listeners, even if all the change listeners were registered before. This
alone can be surprising.
3. Concurrent modification of listeners: if a listener is added or removed
from another thread (while an event is being sent through the listener list
or otherwise), what should happen.
4. Nested events (relevant only for change listeners I think): if a value
is changed inside a listener, what should happen. The docs in
ChangeListener#changed mention that it is "bad practice", but don't
prohibit it.

I have some thoughts, but I haven't constructed an opinion yet:
1+2. It should be tested how much practical maneuvering room we have here
first. If we don't keep an ordered collection of listeners, or don't
dispatch the event to them in that order, or even if we dispatch first to
all change listeners and then to all invalidation listeners, what breaks?
This will give us an idea of our options.
3. I agree with Michael that as long as there is no mention of thread
safety in any of the relevant classes (observables and listeners), the
behavior should be unspecified, exactly as it is in core JDK classes that
don't specify anything. We can revisit this when the GC part of the story
is on the move, as John said.
4. I don't think we can restrict nested changes at this point considering
the docs, and I remember some classes in JavaFX doing this (I think it was
in the animation package). However, I propose to look at another approach:
The event dispatcher will use an iterator, and whatever changes happen
during iteration will reflect on it (if they are relevant). It is not
guaranteed that all listeners that were registered *at the time of the
event* will receive it, but only those that are registered in time for a
dispatch. It removes the hurdles of locking or maintaining a queue. Also,
if I have a listener1 whose job is to remove a listener2 when an event is
received, it's very probable that I don't want listener2 to react to the
event (unless it wasn't registered before listener1, in which case "too
late", but see also points 1+2).
If this approach is viable, it should also be tested practically.

On Sun, Jul 10, 2022 at 4:55 PM Michael Strauß <michaelstrau2 at gmail.com>
wrote:

> 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
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/openjfx-dev/attachments/20220710/567606a6/attachment-0001.htm>