Fixed some async tasks running synchronously. Fixes BUKKIT-2934

Additionally refactored cancel method to be more object-oriented.

4 files changed, 37 insertions, 39 deletions

diff --git a/src/main/java/org/bukkit/craftbukkit/scheduler/CraftAsyncTask.java b/src/main/java/org/bukkit/craftbukkit/scheduler/CraftAsyncTask.java
index 6901e4ee..75a1a63a 100644
--- a/src/main/java/org/bukkit/craftbukkit/scheduler/CraftAsyncTask.java
+++ b/src/main/java/org/bukkit/craftbukkit/scheduler/CraftAsyncTask.java
@@ -95,4 +95,12 @@ class CraftAsyncTask extends CraftTask {
     LinkedList<BukkitWorker> getWorkers() {
         return workers;
     }
+
+    boolean cancel0() {
+        synchronized (workers) {
+            // Synchronizing here prevents race condition for a completing task
+            setPeriod(-2l);
+        }
+        return true;
+    }
 }
diff --git a/src/main/java/org/bukkit/craftbukkit/scheduler/CraftFuture.java b/src/main/java/org/bukkit/craftbukkit/scheduler/CraftFuture.java
index fee49c92..1baec564 100644
--- a/src/main/java/org/bukkit/craftbukkit/scheduler/CraftFuture.java
+++ b/src/main/java/org/bukkit/craftbukkit/scheduler/CraftFuture.java
@@ -96,4 +96,13 @@ class CraftFuture<T> extends CraftTask implements Future<T> {
             }
         }
     }
+
+    synchronized boolean cancel0() {
+        if (getPeriod() != -1l) {
+            return false;
+        }
+        setPeriod(-2l);
+        notifyAll();
+        return true;
+    }
 }
diff --git a/src/main/java/org/bukkit/craftbukkit/scheduler/CraftScheduler.java b/src/main/java/org/bukkit/craftbukkit/scheduler/CraftScheduler.java
index 15b15dbf..c10008fc 100644
--- a/src/main/java/org/bukkit/craftbukkit/scheduler/CraftScheduler.java
+++ b/src/main/java/org/bukkit/craftbukkit/scheduler/CraftScheduler.java
@@ -108,7 +108,7 @@ public class CraftScheduler implements BukkitScheduler {
     }
 
     public BukkitTask runTaskLaterAsynchronously(Plugin plugin, Runnable runnable, long delay) {
-        return runTaskTimer(plugin, runnable, delay, -1l);
+        return runTaskTimerAsynchronously(plugin, runnable, delay, -1l);
     }
 
     public int scheduleSyncRepeatingTask(final Plugin plugin, final Runnable runnable, long delay, long period) {
@@ -158,7 +158,7 @@ public class CraftScheduler implements BukkitScheduler {
         }
         CraftTask task = runners.get(taskId);
         if (task != null) {
-            cancelTask(task);
+            task.cancel0();
         }
         task = new CraftTask(
                 new Runnable() {
@@ -172,7 +172,7 @@ public class CraftScheduler implements BukkitScheduler {
                         while (tasks.hasNext()) {
                             final CraftTask task = tasks.next();
                             if (task.getTaskId() == taskId) {
-                                cancelTask(task);
+                                task.cancel0();
                                 tasks.remove();
                                 if (task.isSync()) {
                                     runners.remove(taskId);
@@ -188,7 +188,7 @@ public class CraftScheduler implements BukkitScheduler {
                 return;
             }
             if (taskPending.getTaskId() == taskId) {
-                cancelTask(taskPending);
+                taskPending.cancel0();
             }
         }
     }
@@ -206,7 +206,7 @@ public class CraftScheduler implements BukkitScheduler {
                         while (tasks.hasNext()) {
                             final CraftTask task = tasks.next();
                             if (task.getOwner().equals(plugin)) {
-                                cancelTask(task);
+                                task.cancel0();
                                 tasks.remove();
                                 if (task.isSync()) {
                                     runners.remove(task.getTaskId());
@@ -222,12 +222,12 @@ public class CraftScheduler implements BukkitScheduler {
                 return;
             }
             if (taskPending.getTaskId() != -1 && taskPending.getOwner().equals(plugin)) {
-                cancelTask(taskPending);
+                taskPending.cancel0();
             }
         }
         for (CraftTask runner : runners.values()) {
             if (runner.getOwner().equals(plugin)) {
-                cancelTask(runner);
+                runner.cancel0();
             }
         }
     }
@@ -239,7 +239,7 @@ public class CraftScheduler implements BukkitScheduler {
                         Iterator<CraftTask> it = CraftScheduler.this.runners.values().iterator();
                         while (it.hasNext()) {
                             CraftTask task = it.next();
-                            cancelTask(task);
+                            task.cancel0();
                             if (task.isSync()) {
                                 it.remove();
                             }
@@ -253,10 +253,10 @@ public class CraftScheduler implements BukkitScheduler {
             if (taskPending == task) {
                 break;
             }
-            cancelTask(taskPending);
+            taskPending.cancel0();
         }
         for (CraftTask runner : runners.values()) {
-            cancelTask(runner);
+            runner.cancel0();
         }
     }
 
@@ -421,35 +421,6 @@ public class CraftScheduler implements BukkitScheduler {
         return !pending.isEmpty() && pending.peek().getNextRun() <= currentTick;
     }
 
-    /**
-     * This method is important to make sure the code is consistent everywhere.
-     * Synchronizing is needed for future and async to prevent race conditions,
-     * main thread or otherwise.
-     * @return True if cancelled
-     */
-    private boolean cancelTask(final CraftTask task) {
-        if (task.isSync()) {
-            if (task instanceof CraftFuture) {
-                synchronized (task) {
-                    if (task.getPeriod() != -1l) {
-                        return false;
-                    }
-                    // This needs to be set INSIDE of the synchronized block
-                    task.setPeriod(-2l);
-                    task.notifyAll();
-                }
-            } else {
-                task.setPeriod(-2l);
-            }
-        } else {
-            synchronized (((CraftAsyncTask) task).getWorkers()) {
-                // Synchronizing here prevents race condition for a completing task
-                task.setPeriod(-2l);
-            }
-        }
-        return true;
-    }
-
     @Override
     public String toString() {
         int debugTick = currentTick;
diff --git a/src/main/java/org/bukkit/craftbukkit/scheduler/CraftTask.java b/src/main/java/org/bukkit/craftbukkit/scheduler/CraftTask.java
index 4dd1cb29..55db3ff8 100644
--- a/src/main/java/org/bukkit/craftbukkit/scheduler/CraftTask.java
+++ b/src/main/java/org/bukkit/craftbukkit/scheduler/CraftTask.java
@@ -84,4 +84,14 @@ class CraftTask implements BukkitTask, Runnable {
     public void cancel() {
         Bukkit.getScheduler().cancelTask(id);
     }
+
+    /**
+     * This method properly sets the status to cancelled, synchronizing when required.
+     *
+     * @return false if it is a craft future task that has already begun execution, true otherwise
+     */
+    boolean cancel0() {
+        setPeriod(-2l);
+        return true;
+    }
 }