Listeners behavior discussion

[Michael Strauß]

>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.

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.

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).

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.

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.

This prevents basic inconsistencies with regards to the value being
observed, but may introduce higher-order inconsistencies where
different listeners for the same value can observe different change
histories.

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.

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.


On Sat, Jul 9, 2022 at 1:02 PM John Hendrikx <john.hendrikx at gmail.com> wrote:
>
> Hi all,
>
> I'm looking into ExpressionHelper as part of my evaluation of adding
> Cleaner's to clean up weak listeners stubs a bit more eagerly.
>
> I would like to hear your thoughts on the following:
>
> ExpressionHelper has a locking mechanism so that when an event occurs,
> it is fired to the listeners which were active at the moment the event
> was triggered. This is achieved by making a copy of the listeners *if*
> the list is modified while an event emission is in progress. This uses a
> simple boolean for the lock ("locked").
>
> It is however conceivable (although perhaps not good practice) to change
> the value again while the event emission is occurring. An easy example I
> can think of is that when a text field is modified, a ChangeListener may
> change the value to uppercase if the field should only contain uppercase
> characters.  This will trigger a nested event emission for the same
> property.  JavaFX does not prohibit this (and probably can't anymore for
> backwards compatibility reasons).  There are several problems with this:
>
> 1) The locking mechanism does not take nesting into account, therefore
> when the 2nd nested emission completes, the listener lists are unlocked,
> and the first event emission will continue with the lists unlocked. If
> there is any modification after the 2nd emission completes, listeners
> may be skipped, notified twice or an IndexOutOfBoundsException may occur.
>
> 2) The events received by the listeners are inconsistent depending on
> where you are in relation to the listener that made the nested change.
> Let's say there are three listeners, A, B and C in that order.  Listener
> B will change any uneven number to the next higher even number.  The
> events seen by the three listeners when the value changes from 2 to 3 are:
>
> A: 2 -> 3; 3 -> 4
> B: 2 -> 3; 3 -> 4
> C: 3 -> 4; 2 -> 4
>
> IMHO, we should strive to have the exact same events for all three
> listeners: 2 -> 3; 3 -> 4 or prohibit this with an exception.
>
> I think it is possible to change this so all listeners receive the exact
> same events; this would be achieved by delaying the 2nd event emission
> until the first has finished.
>
> I think the specification for how invalidation/change listener event
> notification works should perhaps be documented somewhere. I think it
> should be:
>
> When an event occurs, the currently list of registered listeners will
> receive this event in the order they were registered, regardless of
> whether the list is modified during the event emission. If during
> emission, another event occurs for the same property, it will be queued
> and emitted immediately after the current emission completes. This
> ensures all registered listeners receive the same events irrespective of
> registration order.  Event emission is serialized, that is to say, no
> two emissions for the same property will ever run concurrently.
>
> =====
>
> Implementation ideas
>
> If we can all agree on the above specification, I think an
> implementation can be created that never needs to copy a listener list.
> The main trick to get this correct is in how events are emitted. First,
> when a nested emission occurs (easily detected by the fact that the list
> is locked), the change for the nested emission is buffered.  The nested
> emission is then triggered as soon as the first finishes (when the first
> emission is about to unlock, it checks for a nested emission, and does
> another emission if one is present, and so on).  If there are multiple
> emissions after an emission finishes, they probably can and should be
> rolled into a single one.  Note that nested emissions can easily go
> wrong (two listeners that change values in conflicting ways) and cause
> an infinite loop -- we're not looking to prevent these kinds of bugs
> here, although we could set a limit before "giving up".
>
> Secondly, while the lists are locked, any additions or removals to the
> list should be buffered. Just before the list is unlocked (and before a
> second emission occurs), all changes are applied. This has the
> additional bonus that any mass changes to the listener list (for example
> when a large number of stubs were determined to require clean up) can
> use mass change functions like #removeAll and #addAll, potentially
> saving a lot of copying of the listener list, on top of the copy that is
> already avoided due to the lock.
>
> The fireValueChangedEvent method should be synchronized, so only the
> same thread could potentially trigger a nested emission.  I think we
> should steer clear from allowing multiple threads to emit events for the
> same property concurrently as this will make the events received
> unpredictable and probably even incorrect (the last change received may
> not reflect the actual current value!).
>
> Thanks for reading, I look forward to hearing your thoughts!
>
> --John
>
>