Fix #547 and and #548

The first bug in #548 (and the only issue in #547) was a typo. The second bug was fixed by tracking "friends" of the event types, so we can invalidate everything as needed.

proxy/src/main/java/com/velocitypowered/proxy/event/EventTypeTracker.java

@@ -0,0 +1,41 @@
+package com.velocitypowered.proxy.event;
+
+import com.google.common.collect.ArrayListMultimap;
+import com.google.common.collect.HashMultimap;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ListMultimap;
+import com.google.common.collect.Multimap;
+import com.google.common.reflect.TypeToken;
+import java.util.List;
+import java.util.stream.Collectors;
+
+class EventTypeTracker {
+
+  private final ListMultimap<Class<?>, Class<?>> friends;
+
+  public EventTypeTracker() {
+    this.friends = ArrayListMultimap.create();
+  }
+
+  public List<Class<?>> getFriendsOf(final Class<?> eventType) {
+    if (friends.containsKey(eventType)) {
+      return ImmutableList.copyOf(friends.get(eventType));
+    }
+
+    final List<Class<?>> types = getEventTypes(eventType);
+    for (Class<?> type : types) {
+      if (type == eventType) {
+        continue;
+      }
+
+      friends.put(type, eventType);
+    }
+    return types;
+  }
+
+  private static List<Class<?>> getEventTypes(final Class<?> eventType) {
+    return TypeToken.of(eventType).getTypes().rawTypes().stream()
+        .filter(type -> type != Object.class)
+        .collect(Collectors.toList());
+  }
+}

proxy/src/main/java/com/velocitypowered/proxy/event/VelocityEventManager.java

@@ -99,6 +99,7 @@ public class VelocityEventManager implements EventManager {
   private final ReadWriteLock lock = new ReentrantReadWriteLock();
 
   private final List<CustomHandlerAdapter<?>> handlerAdapters = new ArrayList<>();
+  private final EventTypeTracker eventTypeTracker = new EventTypeTracker();
 
   /**
    * Initializes the Velocity event manager.
@@ -172,15 +173,9 @@ public class VelocityEventManager implements EventManager {
     }
   }
 
-  private static List<Class<?>> getEventTypes(final Class<?> eventType) {
-    return TypeToken.of(eventType).getTypes().rawTypes().stream()
-        .filter(type -> type != Object.class)
-        .collect(Collectors.toList());
-  }
-
   private @Nullable HandlersCache bakeHandlers(final Class<?> eventType) {
     final List<HandlerRegistration> baked = new ArrayList<>();
-    final List<Class<?>> types = getEventTypes(eventType);
+    final List<Class<?>> types = eventTypeTracker.getFriendsOf(eventType);
 
     lock.readLock().lock();
     try {
@@ -336,7 +331,7 @@ public class VelocityEventManager implements EventManager {
     }
     // Invalidate all the affected event subtypes
     handlersCache.invalidateAll(registrations.stream()
-        .flatMap(registration -> getEventTypes(registration.eventType).stream())
+        .flatMap(registration -> eventTypeTracker.getFriendsOf(registration.eventType).stream())
         .distinct()
         .collect(Collectors.toList()));
   }
@@ -407,7 +402,7 @@ public class VelocityEventManager implements EventManager {
     final PluginContainer pluginContainer = pluginManager.ensurePluginContainer(plugin);
     requireNonNull(handler, "handler");
     unregisterIf(registration ->
-        registration.plugin == pluginContainer && registration.handler == handler);
+        registration.plugin == pluginContainer && registration.instance == handler);
   }
 
   @Override
@@ -433,7 +428,7 @@ public class VelocityEventManager implements EventManager {
 
     // Invalidate all the affected event subtypes
     handlersCache.invalidateAll(removed.stream()
-        .flatMap(registration -> getEventTypes(registration.eventType).stream())
+        .flatMap(registration -> eventTypeTracker.getFriendsOf(registration.eventType).stream())
         .distinct()
         .collect(Collectors.toList()));
   }

proxy/src/test/java/com/velocitypowered/proxy/event/RegistrationTest.java

@@ -0,0 +1,199 @@
+/*
+ * Copyright (C) 2018 Velocity Contributors
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <https://www.gnu.org/licenses/>.
+ */
+
+package com.velocitypowered.proxy.event;
+
+import static com.velocitypowered.proxy.testutil.FakePluginManager.PLUGIN_A;
+import static com.velocitypowered.proxy.testutil.FakePluginManager.PLUGIN_B;
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertSame;
+
+import com.velocitypowered.api.event.EventHandler;
+import com.velocitypowered.api.event.EventManager;
+import com.velocitypowered.api.event.Subscribe;
+import com.velocitypowered.api.plugin.PluginManager;
+import com.velocitypowered.proxy.testutil.FakePluginManager;
+import java.util.HashSet;
+import java.util.Set;
+import java.util.stream.Stream;
+
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.DynamicNode;
+import org.junit.jupiter.api.DynamicTest;
+import org.junit.jupiter.api.TestFactory;
+
+public class RegistrationTest {
+
+  private EventManager eventManager;
+
+  @BeforeEach
+  public final void setup() {
+    resetEventManager();
+  }
+
+  private void resetEventManager() {
+    eventManager = createEventManager(new FakePluginManager());
+  }
+
+  protected EventManager createEventManager(PluginManager pluginManager) {
+    return new VelocityEventManager(pluginManager);
+  }
+
+  // Must be public in order to generate a method calling it
+  public static class SimpleEvent {
+    int value;
+  }
+
+  public static class SimpleSubclassedEvent extends SimpleEvent { }
+
+  public static class HandlerListener implements EventHandler<SimpleEvent> {
+
+    @Override
+    public void execute(SimpleEvent event) {
+      event.value++;
+    }
+  }
+
+  public static class AnnotatedListener {
+
+    @Subscribe
+    public void increment(SimpleEvent event) {
+      event.value++;
+    }
+  }
+
+  private interface EventGenerator {
+
+    void assertFiredEventValue(int value);
+  }
+
+  private interface TestFunction {
+
+    void runTest(boolean annotated, EventGenerator generator);
+  }
+
+  private Stream<DynamicNode> composeTests(String name, TestFunction testFunction) {
+    Set<DynamicNode> tests = new HashSet<>();
+    boolean[] trueAndFalse = new boolean[] {true, false};
+    for (boolean annotated : trueAndFalse) {
+      for (boolean subclassed : trueAndFalse) {
+
+        EventGenerator generator = (value) -> {
+          SimpleEvent simpleEvent = (subclassed) ? new SimpleSubclassedEvent() : new SimpleEvent();
+          SimpleEvent shouldBeSameEvent = eventManager.fire(simpleEvent).join();
+          assertSame(simpleEvent, shouldBeSameEvent);
+          assertEquals(value, simpleEvent.value);
+        };
+        tests.add(DynamicTest.dynamicTest(name + ". Annotated : " + annotated + ", Subclassed: " + subclassed, () -> {
+          try {
+            testFunction.runTest(annotated, generator);
+          } finally {
+            resetEventManager();
+          }
+        }));
+      }
+    }
+    return tests.stream();
+  }
+
+  @TestFactory
+  Stream<DynamicNode> simpleRegisterAndUnregister() {
+    return composeTests("simpleRegisterAndUnregister", (annotated, generator) -> {
+      if (annotated) {
+        eventManager.register(PLUGIN_A, new AnnotatedListener());
+      } else {
+        eventManager.register(PLUGIN_A, SimpleEvent.class, new HandlerListener());
+      }
+      generator.assertFiredEventValue(1);
+      eventManager.unregisterListeners(PLUGIN_A);
+      generator.assertFiredEventValue(0);
+      assertDoesNotThrow(() -> eventManager.unregisterListeners(PLUGIN_A), "Extra unregister is a no-op");
+    });
+  }
+
+  @TestFactory
+  Stream<DynamicNode> doubleRegisterListener() {
+    return composeTests("doubleRegisterListener", (annotated, generator) -> {
+      if (annotated) {
+        Object annotatedListener = new AnnotatedListener();
+        eventManager.register(PLUGIN_A, annotatedListener);
+        eventManager.register(PLUGIN_A, annotatedListener);
+      } else {
+        EventHandler<SimpleEvent> handler = new HandlerListener();
+        eventManager.register(PLUGIN_A, SimpleEvent.class, handler);
+        eventManager.register(PLUGIN_A, SimpleEvent.class, handler);
+      }
+      generator.assertFiredEventValue(2);
+    });
+  }
+
+  @TestFactory
+  Stream<DynamicNode> doubleRegisterListenerDifferentPlugins() {
+    return composeTests("doubleRegisterListenerDifferentPlugins", (annotated, generator) -> {
+      if (annotated) {
+        Object annotatedListener = new AnnotatedListener();
+        eventManager.register(PLUGIN_A, annotatedListener);
+        eventManager.register(PLUGIN_B, annotatedListener);
+      } else {
+        EventHandler<SimpleEvent> handler = new HandlerListener();
+        eventManager.register(PLUGIN_A, SimpleEvent.class, handler);
+        eventManager.register(PLUGIN_B, SimpleEvent.class, handler);
+      }
+      generator.assertFiredEventValue(2);
+    });
+  }
+
+  @TestFactory
+  Stream<DynamicNode> doubleRegisterListenerThenUnregister() {
+    return composeTests("doubleRegisterListenerThenUnregister", (annotated, generator) -> {
+      if (annotated) {
+        Object annotatedListener = new AnnotatedListener();
+        eventManager.register(PLUGIN_A, annotatedListener);
+        eventManager.register(PLUGIN_A, annotatedListener);
+        eventManager.unregisterListener(PLUGIN_A, annotatedListener);
+      } else {
+        EventHandler<SimpleEvent> handler = new HandlerListener();
+        eventManager.register(PLUGIN_A, SimpleEvent.class, handler);
+        eventManager.register(PLUGIN_A, SimpleEvent.class, handler);
+        eventManager.unregister(PLUGIN_A, handler);
+      }
+      generator.assertFiredEventValue(0);
+    });
+  }
+
+  @TestFactory
+  Stream<DynamicNode> doubleUnregisterListener() {
+    return composeTests("doubleUnregisterListener", (annotated, generator) -> {
+      if (annotated) {
+        Object annotatedListener = new AnnotatedListener();
+        eventManager.register(PLUGIN_A, annotatedListener);
+        eventManager.unregisterListener(PLUGIN_A, annotatedListener);
+        assertDoesNotThrow(() -> eventManager.unregisterListener(PLUGIN_A, annotatedListener),
+            "Extra unregister is a no-op");
+      } else {
+        EventHandler<SimpleEvent> handler = new HandlerListener();
+        eventManager.register(PLUGIN_A, SimpleEvent.class, handler);
+        eventManager.unregister(PLUGIN_A, handler);
+        assertDoesNotThrow(() -> eventManager.unregister(PLUGIN_A, handler),
+            "Extra unregister is a no-op");
+      }
+      generator.assertFiredEventValue(0);
+    });
+  }
+
+}