From e017949abf775cace3f0c63b6e7f9f576d77ff93 Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Mon, 19 Jul 2021 13:26:37 -0400 Subject: [PATCH] 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/event/EventTypeTracker.java | 41 ++++ .../proxy/event/VelocityEventManager.java | 15 +- .../proxy/event/RegistrationTest.java | 199 ++++++++++++++++++ 3 files changed, 245 insertions(+), 10 deletions(-) create mode 100644 proxy/src/main/java/com/velocitypowered/proxy/event/EventTypeTracker.java create mode 100644 proxy/src/test/java/com/velocitypowered/proxy/event/RegistrationTest.java diff --git a/proxy/src/main/java/com/velocitypowered/proxy/event/EventTypeTracker.java b/proxy/src/main/java/com/velocitypowered/proxy/event/EventTypeTracker.java new file mode 100644 index 000000000..6c9c39149 --- /dev/null +++ b/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> friends; + + public EventTypeTracker() { + this.friends = ArrayListMultimap.create(); + } + + public List> getFriendsOf(final Class eventType) { + if (friends.containsKey(eventType)) { + return ImmutableList.copyOf(friends.get(eventType)); + } + + final List> types = getEventTypes(eventType); + for (Class type : types) { + if (type == eventType) { + continue; + } + + friends.put(type, eventType); + } + return types; + } + + private static List> getEventTypes(final Class eventType) { + return TypeToken.of(eventType).getTypes().rawTypes().stream() + .filter(type -> type != Object.class) + .collect(Collectors.toList()); + } +} diff --git a/proxy/src/main/java/com/velocitypowered/proxy/event/VelocityEventManager.java b/proxy/src/main/java/com/velocitypowered/proxy/event/VelocityEventManager.java index ac337aa5f..be1af1e20 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/event/VelocityEventManager.java +++ b/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> 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> 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 baked = new ArrayList<>(); - final List> types = getEventTypes(eventType); + final List> 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())); } diff --git a/proxy/src/test/java/com/velocitypowered/proxy/event/RegistrationTest.java b/proxy/src/test/java/com/velocitypowered/proxy/event/RegistrationTest.java new file mode 100644 index 000000000..a70724b45 --- /dev/null +++ b/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 . + */ + +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 { + + @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 composeTests(String name, TestFunction testFunction) { + Set 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 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 doubleRegisterListener() { + return composeTests("doubleRegisterListener", (annotated, generator) -> { + if (annotated) { + Object annotatedListener = new AnnotatedListener(); + eventManager.register(PLUGIN_A, annotatedListener); + eventManager.register(PLUGIN_A, annotatedListener); + } else { + EventHandler handler = new HandlerListener(); + eventManager.register(PLUGIN_A, SimpleEvent.class, handler); + eventManager.register(PLUGIN_A, SimpleEvent.class, handler); + } + generator.assertFiredEventValue(2); + }); + } + + @TestFactory + Stream doubleRegisterListenerDifferentPlugins() { + return composeTests("doubleRegisterListenerDifferentPlugins", (annotated, generator) -> { + if (annotated) { + Object annotatedListener = new AnnotatedListener(); + eventManager.register(PLUGIN_A, annotatedListener); + eventManager.register(PLUGIN_B, annotatedListener); + } else { + EventHandler handler = new HandlerListener(); + eventManager.register(PLUGIN_A, SimpleEvent.class, handler); + eventManager.register(PLUGIN_B, SimpleEvent.class, handler); + } + generator.assertFiredEventValue(2); + }); + } + + @TestFactory + Stream 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 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 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 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); + }); + } + +}