Should all listeners present at start of event always be called?

[jessegreenberg]

PhET made a design decision that all listeners (and only listeners) present at the start of an event are called. From #72 (comment)

Let's say I have listener A and listener B. Listener A removes listener B.
Then when I emit(), should listener B get a callback?

The decision was "Yes". And this seems to be a convention for other frameworks too. But I encountered a case while using timer where this was undesirable (#248) and ended up adding a hasListener check before calling and removing a listener in timer.setTimeout because the delayed listener may have already been removed by another listener during emit.

I read through #72 but couldn't find why PhET made this decision. Does anyone know? Are there other cases where it is not desirable to call all listeners present at the start of an event? In the "A and B" listener example above, is it generally just the responsibility of listener A to check that listener B is still attached?

[samreid]

In #248 (comment) @jonathanolson said:

DOM events and other event systems I've worked with do typically have the convention that the array of listeners at the point of the event are the ones that will be notified. I recall some more practical examples of how it's beneficial (and avoids infinite loops if you remove and re-add a listener in a callback), but I'd have to think more on it.

[pixelzoom]

+1 for "Yes". Notification should operate on a copy of the listener list, unaffected by adding or removing listeners during the process of notification. And ideally, notification should also be independent of (make no guarantee about) order of notification.

[jessegreenberg]

Notification should operate on a copy of the listener list, unaffected by adding or removing listeners during the process of notification.

What is the reason for this?

[pixelzoom]

Iterating over a list while it is potentially being modified is generally problematic. Let me know if you'd like a concrete example. Beyond that general problem (which is not unique to Observer)...

Allowing the observer list to be modified during notification can result in subtle/difficult bugs that are rooted in order dependencies. For example, change the order of registering and you may change the order that something is added/removed. If you can avoid doing so, it's best not to rely on the order that observers are added or removed, or the order that the observer list is traversed.

Some APIs are explicit about this topic, some aren't. For example, Java's Observable explicitly states that all observers will be notified:

After an observable instance changes, an application calling the Observable's notifyObservers method causes all of its observers to be notified of the change by a call to their update method.

... and warns against relying on order of notification:

The order in which notifications will be delivered is unspecified. The default implementation provided in the Observable class will notify Observers in the order in which they registered interest, but subclasses may change this order, use no guaranteed order, deliver notifications on separate threads, or may guarantee that their subclass follows this order, as they choose.

Finally, this might be good fodder to including in the description of Observer for phetsims/phet-info#88.

[samreid]

Iterating over a list while it is potentially being modified is generally problematic.

Can we change TinyEmitter so that adding or removing a listener during emit throws an error?

Maybe something like this:

Index: js/TinyEmitter.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/TinyEmitter.js	(revision 9ebbc9e32ebeac7973a9e2ab51db4a73218aab15)
+++ js/TinyEmitter.js	(date 1561561871000)
@@ -18,9 +18,8 @@
       // @private {function[]} - the listeners that will be called on emit
       this.listeners = [];
 
-      // @private {function[][]} - during emit() keep track of which listeners should receive events in order to manage
-      //                         - removal of listeners during emit()
-      this.activeListenersStack = [];
+      // @private {boolean} - keep track of when emitting so listeners cannot be added or removed during emit
+      this.emitting = false;
 
       // for production memory concerns; no need to keep this around.
       if ( assert ) {
@@ -35,6 +34,7 @@
      * @public
      */
     dispose() {
+      assert && assert( !this.emitting, 'cannot dispose while emitting' );
       this.listeners.length = 0; // See https://github.com/phetsims/axon/issues/124
 
       if ( assert ) {
@@ -51,15 +51,11 @@
 
       // Notify wired-up listeners, if any
       if ( this.listeners.length > 0 ) {
-        this.activeListenersStack.push( this.listeners );
-
-        // Notify listeners--note the activeListenersStack could change as listeners are called, so we do this by index
-        const lastEntry = this.activeListenersStack.length - 1;
-        for ( let i = 0; i < this.activeListenersStack[ lastEntry ].length; i++ ) {
-          this.activeListenersStack[ lastEntry ][ i ].apply( null, arguments );
+        this.emitting = true;
+        for ( let i = 0; i < this.listeners.length; i++ ) {
+          this.listeners[ i ].apply( null, arguments );
         }
-
-        this.activeListenersStack.pop();
+        this.emitting = false;
       }
     }
 
@@ -71,11 +67,7 @@
     addListener( listener ) {
 
       assert && assert( this.listeners.indexOf( listener ) === -1, 'Cannot add the same listener twice' );
-
-      // If a listener is added during an emit(), we must make a copy of the current list of listeners--the newly added
-      // listener will be available for the next emit() but not the one in progress.  This is to match behavior with
-      // removeListener.
-      this.defendListeners();
+      assert && assert( !this.emitting, 'cannot addListener while emitting' );
 
       this.listeners.push( listener );
     }
@@ -87,6 +79,7 @@
      */
     removeListener( listener ) {
 
+      assert && assert( !this.emitting, 'cannot removeListener while emitting' );
       const index = this.listeners.indexOf( listener );
 
       // Throw an error when removing a non-listener (except when the Emitter has already been disposed, see
@@ -95,10 +88,6 @@
         assert( index !== -1, 'tried to removeListener on something that wasn\'t a listener' );
       }
 
-      // If an emit is in progress, make a copy of the current list of listeners--the removed listener will remain in
-      // the list and be called for this emit call, see #72
-      this.defendListeners();
-
       this.listeners.splice( index, 1 );
     }
 
@@ -112,31 +101,6 @@
       }
     }
 
-    /**
-     * If addListener/removeListener is called while emit() is in progress, we must make a defensive copy of the array
-     * of listeners before changing the array, and use it for the rest of the notifications until the emit call has
-     * completed.
-     * @private
-     */
-    defendListeners() {
-
-      for ( let i = this.activeListenersStack.length - 1; i >= 0; i-- ) {
-
-        // Once we meet a level that was already defended, we can stop, since all previous levels are also defended
-        if ( this.activeListenersStack[ i ].defended ) {
-          break;
-        }
-        else {
-          const defendedListeners = this.listeners.slice();
-
-          // Mark copies as 'defended' so that it will use the original listeners when emit started and not the modified
-          // list.
-          defendedListeners.defended = true;
-          this.activeListenersStack[ i ] = defendedListeners;
-        }
-      }
-    }
-
     /**
      * Checks whether a listener is registered with this Emitter
      * @param {function} listener

JG Edit: formatting diff

[jessegreenberg]

Thanks @pixelzoom, #249 (comment) helps a lot.

Can we change TinyEmitter so that adding or removing a listener during emit throws an error?

I worry that would be too restrictive and we will find cases where it is beneficial to add/remove listeners during an emit call. For instance, disposing an Emitter during an emit seems very reasonable/useful to me.

[samreid]

Notes from dev meeting:

We outlined two potential problems with our Emitter algorithm:

• self reentrant (emitting while emitting)
  • JO: this isn't a logical error, there is no need to assert out here.
  • SR: what is the case here?
• error if add/remove while emitting
  • JO: this gives the most flexibility, not a bug
  • JO: We need a way to add/remove listeners at any point.
  • We need a way for a listener to be able to remove its self.

const e = new EMitter
e.addLIstener (()=>{ e.emit() }); // where does this work in practice wihtout a stack overflow on TinyEmitter.activeListenersStack?

1. layout container. If bounds change then emit a layout change which recalculates its bounds (second bounds calc won't change).
2. Bounds listeners for text reflow, if tech change size, then reflow, which rechanges bounds then changes size which changes bounds (won't rechange bounds).

Since we already decided that Property reentry is sometimes natural, and Property changes are powered by TinyEmitter, as a consequence Emitter must allow re-entry.

@jonathanolson also advocated that there are many natural cases where you should be able to re-enter on emit or add/remove listeners during traversal and those should not be errors. We recommend to close this issue. @pixelzoom and @jbphet were not present at this meeting, please reopen if there is more to discuss.

[pixelzoom]

Reopening.

It's not clear to me from the notes in #249 (comment) what was decided about the original issue. That is: At the time that emit is called, are we operating on a copy of the list, so that the listeners that are notified is known at the time of the emit call? Or are we operating on the list itself, such that the listeners may change during the emit call?

@jonathanolson also advocated that there are many natural cases where you should be able to re-enter on emit or add/remove listeners during traversal and those should not be errors.

I agree. But whether listeners can be added/removed during emit is somewhat independent of whether the add/remove will affect the emit call.

[jessegreenberg]

emit does currently use a copy of the listener list for the reasons outlined in this issue and we agreed that was correct during the meeting.

[jessegreenberg]

Re-closing, please reopen if there are more questions.

[samreid]

One more note in case others pass this way in the future, in #215 we discussed that the sim behavior should be independent of listener order. Having a listener remove itself could satisfy that constraint, but having one listener remove another generally would not.