fix(TaskManager): task ownership #4311

Foundation/include/Poco/Task.h

@@ -98,6 +98,8 @@ class Foundation_API Task: public Runnable, public RefCountedObject
 		/// If task has been cancelled prior to this call, it only sets
 		/// the state to TASK_FINISHED and notifies the owner.
 
+	bool hasOwner() const;
+		/// Returns true iff the task has an owner.
 protected:
 	bool sleep(long milliseconds);
 		/// Suspends the current thread for the specified
@@ -150,12 +152,12 @@ class Foundation_API Task: public Runnable, public RefCountedObject
 	Task(const Task&);
 	Task& operator = (const Task&);
 
-	std::string            _name;
-	TaskManager*           _pOwner;
-	std::atomic<float>     _progress;
-	std::atomic<TaskState> _state;
-	Event                  _cancelEvent;
-	mutable FastMutex      _mutex;
+	std::string               _name;
+	std::atomic<TaskManager*> _pOwner;
+	std::atomic<float>        _progress;
+	std::atomic<TaskState>    _state;
+	Event                     _cancelEvent;
+	mutable FastMutex         _mutex;
 
 	friend class TaskManager;
 };
@@ -190,12 +192,16 @@ inline Task::TaskState Task::state() const
 
 inline TaskManager* Task::getOwner() const
 {
-	FastMutex::ScopedLock lock(_mutex);
-
 	return _pOwner;
 }
 
 
+inline bool Task::hasOwner() const
+{
+	return _pOwner != nullptr;
+}
+
+
 } // namespace Poco
 
 

Foundation/include/Poco/TaskManager.h

@@ -74,7 +74,7 @@ class Foundation_API TaskManager
 		/// 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.
+		/// prior to this call).
 		///
 		/// The TaskManager takes ownership of the Task object
 		/// and deletes it when it is finished.

Foundation/src/Task.cpp

@@ -41,7 +41,7 @@ void Task::cancel()
 	_state = TASK_CANCELLING;
 	_cancelEvent.set();
 	if (_pOwner)
-		_pOwner->taskCancelled(this);
+		_pOwner.load()->taskCancelled(this);
 }
 
 
@@ -104,7 +104,7 @@ void Task::setProgress(float progress)
 	{
 		FastMutex::ScopedLock lock(_mutex);
 		if (_pOwner)
-			_pOwner->taskProgress(this, _progress);
+			_pOwner.load()->taskProgress(this, _progress);
 	}
 }
 
@@ -129,7 +129,7 @@ void Task::postNotification(Notification* pNf)
 	FastMutex::ScopedLock lock(_mutex);
 
 	if (_pOwner)
-		_pOwner->postNotification(pNf);
+		_pOwner.load()->postNotification(pNf);
 	else if (pNf)
 		pNf->release();
 }

Foundation/src/TaskManager.cpp

@@ -48,34 +48,37 @@ TaskManager::TaskManager(ThreadPool& pool):
 
 TaskManager::~TaskManager()
 {
+	for (auto& pTask: _taskList)
+		pTask->setOwner(nullptr);
+
 	if (_ownPool) delete &_threadPool;
 }
 
 
 bool TaskManager::start(Task* pTask)
 {
 	TaskPtr pAutoTask(pTask); // take ownership immediately
-	pTask->setOwner(this);
-	Task::TaskState prevState = pTask->setState(Task::TASK_STARTING);
+	if (pTask->getOwner())
+		throw IllegalStateException("Task already owned by another TaskManager");
 
-	ScopedLockT lock(_mutex);
-	try
+	if (pTask->state() == Task::TASK_IDLE)
 	{
-		// do not start cancelled or running tasks
-		if ((prevState <= Task::TASK_STARTING) || (prevState > Task::TASK_RUNNING))
+		pTask->setOwner(this);
+		pTask->setState(Task::TASK_STARTING);
+		try
 		{
 			_threadPool.start(*pTask, pTask->name());
+			ScopedLockT lock(_mutex);
 			_taskList.push_back(pAutoTask);
 			return true;
 		}
-	}
-	catch (...)
-	{
-		pTask->setOwner(nullptr);
-		throw;
+		catch (...)
+		{
+			pTask->setOwner(nullptr);
+			throw;
+		}
 	}
 
-	pTask->setState(Task::TASK_FINISHED);
 	pTask->setOwner(nullptr);
 	return false;
 }
@@ -158,6 +161,7 @@ void TaskManager::taskFinished(Task* pTask)
 	{
 		if (*it == pTask)
 		{
+			pTask->setOwner(nullptr);
 			_taskList.erase(it);
 			break;
 		}

Foundation/testsuite/src/TaskManagerTest.cpp

@@ -469,8 +469,6 @@ void TaskManagerTest::testCustom()
 
 void TaskManagerTest::testCancelNoStart()
 {
-	std::cout << "TODO";
-	/*
 	TaskManager tm;
 	TaskObserver to;
 	tm.addObserver(Observer<TaskObserver, TaskStartedNotification>(to, &TaskObserver::taskStarted));
@@ -480,23 +478,34 @@ void TaskManagerTest::testCancelNoStart()
 	tm.addObserver(Observer<TaskObserver, TaskProgressNotification>(to, &TaskObserver::taskProgress));
 	AutoPtr<TestTask> pTT = new TestTask;
 	pTT->cancel();
+	assertTrue (pTT->isCancelled());
 	assertFalse(tm.start(pTT.duplicate()));
-	while (pTT->state() < Task::TASK_FINISHED) Thread::sleep(50);
 	assertTrue (pTT->progress() == 0);
+	assertTrue (pTT->isCancelled());
+	assertFalse (pTT->hasOwner());
 	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;
-	tm.start(new SimpleTask);
-	tm.start(new SimpleTask);
-	tm.start(new SimpleTask);
+
+	AutoPtr<SimpleTask> pTT1 = new SimpleTask;
+	AutoPtr<SimpleTask> pTT2 = new SimpleTask;
+	AutoPtr<SimpleTask> pTT3 = new SimpleTask;
+
+	tm.start(pTT1.duplicate());
+	tm.start(pTT2.duplicate());
+	tm.start(pTT3.duplicate());
+
+	assertTrue (pTT1->hasOwner());
+	assertTrue (pTT2->hasOwner());
+	assertTrue (pTT3->hasOwner());
 
 	TaskManager::TaskList list = tm.taskList();
 	assertTrue (list.size() == 3);
@@ -505,6 +514,13 @@ void TaskManagerTest::testMultiTasks()
 	while (tm.count() > 0) Thread::sleep(100);
 	assertTrue (tm.count() == 0);
 	tm.joinAll();
+
+	while (pTT1->state() != Task::TASK_FINISHED) Thread::sleep(50);
+	assertFalse (pTT1->hasOwner());
+	while (pTT2->state() != Task::TASK_FINISHED) Thread::sleep(50);
+	assertFalse (pTT2->hasOwner());
+	while (pTT3->state() != Task::TASK_FINISHED) Thread::sleep(50);
+	assertFalse (pTT3->hasOwner());
 }
 
 

Foundation/testsuite/src/TaskTest.cpp

@@ -112,6 +112,20 @@ void TaskTest::testFinish()
 	pTT->cont();
 	thr.join();
 	assertTrue (pTT->state() == Task::TASK_FINISHED);
+
+	pTT->reset();
+	assertTrue (pTT->progress() == 0);
+	assertTrue (pTT->state() == Task::TASK_IDLE);
+	thr.start(*pTT);
+	assertTrue (pTT->progress() == 0);
+	pTT->cont();
+	while (pTT->progress() != 0.5) Thread::sleep(50);
+	assertTrue (pTT->state() == Task::TASK_RUNNING);
+	pTT->cont();
+	while (pTT->progress() != 1.0) Thread::sleep(50);
+	pTT->cont();
+	thr.join();
+	assertTrue (pTT->state() == Task::TASK_FINISHED);
 }