Listeners behavior discussion

[John Hendrikx]

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