Improved the Scheduler.

Adds nag message when async tasks are not properly shut down and adds a limiter for sync tasks. Once they use 35ms in a single tick, any remaining tasks are not executed until later ticks. Adds a method to report the pending tasks and one to report active worker threads

4 files changed, 181 insertions, 40 deletions

diff --git a/src/main/java/org/bukkit/craftbukkit/CraftServer.java b/src/main/java/org/bukkit/craftbukkit/CraftServer.java
index fbf48c60..f28b7325 100644
--- a/src/main/java/org/bukkit/craftbukkit/CraftServer.java
+++ b/src/main/java/org/bukkit/craftbukkit/CraftServer.java
@@ -21,6 +21,7 @@ import java.util.Map;
 import java.util.Random;
 import java.util.logging.Level;
 import java.util.logging.Logger;
+import java.lang.InterruptedException;
 import jline.ConsoleReader;
 import net.minecraft.server.ChunkCoordinates;
 import net.minecraft.server.ConvertProgressUpdater;
@@ -45,6 +46,8 @@ import org.bukkit.craftbukkit.inventory.CraftFurnaceRecipe;
 import org.bukkit.craftbukkit.inventory.CraftRecipe;
 import org.bukkit.craftbukkit.inventory.CraftShapedRecipe;
 import org.bukkit.craftbukkit.inventory.CraftShapelessRecipe;
+import org.bukkit.scheduler.BukkitWorker;
+import org.bukkit.scheduler.BukkitTask;
 import org.bukkit.craftbukkit.scheduler.CraftScheduler;
 import org.bukkit.util.config.Configuration;
 
@@ -292,6 +295,32 @@ public final class CraftServer implements Server {
 
         pluginManager.clearPlugins();
         commandMap.clearCommands();
+
+        int pollCount = 0;
+
+        // Wait for at most 2.5 seconds for plugins to close their threads
+        while(pollCount < 50 && getScheduler().getActiveWorkers().size() > 0) {
+            try {
+                Thread.sleep(50);
+            } catch (InterruptedException e) {
+            }
+            pollCount++;
+        }
+
+        List<BukkitWorker> overdueWorkers = getScheduler().getActiveWorkers();
+        for(BukkitWorker worker : overdueWorkers) {
+            Plugin plugin = worker.getOwner();
+            String author = "<NoAuthorGiven>";
+            if (plugin.getDescription().getAuthors().size() > 0) {
+                author = plugin.getDescription().getAuthors().get(0);
+            }
+            getLogger().log(Level.SEVERE, String.format(
+                "Nag author: '%s' of '%s' about the following: %s",
+                author,
+                plugin.getDescription().getName(),
+                "This plugin is not properly shutting down its async tasks when it is being reloaded.  This may cause conflicts with the newly loaded version of the plugin"
+            ));
+        }
         loadPlugins();
     }
 
diff --git a/src/main/java/org/bukkit/craftbukkit/scheduler/CraftScheduler.java b/src/main/java/org/bukkit/craftbukkit/scheduler/CraftScheduler.java
index 9caef416..222a3158 100644
--- a/src/main/java/org/bukkit/craftbukkit/scheduler/CraftScheduler.java
+++ b/src/main/java/org/bukkit/craftbukkit/scheduler/CraftScheduler.java
@@ -3,6 +3,8 @@ package org.bukkit.craftbukkit.scheduler;
 import java.util.LinkedList;
 import java.util.TreeMap;
 import java.util.Iterator;
+import java.util.List;
+import java.util.ArrayList;
 
 import java.util.concurrent.locks.Lock;
 import java.util.concurrent.locks.ReentrantLock;
@@ -12,6 +14,9 @@ import java.util.logging.Level;
 import java.util.logging.Logger;
 
 import org.bukkit.scheduler.BukkitScheduler;
+import org.bukkit.scheduler.BukkitWorker;
+import org.bukkit.scheduler.BukkitTask;
+
 import org.bukkit.plugin.Plugin;
 
 import org.bukkit.craftbukkit.CraftServer;
@@ -35,6 +40,7 @@ public class CraftScheduler implements BukkitScheduler, Runnable {
 
     // This lock locks the mainThreadQueue and the currentTick value
     private final Lock mainThreadLock = new ReentrantLock();
+    private final Lock syncedTasksLock = new ReentrantLock();
 
     public void run() {
 
@@ -113,28 +119,33 @@ public class CraftScheduler implements BukkitScheduler, Runnable {
 
     // If the main thread cannot obtain the lock, it doesn't wait
     public void mainThreadHeartbeat(long currentTick) {
-        if (mainThreadLock.tryLock()) {
+        if (syncedTasksLock.tryLock()) {
             try {
-                this.currentTick = currentTick;
-                while (!mainThreadQueue.isEmpty()) {
-                    syncedTasks.addLast(mainThreadQueue.removeFirst());
+                if (mainThreadLock.tryLock()) {
+                    try {
+                        this.currentTick = currentTick;
+                        while (!mainThreadQueue.isEmpty()) {
+                            syncedTasks.addLast(mainThreadQueue.removeFirst());
+                        }
+                    } finally {
+                        mainThreadLock.unlock();
+                    }
                 }
-            } finally {
-                mainThreadLock.unlock();
-            }
-            while(!syncedTasks.isEmpty()) {
-                CraftTask task = syncedTasks.removeFirst();
-                try {
-                    task.getTask().run();
-                } catch (Throwable t) {
-                    // Bad plugin!
-                    logger.log(Level.WARNING, 
-                            "Task of '" + task.getOwner().getDescription().getName()
-                            + "' generated an exception", t);
-                    synchronized (schedulerQueue) {
-                        schedulerQueue.remove(task);
+                long breakTime = System.currentTimeMillis() + 35; // max time spent in loop = 35ms
+                while (!syncedTasks.isEmpty() && System.currentTimeMillis() <= breakTime) {
+                    CraftTask task = syncedTasks.removeFirst();
+                    try {
+                        task.getTask().run();
+                    } catch (Throwable t) {
+                        // Bad plugin!
+                        logger.log(Level.WARNING, "Task of '" + task.getOwner().getDescription().getName() + "' generated an exception", t);
+                        synchronized (schedulerQueue) {
+                            schedulerQueue.remove(task);
+                        }
                     }
                 }
+            } finally {
+                syncedTasksLock.unlock();
             }
         }
     }
@@ -159,6 +170,24 @@ public class CraftScheduler implements BukkitScheduler, Runnable {
         }
     }
 
+    void wipeSyncedTasks() {
+        syncedTasksLock.lock();
+        try {
+            syncedTasks.clear();
+        } finally {
+            syncedTasksLock.unlock();
+        }
+    }
+
+    void wipeMainThreadQueue() {
+        mainThreadLock.lock();
+        try {
+            mainThreadQueue.clear();
+        } finally {
+            mainThreadLock.unlock();
+        }
+    }
+
     public int scheduleSyncDelayedTask(Plugin plugin, Runnable task, long delay) {
         return scheduleSyncRepeatingTask(plugin, task, delay, -1);
     }
@@ -168,10 +197,16 @@ public class CraftScheduler implements BukkitScheduler, Runnable {
     }
 
     public int scheduleSyncRepeatingTask(Plugin plugin, Runnable task, long delay, long period) {
-        if (plugin == null) throw new IllegalArgumentException("Plugin can not null");
-        if (task == null) throw new IllegalArgumentException("Task can not null");
-        if (delay < 0) throw new IllegalArgumentException("Delay cannot be less than 0");
-        
+        if (plugin == null) {
+            throw new IllegalArgumentException("Plugin cannot be null");
+        }
+        if (task == null) {
+            throw new IllegalArgumentException("Task cannot be null");
+        }
+        if (delay < 0) {
+            throw new IllegalArgumentException("Delay cannot be less than 0");
+        }
+
         CraftTask newTask = new CraftTask(plugin, task, true, getCurrentTick()+delay, period);
         synchronized (schedulerQueue) {
             schedulerQueue.put(newTask, true);
@@ -189,10 +224,16 @@ public class CraftScheduler implements BukkitScheduler, Runnable {
     }
 
     public int scheduleAsyncRepeatingTask(Plugin plugin, Runnable task, long delay, long period) {
-        if (plugin == null) throw new IllegalArgumentException("Plugin can not null");
-        if (task == null) throw new IllegalArgumentException("Task can not null");
-        if (delay < 0) throw new IllegalArgumentException("Delay cannot be less than 0");
-        
+        if (plugin == null) {
+            throw new IllegalArgumentException("Plugin cannot be null");
+        }
+        if (task == null) {
+            throw new IllegalArgumentException("Task cannot be null");
+        }
+        if (delay < 0) {
+            throw new IllegalArgumentException("Delay cannot be less than 0");
+        }
+
         CraftTask newTask = new CraftTask(plugin, task, false, getCurrentTick()+delay, period);
         synchronized (schedulerQueue) {
             schedulerQueue.put(newTask, false);
@@ -203,7 +244,7 @@ public class CraftScheduler implements BukkitScheduler, Runnable {
 
     public <T> Future<T> callSyncMethod(Plugin plugin, Callable<T> task) {
         CraftFuture<T> craftFuture = new CraftFuture<T>(this, task);
-        synchronized(craftFuture) {
+        synchronized (craftFuture) {
             int taskId = scheduleSyncDelayedTask(plugin, craftFuture);
             craftFuture.setTaskId(taskId);
         }
@@ -224,15 +265,40 @@ public class CraftScheduler implements BukkitScheduler, Runnable {
     }
 
     public void cancelTasks(Plugin plugin) {
-        synchronized (schedulerQueue) {
-            Iterator<CraftTask> itr = schedulerQueue.keySet().iterator();
-            while (itr.hasNext()) {
-                CraftTask current = itr.next();
-                if (current.getOwner().equals(plugin)) {
-                    itr.remove();
+        syncedTasksLock.lock();
+        try {
+            synchronized(schedulerQueue) {
+                mainThreadLock.lock();
+                try {
+                    Iterator<CraftTask> itr = schedulerQueue.keySet().iterator();
+                    while (itr.hasNext()) {
+                        CraftTask current = itr.next();
+                        if (current.getOwner().equals(plugin)) {
+                            itr.remove();
+                        }
+                    }
+                    itr = mainThreadQueue.iterator();
+                    while (itr.hasNext()) {
+                        CraftTask current = itr.next();
+                        if (current.getOwner().equals(plugin)) {
+                            itr.remove();
+                        }
+                    }
+                    itr = syncedTasks.iterator();
+                    while (itr.hasNext()) {
+                        CraftTask current = itr.next();
+                        if (current.getOwner().equals(plugin)) {
+                            itr.remove();
+                        }
+                    }
+                } finally {
+                    mainThreadLock.unlock();
                 }
             }
+        } finally {
+            syncedTasksLock.unlock();
         }
+
         craftThreadManager.interruptTasks(plugin);
     }
 
@@ -240,6 +306,9 @@ public class CraftScheduler implements BukkitScheduler, Runnable {
         synchronized (schedulerQueue) {
             schedulerQueue.clear();
         }
+        wipeMainThreadQueue();
+        wipeSyncedTasks();
+
         craftThreadManager.interruptAllTasks();
     }
 
@@ -260,4 +329,40 @@ public class CraftScheduler implements BukkitScheduler, Runnable {
         }
     }
 
+    public List<BukkitWorker> getActiveWorkers() {
+        synchronized (craftThreadManager.workers) {
+            List<BukkitWorker> workerList = new ArrayList<BukkitWorker>(craftThreadManager.workers.size());
+            Iterator<CraftWorker> itr = craftThreadManager.workers.iterator();
+            while (itr.hasNext()) {
+                workerList.add((BukkitWorker)itr.next());
+            }
+            return workerList;
+        }
+    }
+
+    public List<BukkitTask> getPendingTasks() {
+        List<CraftTask> taskList = null;
+        syncedTasksLock.lock();
+        try {
+            synchronized (schedulerQueue) {
+                mainThreadLock.lock();
+                try {
+                    taskList = new ArrayList<CraftTask>(mainThreadQueue.size() + syncedTasks.size() + schedulerQueue.size());
+                    taskList.addAll(mainThreadQueue);
+                    taskList.addAll(syncedTasks);
+                    taskList.addAll(schedulerQueue.keySet());
+                } finally {
+                    mainThreadLock.unlock();
+                }
+            }
+        } finally {
+            syncedTasksLock.unlock();
+        }
+        List<BukkitTask> newTaskList = new ArrayList<BukkitTask>(taskList.size());
+        for (CraftTask craftTask : taskList) {
+            newTaskList.add((BukkitTask)craftTask);
+        }
+        return newTaskList;
+    }
+
 }
diff --git a/src/main/java/org/bukkit/craftbukkit/scheduler/CraftTask.java b/src/main/java/org/bukkit/craftbukkit/scheduler/CraftTask.java
index 7d5b610e..d5cc4f4e 100644
--- a/src/main/java/org/bukkit/craftbukkit/scheduler/CraftTask.java
+++ b/src/main/java/org/bukkit/craftbukkit/scheduler/CraftTask.java
@@ -3,8 +3,9 @@ package org.bukkit.craftbukkit.scheduler;
 import java.lang.Comparable;
 
 import org.bukkit.plugin.Plugin;
+import org.bukkit.scheduler.BukkitTask;
 
-public class CraftTask implements Comparable<Object> {
+public class CraftTask implements Comparable<Object>, BukkitTask {
 
     private final Runnable task;
     private final boolean syncTask;
@@ -44,7 +45,7 @@ public class CraftTask implements Comparable<Object> {
         return task;
     }
 
-    boolean isSync() {
+    public boolean isSync() {
         return syncTask;
     }
 
@@ -56,7 +57,7 @@ public class CraftTask implements Comparable<Object> {
         return period;
     }
 
-    Plugin getOwner() {
+    public Plugin getOwner() {
         return owner;
     }
 
@@ -64,6 +65,10 @@ public class CraftTask implements Comparable<Object> {
         executionTick += period;
     }
 
+    public int getTaskId() {
+        return getIdNumber();
+    }
+
     int getIdNumber() {
         return idNumber;
     }
@@ -104,6 +109,4 @@ public class CraftTask implements Comparable<Object> {
     public int hashCode() {
         return getIdNumber();
     }
-
-
 }
diff --git a/src/main/java/org/bukkit/craftbukkit/scheduler/CraftWorker.java b/src/main/java/org/bukkit/craftbukkit/scheduler/CraftWorker.java
index 3bf91e4f..42f9bb9b 100644
--- a/src/main/java/org/bukkit/craftbukkit/scheduler/CraftWorker.java
+++ b/src/main/java/org/bukkit/craftbukkit/scheduler/CraftWorker.java
@@ -1,8 +1,9 @@
 package org.bukkit.craftbukkit.scheduler;
 
 import org.bukkit.plugin.Plugin;
+import org.bukkit.scheduler.BukkitWorker;
 
-public class CraftWorker implements Runnable {
+public class CraftWorker implements Runnable, BukkitWorker {
 
     private static int hashIdCounter = 1;
     private static Object hashIdCounterSync = new Object();
@@ -49,6 +50,10 @@ public class CraftWorker implements Runnable {
         return owner;
     }
 
+    public Thread getThread() {
+        return t;
+    }
+
     public void interrupt() {
         t.interrupt();
     }
@@ -70,7 +75,6 @@ public class CraftWorker implements Runnable {
 
     @Override
     public boolean equals( Object other ) {
-
         if (other == null) {
             return false;
         }