fix(Task): Cancelled Task shouldn't start running #4311 (WIP)

Foundation/include/Poco/Task.h

@@ -77,6 +77,8 @@ class Foundation_API Task: public Runnable, public RefCountedObject
 		/// A Task's runTask() method should periodically
 		/// call this method and stop whatever it is doing in an
 		/// orderly way when this method returns true.
+		/// If task is cancelled before it had a chance to run,
+		/// runTask() will never be called.
 
 	TaskState state() const;
 		/// Returns the task's current state.
@@ -90,8 +92,11 @@ class Foundation_API Task: public Runnable, public RefCountedObject
 		/// be overridden by subclasses.
 
 	void run();
-		/// Calls the task's runTask() method and notifies the owner
-		/// of the task's start and completion.
+		/// If task has not been cancelled prior to this call, it
+		/// calls the task's runTask() method and notifies the owner of
+		/// the task's start and completion.
+		/// If task has been cancelled prior to this call, it only sets
+		/// the state to TASK_FINISHED and notifies the owner.
 
 protected:
 	bool sleep(long milliseconds);
@@ -134,7 +139,7 @@ class Foundation_API Task: public Runnable, public RefCountedObject
 	TaskManager* getOwner() const;
 		/// Returns the owner of the task, which may be NULL.
 
-	void setState(TaskState state);
+	TaskState setState(TaskState state);
 		/// Sets the task's state.
 
 	virtual ~Task();

Foundation/include/Poco/TaskManager.h

@@ -65,9 +65,16 @@ class Foundation_API TaskManager
 	~TaskManager();
 		/// Destroys the TaskManager.
 
-	void start(Task* pTask);
+	bool start(Task* pTask);
 		/// Starts the given task in a thread obtained
-		/// from the thread pool.
+		/// from the thread pool; returns true if successful.
+		///
+		/// If this method returns false, the task was cancelled
+		/// before it could be started, or it was already running;
+		/// in any case, a false return means refusal of ownership
+		/// and indicates that the task pointer may not be valid
+		/// anymore (it will only be valid if it was duplicated
+		/// prior to this call.
 		///
 		/// The TaskManager takes ownership of the Task object
 		/// and deletes it when it is finished.

Foundation/src/Task.cpp

@@ -56,27 +56,29 @@ void Task::reset()
 void Task::run()
 {
 	TaskManager* pOwner = getOwner();
-	if (pOwner)
-		pOwner->taskStarted(this);
-	try
-	{
-		_state = TASK_RUNNING;
-		runTask();
-	}
-	catch (Exception& exc)
-	{
-		if (pOwner)
-			pOwner->taskFailed(this, exc);
-	}
-	catch (std::exception& exc)
-	{
-		if (pOwner)
-			pOwner->taskFailed(this, SystemException("Task::run()", exc.what()));
-	}
-	catch (...)
+	if (_state.exchange(TASK_RUNNING) < TASK_RUNNING)
 	{
 		if (pOwner)
-			pOwner->taskFailed(this, SystemException("Task::run(): unknown exception"));
+			pOwner->taskStarted(this);
+		try
+		{
+			runTask();
+		}
+		catch (Exception& exc)
+		{
+			if (pOwner)
+				pOwner->taskFailed(this, exc);
+		}
+		catch (std::exception& exc)
+		{
+			if (pOwner)
+				pOwner->taskFailed(this, SystemException("Task::run()", exc.what()));
+		}
+		catch (...)
+		{
+			if (pOwner)
+				pOwner->taskFailed(this, SystemException("Task::run(): unknown exception"));
+		}
 	}
 	_state = TASK_FINISHED;
 	if (pOwner) pOwner->taskFinished(this);
@@ -114,9 +116,9 @@ void Task::setOwner(TaskManager* pOwner)
 }
 
 
-void Task::setState(TaskState state)
+Task::TaskState Task::setState(TaskState state)
 {
-	_state = state;
+	return _state.exchange(state);
 }
 
 

Foundation/src/TaskManager.cpp

@@ -52,26 +52,32 @@ TaskManager::~TaskManager()
 }
 
 
-void TaskManager::start(Task* pTask)
+bool TaskManager::start(Task* pTask)
 {
 	TaskPtr pAutoTask(pTask); // take ownership immediately
-	pAutoTask->setOwner(this);
-	pAutoTask->setState(Task::TASK_STARTING);
+	pTask->setOwner(this);
+	Task::TaskState prevState = pTask->setState(Task::TASK_STARTING);
 
 	ScopedLockT lock(_mutex);
-	_taskList.push_back(pAutoTask);
 	try
 	{
-		_threadPool.start(*pAutoTask, pAutoTask->name());
+		// do not start cancelled or running tasks
+		if ((prevState <= Task::TASK_STARTING) || (prevState > Task::TASK_RUNNING))
+		{
+			_threadPool.start(*pTask, pTask->name());
+			_taskList.push_back(pAutoTask);
+			return true;
+		}
 	}
 	catch (...)
 	{
-		// Make sure that we don't act like we own the task since
-		// we never started it.  If we leave the task on our task
-		// list, the size of the list is incorrect.
-		_taskList.pop_back();
+		pTask->setOwner(nullptr);
 		throw;
 	}
+
+	pTask->setState(Task::TASK_FINISHED);
+	pTask->setOwner(nullptr);
+	return false;
 }
 
 

Foundation/testsuite/src/TaskManagerTest.cpp

@@ -22,6 +22,7 @@
 #include "Poco/Observer.h"
 #include "Poco/Exception.h"
 #include "Poco/AutoPtr.h"
+#include <iostream>
 
 
 using Poco::TaskManager;
@@ -51,12 +52,14 @@ namespace
 	public:
 		TestTask():
 			Task("TestTask"),
-			_fail(false)
+			_fail(false),
+			_started(false)
 		{
 		}
 
 		void runTask()
 		{
+			_started = true;
 			_event.wait();
 			setProgress(0.5);
 			_event.wait();
@@ -78,9 +81,15 @@ namespace
 			_event.set();
 		}
 
+		bool started() const
+		{
+			return _started;
+		}
+
 	private:
 		Event _event;
-		bool  _fail;
+		std::atomic<bool> _fail;
+		std::atomic<bool> _started;
 	};
 
 	class SimpleTask: public Task
@@ -277,6 +286,11 @@ void TaskManagerTest::testFinish()
 	assertTrue (!to.error());
 	tm.cancelAll();
 	tm.joinAll();
+	tm.removeObserver(Observer<TaskObserver, TaskStartedNotification>(to, &TaskObserver::taskStarted));
+	tm.removeObserver(Observer<TaskObserver, TaskCancelledNotification>(to, &TaskObserver::taskCancelled));
+	tm.removeObserver(Observer<TaskObserver, TaskFailedNotification>(to, &TaskObserver::taskFailed));
+	tm.removeObserver(Observer<TaskObserver, TaskFinishedNotification>(to, &TaskObserver::taskFinished));
+	tm.removeObserver(Observer<TaskObserver, TaskProgressNotification>(to, &TaskObserver::taskProgress));
 }
 
 
@@ -317,6 +331,11 @@ void TaskManagerTest::testCancel()
 	assertTrue (!to.error());
 	tm.cancelAll();
 	tm.joinAll();
+	tm.removeObserver(Observer<TaskObserver, TaskStartedNotification>(to, &TaskObserver::taskStarted));
+	tm.removeObserver(Observer<TaskObserver, TaskCancelledNotification>(to, &TaskObserver::taskCancelled));
+	tm.removeObserver(Observer<TaskObserver, TaskFailedNotification>(to, &TaskObserver::taskFailed));
+	tm.removeObserver(Observer<TaskObserver, TaskFinishedNotification>(to, &TaskObserver::taskFinished));
+	tm.removeObserver(Observer<TaskObserver, TaskProgressNotification>(to, &TaskObserver::taskProgress));
 }
 
 
@@ -330,7 +349,7 @@ void TaskManagerTest::testError()
 	tm.addObserver(Observer<TaskObserver, TaskFinishedNotification>(to, &TaskObserver::taskFinished));
 	tm.addObserver(Observer<TaskObserver, TaskProgressNotification>(to, &TaskObserver::taskProgress));
 	AutoPtr<TestTask> pTT = new TestTask;
-	tm.start(pTT.duplicate());
+	assertTrue (tm.start(pTT.duplicate()));
 	while (pTT->state() < Task::TASK_RUNNING) Thread::sleep(50);
 	assertTrue (pTT->progress() == 0);
 	Thread::sleep(200);
@@ -356,6 +375,11 @@ void TaskManagerTest::testError()
 	assertTrue (list.empty());
 	tm.cancelAll();
 	tm.joinAll();
+	tm.removeObserver(Observer<TaskObserver, TaskStartedNotification>(to, &TaskObserver::taskStarted));
+	tm.removeObserver(Observer<TaskObserver, TaskCancelledNotification>(to, &TaskObserver::taskCancelled));
+	tm.removeObserver(Observer<TaskObserver, TaskFailedNotification>(to, &TaskObserver::taskFailed));
+	tm.removeObserver(Observer<TaskObserver, TaskFinishedNotification>(to, &TaskObserver::taskFinished));
+	tm.removeObserver(Observer<TaskObserver, TaskProgressNotification>(to, &TaskObserver::taskProgress));
 }
 
 
@@ -443,6 +467,30 @@ void TaskManagerTest::testCustom()
 }
 
 
+void TaskManagerTest::testCancelNoStart()
+{
+	std::cout << "TODO";
+	/*
+	TaskManager tm;
+	TaskObserver to;
+	tm.addObserver(Observer<TaskObserver, TaskStartedNotification>(to, &TaskObserver::taskStarted));
+	tm.addObserver(Observer<TaskObserver, TaskCancelledNotification>(to, &TaskObserver::taskCancelled));
+	tm.addObserver(Observer<TaskObserver, TaskFailedNotification>(to, &TaskObserver::taskFailed));
+	tm.addObserver(Observer<TaskObserver, TaskFinishedNotification>(to, &TaskObserver::taskFinished));
+	tm.addObserver(Observer<TaskObserver, TaskProgressNotification>(to, &TaskObserver::taskProgress));
+	AutoPtr<TestTask> pTT = new TestTask;
+	pTT->cancel();
+	assertFalse(tm.start(pTT.duplicate()));
+	while (pTT->state() < Task::TASK_FINISHED) Thread::sleep(50);
+	assertTrue (pTT->progress() == 0);
+	tm.removeObserver(Observer<TaskObserver, TaskStartedNotification>(to, &TaskObserver::taskStarted));
+	tm.removeObserver(Observer<TaskObserver, TaskCancelledNotification>(to, &TaskObserver::taskCancelled));
+	tm.removeObserver(Observer<TaskObserver, TaskFailedNotification>(to, &TaskObserver::taskFailed));
+	tm.removeObserver(Observer<TaskObserver, TaskFinishedNotification>(to, &TaskObserver::taskFinished));
+	tm.removeObserver(Observer<TaskObserver, TaskProgressNotification>(to, &TaskObserver::taskProgress));
+*/}
+
+
 void TaskManagerTest::testMultiTasks()
 {
 	TaskManager tm;
@@ -510,6 +558,7 @@ CppUnit::Test* TaskManagerTest::suite()
 	CppUnit_addTest(pSuite, TaskManagerTest, testFinish);
 	CppUnit_addTest(pSuite, TaskManagerTest, testCancel);
 	CppUnit_addTest(pSuite, TaskManagerTest, testError);
+	CppUnit_addTest(pSuite, TaskManagerTest, testCancelNoStart);
 	CppUnit_addTest(pSuite, TaskManagerTest, testMultiTasks);
 	CppUnit_addTest(pSuite, TaskManagerTest, testCustom);
 	CppUnit_addTest(pSuite, TaskManagerTest, testCustomThreadPool);

Foundation/testsuite/src/TaskManagerTest.h

@@ -33,6 +33,7 @@ class TaskManagerTest: public CppUnit::TestCase
 	void testFinish();
 	void testCancel();
 	void testError();
+	void testCancelNoStart();
 	void testCustom();
 	void testMultiTasks();
 	void testCustomThreadPool();

Foundation/testsuite/src/TaskTest.cpp

@@ -29,12 +29,14 @@ namespace
 	class TestTask: public Task
 	{
 	public:
-		TestTask(): Task("TestTask")
+		TestTask(): Task("TestTask"),
+			_started(false)
 		{
 		}
 
 		void runTask()
 		{
+			_started = true;
 			try
 			{
 				_event.wait();
@@ -73,8 +75,14 @@ namespace
 			}
 		}
 
+		bool started() const
+		{
+			return _started;
+		}
+
 	private:
 		Event _event;
+		std::atomic<bool> _started;
 	};
 }
 
@@ -142,6 +150,21 @@ void TaskTest::testCancel2()
 }
 
 
+void TaskTest::testCancelNoStart()
+{
+	AutoPtr<TestTask> pTT = new TestTask;
+	assertTrue (pTT->state() == Task::TASK_IDLE);
+	pTT->cancel();
+	assertTrue (pTT->state() == Task::TASK_CANCELLING);
+	Thread thr;
+	thr.start(*pTT);
+	while (pTT->state() != Task::TASK_FINISHED)
+		Thread::sleep(50);
+	assertTrue (pTT->state() == Task::TASK_FINISHED);
+	assertFalse (pTT->started());
+}
+
+
 void TaskTest::setUp()
 {
 }
@@ -159,6 +182,7 @@ CppUnit::Test* TaskTest::suite()
 	CppUnit_addTest(pSuite, TaskTest, testFinish);
 	CppUnit_addTest(pSuite, TaskTest, testCancel1);
 	CppUnit_addTest(pSuite, TaskTest, testCancel2);
+	CppUnit_addTest(pSuite, TaskTest, testCancelNoStart);
 
 	return pSuite;
 }

Foundation/testsuite/src/TaskTest.h

@@ -27,6 +27,7 @@ class TaskTest: public CppUnit::TestCase
 	void testFinish();
 	void testCancel1();
 	void testCancel2();
+	void testCancelNoStart();
 
 	void setUp();
 	void tearDown();