Fixed Timer tasks being run after cancellation.

CHANGES

@@ -13,6 +13,7 @@
 - Fix: Keep SelectBox popup from extending past right edge of stage.
 - Added Framebuffer multisample support (see GL31FrameBufferMultisampleTest.java for basic usage)
 - Fix: Fonts generated with gdx-freetype no longer bleed when drawn with a shadow
+- Fixed Timer tasks being run after cancellation.
 
 [1.12.1]
 - LWJGL3 Improvement: Audio device is automatically switched if it was changed in the operating system.

gdx/src/com/badlogic/gdx/utils/Timer.java

@@ -27,6 +27,7 @@ public class Timer {
 	// TimerThread access is synchronized using threadLock.
 	// Timer access is synchronized using the Timer instance.
 	// Task access is synchronized using the Task instance.
+	// Posted tasks are synchronized using TimerThread#postedTasks.
 
 	static final Object threadLock = new Object();
 	static TimerThread thread;
@@ -118,15 +119,20 @@ public void start () {
 	}
 
 	/** Cancels all tasks. */
-	public synchronized void clear () {
-		for (int i = 0, n = tasks.size; i < n; i++) {
-			Task task = tasks.get(i);
-			synchronized (task) {
-				task.executeTimeMillis = 0;
-				task.timer = null;
+	public void clear () {
+		synchronized (threadLock) {
+			TimerThread thread = thread();
+			synchronized (this) {
+				synchronized (thread.postedTasks) {
+					for (int i = 0, n = tasks.size; i < n; i++) {
+						Task task = tasks.get(i);
+						thread.removePostedTask(task);
+						task.reset();
+					}
+				}
+				tasks.clear();
 			}
 		}
-		tasks.clear();
 	}
 
 	/** Returns true if the timer has no tasks in the queue. Note that this can change at any time. Synchronize on the timer
@@ -135,7 +141,7 @@ public synchronized boolean isEmpty () {
 		return tasks.size == 0;
 	}
 
-	synchronized long update (long timeMillis, long waitMillis) {
+	synchronized long update (TimerThread thread, long timeMillis, long waitMillis) {
 		for (int i = 0, n = tasks.size; i < n; i++) {
 			Task task = tasks.get(i);
 			synchronized (task) {
@@ -153,7 +159,7 @@ synchronized long update (long timeMillis, long waitMillis) {
 					waitMillis = Math.min(waitMillis, task.intervalMillis);
 					if (task.repeatCount > 0) task.repeatCount--;
 				}
-				task.app.postRunnable(task);
+				thread.addPostedTask(task);
 			}
 		}
 		return waitMillis;
@@ -212,23 +218,24 @@ public Task () {
 
 		/** Cancels the task. It will not be executed until it is scheduled again. This method can be called at any time. */
 		public void cancel () {
-			Timer timer = this.timer;
-			if (timer != null) {
-				synchronized (timer) {
-					synchronized (this) {
-						executeTimeMillis = 0;
-						this.timer = null;
+			synchronized (threadLock) {
+				thread().removePostedTask(this);
+				Timer timer = this.timer;
+				if (timer != null) {
+					synchronized (timer) {
 						timer.tasks.removeValue(this, true);
+						reset();
 					}
-				}
-			} else {
-				synchronized (this) {
-					executeTimeMillis = 0;
-					this.timer = null;
-				}
+				} else
+					reset();
 			}
 		}
 
+		synchronized void reset () {
+			executeTimeMillis = 0;
+			this.timer = null;
+		}
+
 		/** Returns true if this task is scheduled to be executed in the future by a timer. The execution time may be reached at any
 		 * time after calling this method, which may change the scheduled state. To prevent the scheduled state from changing,
 		 * synchronize on this task object, eg:
@@ -258,6 +265,14 @@ static class TimerThread implements Runnable, LifecycleListener {
 		Timer instance;
 		long pauseTimeMillis;
 
+		final Array<Task> postedTasks = new Array(2);
+		final Array<Task> runTasks = new Array(2);
+		private final Runnable runPostedTasks = new Runnable() {
+			public void run () {
+				runPostedTasks();
+			}
+		};
+
 		public TimerThread () {
 			files = Gdx.files;
 			app = Gdx.app;
@@ -279,7 +294,7 @@ public void run () {
 						long timeMillis = System.nanoTime() / 1000000;
 						for (int i = 0, n = instances.size; i < n; i++) {
 							try {
-								waitMillis = instances.get(i).update(timeMillis, waitMillis);
+								waitMillis = instances.get(i).update(this, timeMillis, waitMillis);
 							} catch (Throwable ex) {
 								throw new GdxRuntimeException("Task failed: " + instances.get(i).getClass().getName(), ex);
 							}
@@ -297,6 +312,32 @@ public void run () {
 			dispose();
 		}
 
+		void runPostedTasks () {
+			synchronized (postedTasks) {
+				runTasks.addAll(postedTasks);
+				postedTasks.clear();
+			}
+			Object[] items = runTasks.items;
+			for (int i = 0, n = runTasks.size; i < n; i++)
+				((Task)items[i]).run();
+			runTasks.clear();
+		}
+
+		void addPostedTask (Task task) {
+			synchronized (postedTasks) {
+				if (postedTasks.isEmpty()) task.app.postRunnable(runPostedTasks);
+				postedTasks.add(task);
+			}
+		}
+
+		void removePostedTask (Task task) {
+			synchronized (postedTasks) {
+				Object[] items = postedTasks.items;
+				for (int i = postedTasks.size - 1; i >= 0; i--)
+					if (items[i] == task) postedTasks.removeIndex(i);
+			}
+		}
+
 		public void resume () {
 			synchronized (threadLock) {
 				long delayMillis = System.nanoTime() / 1000000 - pauseTimeMillis;
@@ -316,6 +357,9 @@ public void pause () {
 
 		public void dispose () { // OK to call multiple times.
 			synchronized (threadLock) {
+				synchronized (postedTasks) {
+					postedTasks.clear();
+				}
 				if (thread == this) thread = null;
 				instances.clear();
 				threadLock.notifyAll();

tests/gdx-tests/src/com/badlogic/gdx/tests/TimerTest.java

@@ -22,16 +22,192 @@
 import com.badlogic.gdx.utils.Timer.Task;
 
 public class TimerTest extends GdxTest {
+	static final int expectedTests = 10;
+
 	@Override
 	public void create () {
+		Gdx.app.log("TimerTest", "Starting tests...");
+		testOnce();
+		testRepeat();
+		testCancelPosted();
+		testCancelPostedTwice();
+		testCancelPostedReschedule();
+		testStop();
+		testStopStart();
+		testDelay();
+		testClear();
+	}
+
+	float time;
+	int testCount;
+	Task repeatTask;
+
+	public void render () {
+		if (time < 2) {
+			time += Gdx.graphics.getDeltaTime();
+			if (time >= 2) {
+				if (testCount != expectedTests)
+					throw new RuntimeException("Some tests did not run: " + testCount + " != " + expectedTests);
+				Gdx.app.log("TimerTest", "SUCCESS! " + testCount + " tests passed.");
+				repeatTask.cancel();
+			}
+		}
+	}
+
+	void testOnce () {
+		Task task = Timer.schedule(new Task() {
+			@Override
+			public void run () {
+				Gdx.app.log("TimerTest", "testOnce");
+				testCount++;
+			}
+		}, 1);
+		assertScheduled(task);
+	}
+
+	void testRepeat () {
+		repeatTask = Timer.schedule(new Task() {
+			int count;
+
+			@Override
+			public void run () {
+				Gdx.app.log("TimerTest", "testRepeat");
+				if (++count <= 2) testCount++;
+			}
+		}, 1, 1);
+		assertScheduled(repeatTask);
+	}
+
+	void testCancelPosted () {
+		Task task = Timer.schedule(new Task() {
+			@Override
+			public void run () {
+				throw new RuntimeException("Cancelled task should not run.");
+			}
+		}, 0.01f);
+		assertScheduled(task);
+		sleep(200); // Sleep so the task is posted.
+		assertNotScheduled(task);
+		task.cancel(); // The posted task should not execute.
+		assertNotScheduled(task);
+		Gdx.app.log("TimerTest", "testCancelPosted");
+		testCount++;
+	}
+
+	void testCancelPostedTwice () {
+		Task task = new Task() {
+			@Override
+			public void run () {
+				throw new RuntimeException("Cancelled task should not run.");
+			}
+		};
+		Timer.schedule(task, 0.01f);
+		assertScheduled(task);
+		sleep(200); // Sleep so the task is posted.
+		assertNotScheduled(task);
+		Timer.schedule(task, 0.01f);
+		assertScheduled(task);
+		sleep(200); // Sleep so the task is posted.
+		assertNotScheduled(task);
+		task.cancel(); // The twice posted task should not execute.
+		assertNotScheduled(task);
+		Gdx.app.log("TimerTest", "testCancelPostedTwice");
+		testCount++;
+	}
+
+	void testCancelPostedReschedule () {
+		Task task = Timer.schedule(new Task() {
+			int count;
+
+			@Override
+			public void run () {
+				if (++count != 1) throw new RuntimeException("Rescheduled task should only run once.");
+				Gdx.app.log("TimerTest", "testCancelPostedReschedule");
+				testCount++;
+			}
+		}, 0.01f);
+		assertScheduled(task);
+		sleep(200); // Sleep so the task is posted.
+		assertNotScheduled(task);
+		task.cancel(); // The posted task should not execute.
+		assertNotScheduled(task);
+		Timer.schedule(task, 0.01f); // Schedule it again.
+		assertScheduled(task);
+	}
+
+	void testStop () {
 		Timer timer = new Timer();
 		Task task = timer.scheduleTask(new Task() {
 			@Override
 			public void run () {
-				Gdx.app.log("TimerTest", "ping");
+				throw new RuntimeException("Stopped timer should not run tasks.");
 			}
-		}, 1, 1);
+		}, 1);
+		assertScheduled(task);
+		timer.stop();
+		Gdx.app.log("TimerTest", "testStop");
+		testCount++;
+	}
+
+	void testStopStart () {
+		Timer timer = new Timer();
+		Task task = timer.scheduleTask(new Task() {
+			@Override
+			public void run () {
+				Gdx.app.log("TimerTest", "testStopStart");
+				testCount++;
+			}
+		}, 0.200f);
+		assertScheduled(task);
+		timer.stop();
+		sleep(300); // Sleep longer than task delay while stopped.
+		timer.start();
+		sleep(100);
+		assertScheduled(task); // Shouldn't have happened yet.
+	}
+
+	void testDelay () {
+		Timer timer = new Timer();
+		Task task = timer.scheduleTask(new Task() {
+			@Override
+			public void run () {
+				Gdx.app.log("TimerTest", "testDelay");
+				testCount++;
+			}
+		}, 0.200f);
+		assertScheduled(task);
+		timer.delay(200);
+		sleep(300); // Sleep longer than task delay.
+		assertScheduled(task); // Shouldn't have happened yet.
+	}
+
+	void testClear () {
+		Timer timer = new Timer();
+		Task task = timer.scheduleTask(new Task() {
+			@Override
+			public void run () {
+				throw new RuntimeException("Cleared timer should not run tasks.");
+			}
+		}, 0.200f);
+		assertScheduled(task);
+		timer.clear();
+		assertNotScheduled(task);
+		Gdx.app.log("TimerTest", "testClear");
+		testCount++;
+	}
+
+	void assertScheduled (Task task) {
+		if (!task.isScheduled()) throw new RuntimeException("Should be scheduled.");
+	}
+
+	void assertNotScheduled (Task task) {
+		if (task.isScheduled()) throw new RuntimeException("Should not be scheduled.");
+	}
 
-		Gdx.app.log("TimerTest", "is task scheduled: " + String.valueOf(task.isScheduled()));
+	void sleep (long millis) {
+		try {
+			Thread.sleep(millis);
+		} catch (InterruptedException ignored) {
+		}
 	}
 }