From f54e62c35ce6d54389593a0482fc73ac8951e5c0 Mon Sep 17 00:00:00 2001 From: Aleksandar Fabijanic Date: Tue, 28 Nov 2023 00:41:50 -0600 Subject: [PATCH] fix(Task): Cancelled Task shouldn't start running #4311 (WIP) --- Foundation/include/Poco/Task.h | 11 ++-- Foundation/include/Poco/TaskManager.h | 11 +++- Foundation/src/Task.cpp | 44 ++++++++-------- Foundation/src/TaskManager.cpp | 24 +++++---- Foundation/testsuite/src/TaskManagerTest.cpp | 55 ++++++++++++++++++-- Foundation/testsuite/src/TaskManagerTest.h | 1 + Foundation/testsuite/src/TaskTest.cpp | 26 ++++++++- Foundation/testsuite/src/TaskTest.h | 1 + 8 files changed, 134 insertions(+), 39 deletions(-) diff --git a/Foundation/include/Poco/Task.h b/Foundation/include/Poco/Task.h index 904dc97ef6..30d10ed801 100644 --- a/Foundation/include/Poco/Task.h +++ b/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(); diff --git a/Foundation/include/Poco/TaskManager.h b/Foundation/include/Poco/TaskManager.h index e8f2836f20..ed7aafb630 100644 --- a/Foundation/include/Poco/TaskManager.h +++ b/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. diff --git a/Foundation/src/Task.cpp b/Foundation/src/Task.cpp index a84c0c2732..10e19961e5 100644 --- a/Foundation/src/Task.cpp +++ b/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); } diff --git a/Foundation/src/TaskManager.cpp b/Foundation/src/TaskManager.cpp index cddf027b69..7360ce9224 100644 --- a/Foundation/src/TaskManager.cpp +++ b/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; } diff --git a/Foundation/testsuite/src/TaskManagerTest.cpp b/Foundation/testsuite/src/TaskManagerTest.cpp index 070b0ed407..71f174b086 100644 --- a/Foundation/testsuite/src/TaskManagerTest.cpp +++ b/Foundation/testsuite/src/TaskManagerTest.cpp @@ -22,6 +22,7 @@ #include "Poco/Observer.h" #include "Poco/Exception.h" #include "Poco/AutoPtr.h" +#include 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 _fail; + std::atomic _started; }; class SimpleTask: public Task @@ -277,6 +286,11 @@ void TaskManagerTest::testFinish() assertTrue (!to.error()); tm.cancelAll(); tm.joinAll(); + 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)); } @@ -317,6 +331,11 @@ void TaskManagerTest::testCancel() assertTrue (!to.error()); tm.cancelAll(); tm.joinAll(); + 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)); } @@ -330,7 +349,7 @@ void TaskManagerTest::testError() tm.addObserver(Observer(to, &TaskObserver::taskFinished)); tm.addObserver(Observer(to, &TaskObserver::taskProgress)); AutoPtr 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(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)); } @@ -443,6 +467,30 @@ void TaskManagerTest::testCustom() } +void TaskManagerTest::testCancelNoStart() +{ + std::cout << "TODO"; + /* + TaskManager tm; + TaskObserver to; + tm.addObserver(Observer(to, &TaskObserver::taskStarted)); + tm.addObserver(Observer(to, &TaskObserver::taskCancelled)); + tm.addObserver(Observer(to, &TaskObserver::taskFailed)); + tm.addObserver(Observer(to, &TaskObserver::taskFinished)); + tm.addObserver(Observer(to, &TaskObserver::taskProgress)); + AutoPtr 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(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; @@ -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); diff --git a/Foundation/testsuite/src/TaskManagerTest.h b/Foundation/testsuite/src/TaskManagerTest.h index 54e9315732..f1fa21e17b 100644 --- a/Foundation/testsuite/src/TaskManagerTest.h +++ b/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(); diff --git a/Foundation/testsuite/src/TaskTest.cpp b/Foundation/testsuite/src/TaskTest.cpp index 9e6b67d587..6ba7931c0a 100644 --- a/Foundation/testsuite/src/TaskTest.cpp +++ b/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 _started; }; } @@ -142,6 +150,21 @@ void TaskTest::testCancel2() } +void TaskTest::testCancelNoStart() +{ + AutoPtr 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; } diff --git a/Foundation/testsuite/src/TaskTest.h b/Foundation/testsuite/src/TaskTest.h index 8da65fc86f..e35972937c 100644 --- a/Foundation/testsuite/src/TaskTest.h +++ b/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();