From 9e73a8887c5f142abbc7a324df17d167b2427478 Mon Sep 17 00:00:00 2001 From: Wesley Wolfe Date: Wed, 13 Jun 2012 21:28:13 -0500 Subject: Support asynchronous events; Addresses BUKKIT-1212 --- src/main/java/org/bukkit/Bukkit.java | 4 + src/main/java/org/bukkit/Server.java | 5 + src/main/java/org/bukkit/event/Event.java | 35 ++++++ src/main/java/org/bukkit/event/HandlerList.java | 67 +++++++----- src/main/java/org/bukkit/plugin/PluginManager.java | 5 +- .../org/bukkit/plugin/SimplePluginManager.java | 21 +++- .../org/bukkit/plugin/TimedRegisteredListener.java | 5 + src/test/java/org/bukkit/TestServer.java | 48 +++++++++ src/test/java/org/bukkit/event/TestEvent.java | 19 ++++ .../java/org/bukkit/plugin/PluginManagerTest.java | 118 +++++++++++++++++++++ 10 files changed, 300 insertions(+), 27 deletions(-) create mode 100644 src/test/java/org/bukkit/TestServer.java create mode 100644 src/test/java/org/bukkit/event/TestEvent.java create mode 100644 src/test/java/org/bukkit/plugin/PluginManagerTest.java (limited to 'src') diff --git a/src/main/java/org/bukkit/Bukkit.java b/src/main/java/org/bukkit/Bukkit.java index 8654b50a..f59d4342 100644 --- a/src/main/java/org/bukkit/Bukkit.java +++ b/src/main/java/org/bukkit/Bukkit.java @@ -365,4 +365,8 @@ public final class Bukkit { public static int getWaterAnimalSpawnLimit() { return server.getWaterAnimalSpawnLimit(); } + + public static boolean isPrimaryThread() { + return server.isPrimaryThread(); + } } diff --git a/src/main/java/org/bukkit/Server.java b/src/main/java/org/bukkit/Server.java index 9377d0cd..a2b3ff9c 100644 --- a/src/main/java/org/bukkit/Server.java +++ b/src/main/java/org/bukkit/Server.java @@ -639,4 +639,9 @@ public interface Server extends PluginMessageRecipient { * @returns The water animal spawn limit */ int getWaterAnimalSpawnLimit(); + + /** + * Returns true if the current {@link Thread} is the server's primary thread + */ + boolean isPrimaryThread(); } \ No newline at end of file diff --git a/src/main/java/org/bukkit/event/Event.java b/src/main/java/org/bukkit/event/Event.java index 92351af2..273e5b26 100644 --- a/src/main/java/org/bukkit/event/Event.java +++ b/src/main/java/org/bukkit/event/Event.java @@ -5,6 +5,23 @@ package org.bukkit.event; */ public abstract class Event { private String name; + private final boolean async; + + /** + * The default constructor is defined for cleaner code. + * This constructor assumes the event is synchronous. + */ + public Event() { + this(false); + } + + /** + * This constructor is used to explicitly declare an event as synchronous or asynchronous. + * @param isAsync true indicates the event will fire asynchronously. false by default + */ + public Event(boolean isAsync) { + this.async = isAsync; + } /** * @return Name of this event @@ -18,6 +35,24 @@ public abstract class Event { public abstract HandlerList getHandlers(); + /** + * Any custom event that should not by synchronized with other events must use the specific constructor. + * These are the caveats of using an asynchronous event: + *
  • The event is never fired from inside code triggered by a synchronous event. + * Attempting to do so results in an {@link java.lang.IllegalStateException}.
    • + *
  • However, asynchronous event handlers may fire synchronous or asynchronous events
    • + *
  • The event may be fired multiple times simultaneously and in any order.
    • + *
  • Any newly registered or unregistered handler is ignored after an event starts execution.
    • + *
  • The handlers for this event may block for any length of time.
    • + *
  • Some implementations may selectively declare a specific event use as asynchronous. + * This behavior should be clearly defined.
    • + *
  • Asynchronous calls are not calculated in the plugin timing system.
    • + * @return false by default, true if the event fires asynchronously + */ + public final boolean isAsynchronous() { + return async; + } + public enum Result { /** diff --git a/src/main/java/org/bukkit/event/HandlerList.java b/src/main/java/org/bukkit/event/HandlerList.java index f48d942f..b0014119 100644 --- a/src/main/java/org/bukkit/event/HandlerList.java +++ b/src/main/java/org/bukkit/event/HandlerList.java @@ -13,7 +13,7 @@ public class HandlerList { /** * Handler array. This field being an array is the key to this system's speed. */ - private RegisteredListener[] handlers = null; + private volatile RegisteredListener[] handlers = null; /** * Dynamic handler lists. These are changed using register() and @@ -33,8 +33,10 @@ public class HandlerList { * you're using fevents in a plugin system. */ public static void bakeAll() { - for (HandlerList h : allLists) { - h.bake(); + synchronized (allLists) { + for (HandlerList h : allLists) { + h.bake(); + } } } @@ -42,11 +44,15 @@ public class HandlerList { * Unregister all listeners from all handler lists. */ public static void unregisterAll() { - for (HandlerList h : allLists) { - for (List list : h.handlerslots.values()) { - list.clear(); + synchronized (allLists) { + for (HandlerList h : allLists) { + synchronized (h) { + for (List list : h.handlerslots.values()) { + list.clear(); + } + h.handlers = null; + } } - h.handlers = null; } } @@ -56,8 +62,10 @@ public class HandlerList { * @param plugin plugin to unregister */ public static void unregisterAll(Plugin plugin) { - for (HandlerList h : allLists) { - h.unregister(plugin); + synchronized (allLists) { + for (HandlerList h : allLists) { + h.unregister(plugin); + } } } @@ -67,8 +75,10 @@ public class HandlerList { * @param listener listener to unregister */ public static void unregisterAll(Listener listener) { - for (HandlerList h : allLists) { - h.unregister(listener); + synchronized (allLists) { + for (HandlerList h : allLists) { + h.unregister(listener); + } } } @@ -81,7 +91,9 @@ public class HandlerList { for (EventPriority o : EventPriority.values()) { handlerslots.put(o, new ArrayList()); } - allLists.add(this); + synchronized (allLists) { + allLists.add(this); + } } /** @@ -89,7 +101,7 @@ public class HandlerList { * * @param listener listener to register */ - public void register(RegisteredListener listener) { + public synchronized void register(RegisteredListener listener) { if (handlerslots.get(listener.getPriority()).contains(listener)) throw new IllegalStateException("This listener is already registered to priority " + listener.getPriority().toString()); handlers = null; @@ -112,7 +124,7 @@ public class HandlerList { * * @param listener listener to remove */ - public void unregister(RegisteredListener listener) { + public synchronized void unregister(RegisteredListener listener) { if (handlerslots.get(listener.getPriority()).remove(listener)) { handlers = null; } @@ -123,7 +135,7 @@ public class HandlerList { * * @param plugin plugin to remove */ - public void unregister(Plugin plugin) { + public synchronized void unregister(Plugin plugin) { boolean changed = false; for (List list : handlerslots.values()) { for (ListIterator i = list.listIterator(); i.hasNext();) { @@ -141,7 +153,7 @@ public class HandlerList { * * @param listener listener to remove */ - public void unregister(Listener listener) { + public synchronized void unregister(Listener listener) { boolean changed = false; for (List list : handlerslots.values()) { for (ListIterator i = list.listIterator(); i.hasNext();) { @@ -157,7 +169,7 @@ public class HandlerList { /** * Bake HashMap and ArrayLists to 2d array - does nothing if not necessary */ - public void bake() { + public synchronized void bake() { if (handlers != null) return; // don't re-bake when still valid List entries = new ArrayList(); for (Entry> entry : handlerslots.entrySet()) { @@ -172,7 +184,8 @@ public class HandlerList { * @return the array of registered listeners */ public RegisteredListener[] getRegisteredListeners() { - bake(); + RegisteredListener[] handlers; + while ((handlers = this.handlers) == null) bake(); // This prevents fringe cases of returning null return handlers; } @@ -185,11 +198,15 @@ public class HandlerList { */ public static ArrayList getRegisteredListeners(Plugin plugin) { ArrayList listeners = new ArrayList(); - for (HandlerList h : allLists) { - for (List list : h.handlerslots.values()) { - for (RegisteredListener listener : list) { - if (listener.getPlugin().equals(plugin)) { - listeners.add(listener); + synchronized (allLists) { + for (HandlerList h : allLists) { + synchronized (h) { + for (List list : h.handlerslots.values()) { + for (RegisteredListener listener : list) { + if (listener.getPlugin().equals(plugin)) { + listeners.add(listener); + } + } } } } @@ -204,6 +221,8 @@ public class HandlerList { */ @SuppressWarnings("unchecked") public static ArrayList getHandlerLists() { - return (ArrayList) allLists.clone(); + synchronized (allLists) { + return (ArrayList) allLists.clone(); + } } } diff --git a/src/main/java/org/bukkit/plugin/PluginManager.java b/src/main/java/org/bukkit/plugin/PluginManager.java index 804f4415..89697de8 100644 --- a/src/main/java/org/bukkit/plugin/PluginManager.java +++ b/src/main/java/org/bukkit/plugin/PluginManager.java @@ -92,8 +92,11 @@ public interface PluginManager { * Calls an event with the given details * * @param event Event details + * @throws IllegalStateException Thrown when an asynchronous event is fired from synchronous code.
    + * Note: This is best-effort basis, and should not be used to test synchronized state. This + * is an indicator for flawed flow logic. */ - public void callEvent(Event event); + public void callEvent(Event event) throws IllegalStateException; /** * Registers all the events in the given listener class diff --git a/src/main/java/org/bukkit/plugin/SimplePluginManager.java b/src/main/java/org/bukkit/plugin/SimplePluginManager.java index e017ffdf..ca780fca 100644 --- a/src/main/java/org/bukkit/plugin/SimplePluginManager.java +++ b/src/main/java/org/bukkit/plugin/SimplePluginManager.java @@ -443,11 +443,28 @@ public final class SimplePluginManager implements PluginManager { } /** - * Calls an event with the given details + * Calls an event with the given details.
    + * This method only synchronizes when the event is not asynchronous. * * @param event Event details */ - public synchronized void callEvent(Event event) { + public void callEvent(Event event) { + if (event.isAsynchronous()) { + if (Thread.holdsLock(this)) { + throw new IllegalStateException(event.getEventName() + " cannot be triggered asynchronously from inside synchronized code."); + } + if (server.isPrimaryThread()) { + throw new IllegalStateException(event.getEventName() + " cannot be triggered asynchronously from primary server thread."); + } + fireEvent(event); + } else { + synchronized (this) { + fireEvent(event); + } + } + } + + private void fireEvent(Event event) { HandlerList handlers = event.getHandlers(); RegisteredListener[] listeners = handlers.getRegisteredListeners(); diff --git a/src/main/java/org/bukkit/plugin/TimedRegisteredListener.java b/src/main/java/org/bukkit/plugin/TimedRegisteredListener.java index 53dfde38..ed25e179 100644 --- a/src/main/java/org/bukkit/plugin/TimedRegisteredListener.java +++ b/src/main/java/org/bukkit/plugin/TimedRegisteredListener.java @@ -18,7 +18,12 @@ public class TimedRegisteredListener extends RegisteredListener { super(pluginListener, eventExecutor, eventPriority, registeredPlugin, listenCancelled); } + @Override public void callEvent(Event event) throws EventException { + if (event.isAsynchronous()) { + super.callEvent(event); + return; + } count++; if (this.event == null) { this.event = event; diff --git a/src/test/java/org/bukkit/TestServer.java b/src/test/java/org/bukkit/TestServer.java new file mode 100644 index 00000000..63e8aeff --- /dev/null +++ b/src/test/java/org/bukkit/TestServer.java @@ -0,0 +1,48 @@ +package org.bukkit; + +import java.lang.reflect.Constructor; +import java.lang.reflect.InvocationHandler; +import java.lang.reflect.Method; +import java.lang.reflect.Proxy; +import java.util.HashMap; + + +public class TestServer implements InvocationHandler { + private static interface MethodHandler { + Object handle(TestServer server, Object[] args); + } + private static final Constructor constructor; + private static final HashMap methods = new HashMap(); + static { + try { + methods.put(Server.class.getMethod("isPrimaryThread"), + new MethodHandler() { + public Object handle(TestServer server, Object[] args) { + return Thread.currentThread().equals(server.creatingThread); + } + }); + constructor = Proxy.getProxyClass(Server.class.getClassLoader(), Server.class).asSubclass(Server.class).getConstructor(InvocationHandler.class); + } catch (Throwable t) { + throw new Error(t); + } + } + + private Thread creatingThread = Thread.currentThread(); + private TestServer() {}; + + public static Server getInstance() { + try { + return constructor.newInstance(new TestServer()); + } catch (Throwable t) { + throw new RuntimeException(t); + } + } + + public Object invoke(Object proxy, Method method, Object[] args) { + MethodHandler handler = methods.get(method); + if (handler != null) { + return handler.handle(this, args); + } + throw new UnsupportedOperationException(String.valueOf(method)); + } +} diff --git a/src/test/java/org/bukkit/event/TestEvent.java b/src/test/java/org/bukkit/event/TestEvent.java new file mode 100644 index 00000000..c06dfe94 --- /dev/null +++ b/src/test/java/org/bukkit/event/TestEvent.java @@ -0,0 +1,19 @@ +package org.bukkit.event; + + +public class TestEvent extends Event { + private static final HandlerList handlers = new HandlerList(); + + public TestEvent(boolean async) { + super(async); + } + + @Override + public HandlerList getHandlers() { + return handlers; + } + + public static HandlerList getHandlerList() { + return handlers; + } +} diff --git a/src/test/java/org/bukkit/plugin/PluginManagerTest.java b/src/test/java/org/bukkit/plugin/PluginManagerTest.java new file mode 100644 index 00000000..b5cb7408 --- /dev/null +++ b/src/test/java/org/bukkit/plugin/PluginManagerTest.java @@ -0,0 +1,118 @@ +package org.bukkit.plugin; + +import static junit.framework.Assert.*; + +import org.bukkit.Server; +import org.bukkit.TestServer; +import org.bukkit.command.SimpleCommandMap; +import org.bukkit.event.Event; +import org.bukkit.event.TestEvent; +import org.junit.Test; + +public class PluginManagerTest { + private class MutableObject { + volatile Object value = null; + } + + final Server server = TestServer.getInstance(); + final SimpleCommandMap commandMap = new SimpleCommandMap(server); + final PluginManager pm = new SimplePluginManager(server, commandMap); + final MutableObject store = new MutableObject(); + + @Test + public void testAsyncSameThread() { + final Event event = new TestEvent(true); + try { + pm.callEvent(event); + } catch (IllegalStateException ex) { + assertEquals(event.getEventName() + " cannot be triggered asynchronously from primary server thread.", ex.getMessage()); + return; + } + throw new IllegalStateException("No exception thrown"); + } + + @Test + public void testSyncSameThread() { + final Event event = new TestEvent(false); + pm.callEvent(event); + } + + @Test + public void testAsyncLocked() throws InterruptedException { + final Event event = new TestEvent(true); + Thread secondThread = new Thread( + new Runnable() { + public void run() { + try { + synchronized (pm) { + pm.callEvent(event); + } + } catch (Throwable ex) { + store.value = ex; + } + }}); + secondThread.start(); + secondThread.join(); + assertTrue(store.value instanceof IllegalStateException); + assertEquals(event.getEventName() + " cannot be triggered asynchronously from inside synchronized code.", ((Throwable) store.value).getMessage()); + } + + @Test + public void testAsyncUnlocked() throws InterruptedException { + final Event event = new TestEvent(true); + Thread secondThread = new Thread( + new Runnable() { + public void run() { + try { + pm.callEvent(event); + } catch (Throwable ex) { + store.value = ex; + } + }}); + secondThread.start(); + secondThread.join(); + if (store.value != null) { + throw new RuntimeException((Throwable) store.value); + } + } + + @Test + public void testSyncUnlocked() throws InterruptedException { + final Event event = new TestEvent(false); + Thread secondThread = new Thread( + new Runnable() { + public void run() { + try { + pm.callEvent(event); + } catch (Throwable ex) { + store.value = ex; + } + }}); + secondThread.start(); + secondThread.join(); + if (store.value != null) { + throw new RuntimeException((Throwable) store.value); + } + } + + @Test + public void testSyncLocked() throws InterruptedException { + final Event event = new TestEvent(false); + Thread secondThread = new Thread( + new Runnable() { + public void run() { + try { + synchronized (pm) { + pm.callEvent(event); + } + } catch (Throwable ex) { + store.value = ex; + } + }}); + secondThread.start(); + secondThread.join(); + if (store.value != null) { + throw new RuntimeException((Throwable) store.value); + } + } +} -- cgit v1.2.3