Listeners behavior discussion

[John Hendrikx]

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)