Listeners behavior discussion

[Michael Strauß]

1. I don't understand your point about concurrency. The only way you
end up in ExpressionHelper.fireValueChangedEvent is through the
*PropertyBase and binding implementations, none of which are specified
to support concurrent access. If you call set(...) in anything other
than a strictly single-threaded context, you're relying on unspecified
behavior (in this case, you're actually pretty likely to corrupt
property state). There's no need to even consider concurrent access
*except* when it is only an implementation detail that cannot be
observed from the outside.

Note that even if we did specify that event emission may not happen
concurrently (and enforced that by making fireValueChangedEvent a
synchronized method), that still would not make it acceptable to
modify or access a property from multiple threads. Property
implementations would need to be explicitly designed for that, which
they are not (and should not be, in my opinion).

2. Listener invocation order: I don't think we can specify this for
ObservableValue, as it could render many existing third-party
implementations defective. We could, however, guarantee an ordering
for the *PropertyBase implementations. But then our story for
ObservableValue would be: "You can't rely on any particular order,
except when the ObservableValue is a PropertyBase, then you can. But
be aware that your code may silently break if someone changes the
implementation of your ObservableValue to something other than
PropertyBase."
That doesn't sound like a great story. I'd much rather we fix the code
that depends on a particular order of listener invocations.

3. Adding and removing listeners: The specification is pretty simple
for the normal case. If you add a listener, you'll get a change
notification for the next change. If you remove a listener, you will
not get a change notification for the next change. Problems arise when
a listener is added or removed during a change notification. I see no
great reason to guarantee that additions or removals of listeners
during a change will have any particular effect for the current
change. It should be expressly unspecified whether an added or removed
listener is invoked during a change.
First of all, I think that adding or removing listeners during a
change notification is pretty dubious to begin with, and we shouldn't
encourage applications to do that. But what's more important is that
such a guarantee severely limits the design space for ObservableValue
implementations for an extremely dubious benefit. The cost vs benefit
ratio seems pretty off.

Here's an example: if we leave this unspecified, we have the option of
using lock-free listener lists that support removals on a background
thread (ConcurrentLinkedQueue is such an implementation). That's a
huge benefit that, in my opinion, far outweighs the usefulness of
having applications be able to rely on weird nested code.

On Mon, Jul 11, 2022 at 8:09 AM John Hendrikx <john.hendrikx at gmail.com> wrote:
>
> I've done some preliminary testing on the limits of what's possible.
>
> Disallowing nested changes broke about 50 tests. Changing the order of
> listener events broke 9 tests (I reversed the order, see this diff for
> what I did:
> https://github.com/hjohn/jfx/commit/c7588c49d42257f75627f15ca7ed01147d5e5e3d).
>
> I investigated the tests that broke when the ordering changed. They're
> all Tree/Table/ListView tests. The tests breaking all seem to interact
> with the selection and focus model, so there might be some kind of
> ordering dependency in there (list of the 9 tests that broke added at
> end of this post).
>
> Disallowing nested emissions broke far more, Spinner, ComboBox,
> ColorPicker, Scroll related things, TableCell.  I didn't run systemTests
> or web tests, so there might be even more that breaks.
>
> Further comments inline:
>
> On 10/07/2022 20:11, Nir Lisker wrote:
> > 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.
>
> Thanks, good summary there.
>
> > 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.
> >
> As for the last point, I don't think we should change a current event
> emission. When an event occurs, I think every one that was registered
> should receive it (which is currently ensured by making a copy). Doing
> it differently (allowing the list to change during emission of a single
> event) would actually introduce ordering effects and would be a much
> harder behavior to document.
>
> As for it removing hurdles, I think a copy free approach is already
> possible *if* we specify that changes are historical (ie. everyone at
> the time of registration receives the exact same change event,
> regardless of nested value changes or nested listener list changes).
>
> If changes are historical, the implementation probably must delay nested
> value changes to adhere to the specification.
>
> If we further specify (or implement in this case) that for a single
> property, no more than a single emission can run concurrently (this
> seems reasonable IMHO, especially given the fact that concurrent
> emissions probably hardly happen currently as it would break too much
> things in unexpected ways), then the implementation can buffer any list
> modifications made.
>
> All the above taken together will allow a copy free use of listener
> list. A lock is still needed to detect a nested emission, when to buffer
> changes and to prevent emissions on another thread.
>
> --John
>
>
> ListViewTest > ensureSelectionIsCorrectWhenItemsChange FAILED
>      java.lang.AssertionError: expected:<0> but was:<-1>
>          at org.junit.Assert.fail(Assert.java:89)
>          at org.junit.Assert.failNotEquals(Assert.java:835)
>          at org.junit.Assert.assertEquals(Assert.java:647)
>          at org.junit.Assert.assertEquals(Assert.java:633)
>          at
> test.javafx.scene.control.ListViewTest.ensureSelectionIsCorrectWhenItemsChange(ListViewTest.java:337)
>
> TableViewTest > ensureSelectionIsCorrectWhenItemsChange FAILED
>      java.lang.AssertionError: expected:<0> but was:<-1>
>          at org.junit.Assert.fail(Assert.java:89)
>          at org.junit.Assert.failNotEquals(Assert.java:835)
>          at org.junit.Assert.assertEquals(Assert.java:647)
>          at org.junit.Assert.assertEquals(Assert.java:633)
>          at
> test.javafx.scene.control.TableViewTest.ensureSelectionIsCorrectWhenItemsChange(TableViewTest.java:351)
>
> TreeTableViewTest >
> test_rt27180_expandBranch_laterSiblingSelected_singleSelection FAILED
>      java.lang.AssertionError
>          at org.junit.Assert.fail(Assert.java:87)
>          at org.junit.Assert.assertTrue(Assert.java:42)
>          at org.junit.Assert.assertTrue(Assert.java:53)
>          at
> test.javafx.scene.control.TreeTableViewTest.test_rt27180_expandBranch_laterSiblingSelected_singleSelection(TreeTableViewTest.java:2315)
>
> TreeTableViewTest >
> test_rt27180_expandBranch_laterSiblingAndChildrenSelected FAILED
>      java.lang.AssertionError
>          at org.junit.Assert.fail(Assert.java:87)
>          at org.junit.Assert.assertTrue(Assert.java:42)
>          at org.junit.Assert.assertTrue(Assert.java:53)
>          at
> test.javafx.scene.control.TreeTableViewTest.test_rt27180_expandBranch_laterSiblingAndChildrenSelected(TreeTableViewTest.java:2340)
>
> TreeTableViewTest > test_rt27185 FAILED
>      java.lang.AssertionError: expected:<TreeItem [ value: Mike Graham
> ]> but was:<null>
>          at org.junit.Assert.fail(Assert.java:89)
>          at org.junit.Assert.failNotEquals(Assert.java:835)
>          at org.junit.Assert.assertEquals(Assert.java:120)
>          at org.junit.Assert.assertEquals(Assert.java:146)
>          at
> test.javafx.scene.control.TreeTableViewTest.test_rt27185(TreeTableViewTest.java:1569)
>
> TreeViewTest >
> test_rt27180_expandBranch_laterSiblingSelected_singleSelection FAILED
>      java.lang.AssertionError
>          at org.junit.Assert.fail(Assert.java:87)
>          at org.junit.Assert.assertTrue(Assert.java:42)
>          at org.junit.Assert.assertTrue(Assert.java:53)
>          at
> test.javafx.scene.control.TreeViewTest.test_rt27180_expandBranch_laterSiblingSelected_singleSelection(TreeViewTest.java:1018)
>
> TreeViewTest >
> test_rt27180_collapseBranch_laterSiblingAndChildrenSelected FAILED
>      java.lang.AssertionError
>          at org.junit.Assert.fail(Assert.java:87)
>          at org.junit.Assert.assertTrue(Assert.java:42)
>          at org.junit.Assert.assertTrue(Assert.java:53)
>          at
> test.javafx.scene.control.TreeViewTest.test_rt27180_collapseBranch_laterSiblingAndChildrenSelected(TreeViewTest.java:999)
>
> TreeViewTest > test_rt27180_expandBranch_laterSiblingAndChildrenSelected
> FAILED
>      java.lang.AssertionError
>          at org.junit.Assert.fail(Assert.java:87)
>          at org.junit.Assert.assertTrue(Assert.java:42)
>          at org.junit.Assert.assertTrue(Assert.java:53)
>          at
> test.javafx.scene.control.TreeViewTest.test_rt27180_expandBranch_laterSiblingAndChildrenSelected(TreeViewTest.java:1043)
>
> TreeViewTest > test_rt27185 FAILED
>      java.lang.AssertionError: expected:<TreeItem [ value: Mike Graham
> ]> but was:<null>
>          at org.junit.Assert.fail(Assert.java:89)
>          at org.junit.Assert.failNotEquals(Assert.java:835)
>          at org.junit.Assert.assertEquals(Assert.java:120)
>          at org.junit.Assert.assertEquals(Assert.java:146)
>          at
> test.javafx.scene.control.TreeViewTest.test_rt27185(TreeViewTest.java:630)
>