Repository: ably/ably-java
Issue: EventEmitter.on() does not permit the same listener to be added multiple times

https://github.com/ably/ably-java/blob/9786de65bd44c2f139a53ee235535cb7617cbc87/lib/src/main/java/io/ably/lib/util/EventEmitter.java#L41

The two implementations of EventEmitter.on() prevents the same listener from being registered multiple times (and thus being invoked multiple times per event). It should allow this to happen as per https://sdk.ably.com/builds/ably/specification/main/features/#RTE4.

Changed files:
core/src/test/java/io/ably/lib/util/EventEmitterTest.java, core/src/main/java/io/ably/lib/util/EventEmitter.java

diff --git a/core/src/main/java/io/ably/lib/util/EventEmitter.java b/core/src/main/java/io/ably/lib/util/EventEmitter.java
index 46fac0a7..fc6c7c87 100644
--- a/core/src/main/java/io/ably/lib/util/EventEmitter.java
+++ b/core/src/main/java/io/ably/lib/util/EventEmitter.java
@@ -1,10 +1,7 @@
 package io.ably.lib.util;
 
 import java.util.ArrayList;
-import java.util.HashMap;
-import java.util.Iterator;
 import java.util.List;
-import java.util.Map;
 
 /**
  * A generic interface for event registration and delivery used in a number of the types in the Realtime client library.
@@ -26,8 +23,8 @@ public abstract class EventEmitter<Event, Listener> {
     }
 
     /**
-     * Registers the provided listener all events.
-     * If on() is called more than once with the same listener and event,
+     * Registers the provided listener for all events.
+     * If on() is called more than once with the same listener,
      * the listener is added multiple times to its listener registry.
      * Therefore, as an example, assuming the same listener is registered twice using on(),
      * and an event is emitted once, the listener would be invoked twice.
@@ -39,8 +36,7 @@ public abstract class EventEmitter<Event, Listener> {
      * This listener is invoked on a background thread.
      */
     public synchronized void on(Listener listener) {
-        if(!listeners.contains(listener))
-            listeners.add(listener);
+        listeners.add(listener);
     }
 
     /**
@@ -58,7 +54,7 @@ public abstract class EventEmitter<Event, Listener> {
      * This listener is invoked on a background thread.
      */
     public synchronized void once(Listener listener) {
-        filters.put(listener, new Filter(null, listener, true));
+        filters.add(new Filter(null, listener, true));
     }
 
     /**
@@ -70,7 +66,7 @@ public abstract class EventEmitter<Event, Listener> {
      */
     public synchronized void off(Listener listener) {
         listeners.remove(listener);
-        filters.remove(listener);
+        filters.removeIf(pair -> pair.listener == listener);
     }
 
     /**
@@ -88,7 +84,7 @@ public abstract class EventEmitter<Event, Listener> {
      * This listener is invoked on a background thread.
      */
     public synchronized void on(Event event, Listener listener) {
-        filters.put(listener, new Filter(event, listener, false));
+        filters.add(new Filter(event, listener, false));
     }
 
     /**
@@ -106,7 +102,7 @@ public abstract class EventEmitter<Event, Listener> {
      * This listener is invoked on a background thread.
      */
     public synchronized void once(Event event, Listener listener) {
-        filters.put(listener, new Filter(event, listener, true));
+        filters.add(new Filter(event, listener, true));
     }
 
     /**
@@ -117,9 +113,7 @@ public abstract class EventEmitter<Event, Listener> {
      * @param event The named event.
      */
     public synchronized void off(Event event, Listener listener) {
-        Filter filter = filters.get(listener);
-        if(filter != null && filter.event == event)
-            filters.remove(listener);
+        filters.removeIf(filter -> filter.listener == listener && filter.event != null && filter.event == event);
     }
 
     /**
@@ -144,17 +138,19 @@ public abstract class EventEmitter<Event, Listener> {
             apply(listener, event, args);
         }
 
-        Map<Listener, Filter> clonedFilters = new HashMap<>(filters);
-        for (Iterator<Map.Entry<Listener, Filter>> it = clonedFilters.entrySet().iterator(); it.hasNext(); ) {
-            Map.Entry<Listener, Filter> entry = it.next();
-            if (entry.getValue().apply(event, args)) {
-                filters.remove(entry.getKey());
+        List<Filter> clonedFilters = new ArrayList<>(filters);
+        for (Filter entry: clonedFilters) {
+            if (entry.apply(event, args)) {
+                filters.remove(entry);
             }
         }
     }
 
     protected abstract void apply(Listener listener, Event event, Object... args);
 
+    /**
+     * Used to determine whether or not a listener is interested
+     */
     protected class Filter {
         Filter(Event event, Listener listener, boolean once) { this.event = event; this.listener = listener; this.once = once; }
         private Event event;
@@ -169,6 +165,6 @@ public abstract class EventEmitter<Event, Listener> {
         }
     }
 
-    Map<Listener, Filter> filters = new HashMap<Listener, Filter>();
+    List<Filter> filters = new ArrayList<Filter>();
     List<Listener> listeners = new ArrayList<Listener>();
 }
diff --git a/core/src/test/java/io/ably/lib/util/EventEmitterTest.java b/core/src/test/java/io/ably/lib/util/EventEmitterTest.java
index d56a0e0c..d272a23b 100644
--- a/core/src/test/java/io/ably/lib/util/EventEmitterTest.java
+++ b/core/src/test/java/io/ably/lib/util/EventEmitterTest.java
@@ -73,6 +73,22 @@ public class EventEmitterTest {
         assertEquals(listener.counts.get(MyEvents.event_1), Integer.valueOf(1));
     }
 
+    /**
+     * Register a listener twice, and verify it is called twice
+     * when the event is emitted
+     */
+    @Test
+    public void on_multiple_registrations() {
+        MyEmitter emitter = new MyEmitter();
+        CountingListener listener = new CountingListener();
+        emitter.on(listener);
+        emitter.on(listener);
+        emitter.emit(MyEvents.event_0, "on_multiple_0");
+        emitter.emit(MyEvents.event_1, "on_multiple_1");
+        assertEquals(listener.counts.get(MyEvents.event_0), Integer.valueOf(2));
+        assertEquals(listener.counts.get(MyEvents.event_1), Integer.valueOf(2));
+    }
+
     /**
      * Register and unregister listener, and verify it
      * is not called when the event is emitted
@@ -111,7 +127,7 @@ public class EventEmitterTest {
 
     /**
      * Register a listener for a specific event, and verify it is called
-     * only when that event is emitted
+     * only when that event is emitted.
      */
     @Test
     public void on_event_simple() {
@@ -125,6 +141,23 @@ public class EventEmitterTest {
         assertNull(listener.counts.get(MyEvents.event_1));
     }
 
+    /**
+     * Register a listener multiple times for a specific event, and verify it is called
+     * multiple times when that event is emitted
+     */
+    @Test
+    public void on_event_multiple_registrations() {
+        MyEmitter emitter = new MyEmitter();
+        CountingListener listener = new CountingListener();
+        emitter.on(MyEvents.event_0, listener);
+        emitter.on(MyEvents.event_0, listener);
+        emitter.on(MyEvents.event_1, listener);
+        emitter.emit(MyEvents.event_0, "on_event_simple_0");
+        emitter.emit(MyEvents.event_1, "on_event_simple_1");
+        assertEquals(listener.counts.get(MyEvents.event_0), Integer.valueOf(2));
+        assertEquals(listener.counts.get(MyEvents.event_1), Integer.valueOf(1));
+    }
+
     /**
      * Register a listener for a specific event, and verify
      * it is no longer called after it has been removed