From 5b74e06f5985bc918d380f15f2d7078528f0b129 Mon Sep 17 00:00:00 2001 From: Aleksandar Fabijanic Date: Mon, 4 Dec 2023 19:26:31 -0600 Subject: [PATCH] fix(TaskManager): task ownership #4311 --- Foundation/include/Poco/Task.h | 22 ++++++++------ Foundation/include/Poco/TaskManager.h | 2 +- Foundation/src/Task.cpp | 6 ++-- Foundation/src/TaskManager.cpp | 28 ++++++++++-------- Foundation/testsuite/src/TaskManagerTest.cpp | 30 +++++++++++++++----- Foundation/testsuite/src/TaskTest.cpp | 14 +++++++++ 6 files changed, 71 insertions(+), 31 deletions(-) diff --git a/Foundation/include/Poco/Task.h b/Foundation/include/Poco/Task.h index 30d10ed801..b2c4441a35 100644 --- a/Foundation/include/Poco/Task.h +++ b/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 _progress; - std::atomic _state; - Event _cancelEvent; - mutable FastMutex _mutex; + std::string _name; + std::atomic _pOwner; + std::atomic _progress; + std::atomic _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 diff --git a/Foundation/include/Poco/TaskManager.h b/Foundation/include/Poco/TaskManager.h index ed7aafb630..6ca0c2f9fc 100644 --- a/Foundation/include/Poco/TaskManager.h +++ b/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. diff --git a/Foundation/src/Task.cpp b/Foundation/src/Task.cpp index 10e19961e5..a425335a08 100644 --- a/Foundation/src/Task.cpp +++ b/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(); } diff --git a/Foundation/src/TaskManager.cpp b/Foundation/src/TaskManager.cpp index 7360ce9224..3daee633fe 100644 --- a/Foundation/src/TaskManager.cpp +++ b/Foundation/src/TaskManager.cpp @@ -48,6 +48,9 @@ TaskManager::TaskManager(ThreadPool& pool): TaskManager::~TaskManager() { + for (auto& pTask: _taskList) + pTask->setOwner(nullptr); + if (_ownPool) delete &_threadPool; } @@ -55,27 +58,27 @@ TaskManager::~TaskManager() 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; } diff --git a/Foundation/testsuite/src/TaskManagerTest.cpp b/Foundation/testsuite/src/TaskManagerTest.cpp index 71f174b086..0c94840f40 100644 --- a/Foundation/testsuite/src/TaskManagerTest.cpp +++ b/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(to, &TaskObserver::taskStarted)); @@ -480,23 +478,34 @@ void TaskManagerTest::testCancelNoStart() tm.addObserver(Observer(to, &TaskObserver::taskProgress)); AutoPtr 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(to, &TaskObserver::taskStarted)); tm.removeObserver(Observer(to, &TaskObserver::taskCancelled)); tm.removeObserver(Observer(to, &TaskObserver::taskFailed)); tm.removeObserver(Observer(to, &TaskObserver::taskFinished)); tm.removeObserver(Observer(to, &TaskObserver::taskProgress)); -*/} +} void TaskManagerTest::testMultiTasks() { TaskManager tm; - tm.start(new SimpleTask); - tm.start(new SimpleTask); - tm.start(new SimpleTask); + + AutoPtr pTT1 = new SimpleTask; + AutoPtr pTT2 = new SimpleTask; + AutoPtr 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()); } diff --git a/Foundation/testsuite/src/TaskTest.cpp b/Foundation/testsuite/src/TaskTest.cpp index 6ba7931c0a..8677430d94 100644 --- a/Foundation/testsuite/src/TaskTest.cpp +++ b/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); }