From 84c6c70b9003433714552ca733a6b38ad5c84208 Mon Sep 17 00:00:00 2001 From: Score_Under Date: Thu, 5 Jul 2012 23:51:12 +0100 Subject: [BREAKING] Use event class instead of event for timings. Fixes BUKKIT-4664 TimedRegisteredListener uses a reference to the first event fired. This causes a memory leak in the server for any references that event has. This changes TimedRegisteredListener to only store a reference to the class of the event. This change is intentionally a breaking change, as it is an obscure part of the API. A non-breaking change would require the leak to be maintained or an immediate update for any plugins using the method, as it would be an indirect break. A unit test is also included to check behavior of shared superclass functionality. --- .../bukkit/command/defaults/TimingsCommand.java | 6 +-- .../org/bukkit/plugin/TimedRegisteredListener.java | 38 ++++++++++----- .../bukkit/plugin/TimedRegisteredListenerTest.java | 56 ++++++++++++++++++++++ 3 files changed, 86 insertions(+), 14 deletions(-) create mode 100644 src/test/java/org/bukkit/plugin/TimedRegisteredListenerTest.java diff --git a/src/main/java/org/bukkit/command/defaults/TimingsCommand.java b/src/main/java/org/bukkit/command/defaults/TimingsCommand.java index 29ebbe05..05cfcb01 100644 --- a/src/main/java/org/bukkit/command/defaults/TimingsCommand.java +++ b/src/main/java/org/bukkit/command/defaults/TimingsCommand.java @@ -84,9 +84,9 @@ public class TimingsCommand extends BukkitCommand { if (count == 0) continue; long avg = time / count; totalTime += time; - Event event = trl.getEvent(); - if (count > 0 && event != null) { - fileTimings.println(" " + event.getClass().getSimpleName() + (trl.hasMultiple() ? " (and others)" : "") + " Time: " + time + " Count: " + count + " Avg: " + avg); + Class eventClass = trl.getEventClass(); + if (count > 0 && eventClass != null) { + fileTimings.println(" " + eventClass.getSimpleName() + (trl.hasMultiple() ? " (and sub-classes)" : "") + " Time: " + time + " Count: " + count + " Avg: " + avg); } } } diff --git a/src/main/java/org/bukkit/plugin/TimedRegisteredListener.java b/src/main/java/org/bukkit/plugin/TimedRegisteredListener.java index ed25e179..d86805b9 100644 --- a/src/main/java/org/bukkit/plugin/TimedRegisteredListener.java +++ b/src/main/java/org/bukkit/plugin/TimedRegisteredListener.java @@ -11,7 +11,7 @@ import org.bukkit.event.Listener; public class TimedRegisteredListener extends RegisteredListener { private int count; private long totalTime; - private Event event; + private Class eventClass; private boolean multiple = false; public TimedRegisteredListener(final Listener pluginListener, final EventExecutor eventExecutor, final EventPriority eventPriority, final Plugin registeredPlugin, final boolean listenCancelled) { @@ -25,17 +25,25 @@ public class TimedRegisteredListener extends RegisteredListener { return; } count++; - if (this.event == null) { - this.event = event; - } - else if (!this.event.getClass().equals(event.getClass())) { + Class newEventClass = event.getClass(); + if (this.eventClass == null) { + this.eventClass = newEventClass; + } else if (!this.eventClass.equals(newEventClass)) { multiple = true; + this.eventClass = getCommonSuperclass(newEventClass, this.eventClass).asSubclass(Event.class); } long start = System.nanoTime(); super.callEvent(event); totalTime += System.nanoTime() - start; } + private static Class getCommonSuperclass(Class class1, Class class2) { + while (!class1.isAssignableFrom(class2)) { + class1 = class1.getSuperclass(); + } + return class1; + } + /** * Resets the call count and total time for this listener */ @@ -63,18 +71,26 @@ public class TimedRegisteredListener extends RegisteredListener { } /** - * Gets the first event this listener handled + * Gets the class of the events this listener handled. If it handled + * multiple classes of event, the closest shared superclass will be + * returned, such that for any event this listener has handled, + * this.getEventClass().isAssignableFrom(event.getClass()) + * and no class + * this.getEventClass().isAssignableFrom(clazz) + * && this.getEventClass() != clazz && + * event.getClass().isAssignableFrom(clazz) for all handled events. * - * @return An event handled by this RegisteredListener + * @return the event class handled by this RegisteredListener */ - public Event getEvent() { - return event; + public Class getEventClass() { + return eventClass; } /** - * Gets whether this listener has handled multiple events + * Gets whether this listener has handled multiple events, such that for + * some two events, eventA.getClass() != eventB.getClass(). * - * @return True if this listener has handled multiple events + * @return true if this listener has handled multiple events */ public boolean hasMultiple() { return multiple; diff --git a/src/test/java/org/bukkit/plugin/TimedRegisteredListenerTest.java b/src/test/java/org/bukkit/plugin/TimedRegisteredListenerTest.java new file mode 100644 index 00000000..b206b1f3 --- /dev/null +++ b/src/test/java/org/bukkit/plugin/TimedRegisteredListenerTest.java @@ -0,0 +1,56 @@ +package org.bukkit.plugin; + +import static org.hamcrest.Matchers.*; +import static org.junit.Assert.*; + +import org.bukkit.event.Event; +import org.bukkit.event.EventException; +import org.bukkit.event.EventPriority; +import org.bukkit.event.Listener; +import org.bukkit.event.block.BlockBreakEvent; +import org.bukkit.event.player.PlayerEvent; +import org.bukkit.event.player.PlayerInteractEvent; +import org.bukkit.event.player.PlayerMoveEvent; +import org.junit.Test; + +public class TimedRegisteredListenerTest { + + @Test + public void testEventClass() throws EventException { + Listener listener = new Listener() {}; + EventExecutor executor = new EventExecutor() { + public void execute(Listener listener, Event event) {} + }; + TestPlugin plugin = new TestPlugin("Test"); + + PlayerInteractEvent interactEvent = new PlayerInteractEvent(null, null, null, null, null); + PlayerMoveEvent moveEvent = new PlayerMoveEvent(null, null, null); + BlockBreakEvent breakEvent = new BlockBreakEvent(null, null); + + TimedRegisteredListener trl = new TimedRegisteredListener(listener, executor, EventPriority.NORMAL, plugin, false); + + // Ensure that the correct event type is reported for a single event + trl.callEvent(interactEvent); + assertThat(trl.getEventClass(), is((Object) PlayerInteractEvent.class)); + // Ensure that no superclass is used in lieu of the actual event, after two identical event types + trl.callEvent(interactEvent); + assertThat(trl.getEventClass(), is((Object) PlayerInteractEvent.class)); + // Ensure that the closest superclass of the two events is chosen + trl.callEvent(moveEvent); + assertThat(trl.getEventClass(), is((Object) PlayerEvent.class)); + // As above, so below + trl.callEvent(breakEvent); + assertThat(trl.getEventClass(), is((Object) Event.class)); + // In the name of being thorough, check that it never travels down the hierarchy again. + trl.callEvent(breakEvent); + assertThat(trl.getEventClass(), is((Object) Event.class)); + + trl = new TimedRegisteredListener(listener, executor, EventPriority.NORMAL, plugin, false); + + trl.callEvent(breakEvent); + assertThat(trl.getEventClass(), is((Object) BlockBreakEvent.class)); + // Test moving up the class hierarchy by more than one class at a time + trl.callEvent(moveEvent); + assertThat(trl.getEventClass(), is((Object) Event.class)); + } +} -- cgit v1.2.3