From b8d1792fa0924348eb898e1256d300b656a89fe1 Mon Sep 17 00:00:00 2001 From: Vojin Ilic Date: Tue, 4 Apr 2023 10:54:30 +0200 Subject: [PATCH] Fix hang in destructor Consider following situation. A class owns a timer. In destructor of that class we call .cancel() asynchronous on timer before it's destruction. Now timer is executing cancel in it's own internal thread, while it's doing that destructor of timer is called from owner's destructor. Timer destructor enqueues stop notification. If that enqueue is happening just after while loop from cancel notification, stop notification is gonna be dropped and timer will never stop. Fix: Add new method in TimedNotificationQueue which will return a notification regardless of the time it needs to be executed. Get number of pending tasks in the queue. Flush out that many notifications from queue while taking special consideration of pending Stop and Cancel notifications. Add test for new method in TimedNotificationQueue and fix cancel all tests to actually check if notification got executed. fixes #3986 --- .../include/Poco/TimedNotificationQueue.h | 10 +++++++++ Foundation/src/TimedNotificationQueue.cpp | 14 ++++++++++++ .../src/TimedNotificationQueueTest.cpp | 20 +++++++++++++++++ .../src/TimedNotificationQueueTest.h | 1 + Util/src/Timer.cpp | 12 ++++++---- Util/testsuite/src/TimerTest.cpp | 22 ++++++++++++++----- 6 files changed, 69 insertions(+), 10 deletions(-) diff --git a/Foundation/include/Poco/TimedNotificationQueue.h b/Foundation/include/Poco/TimedNotificationQueue.h index d08e54bcf..8a48b8561 100644 --- a/Foundation/include/Poco/TimedNotificationQueue.h +++ b/Foundation/include/Poco/TimedNotificationQueue.h @@ -89,6 +89,16 @@ public: /// assigned to a Notification::Ptr, to avoid potential /// memory management issues. + Notification* dequeueNextNotification(); + /// Dequeues the next notification regardless of timestamp. + /// Returns 0 (null) if no notification is available. + /// The caller gains ownership of the notification and + /// is expected to release it when done with it. + /// + /// It is highly recommended that the result is immediately + /// assigned to a Notification::Ptr, to avoid potential + /// memory management issues. + Notification* waitDequeueNotification(); /// Dequeues the next pending notification. /// If no notification is available, waits for a notification diff --git a/Foundation/src/TimedNotificationQueue.cpp b/Foundation/src/TimedNotificationQueue.cpp index 59f92c7da..8e2fbc09e 100644 --- a/Foundation/src/TimedNotificationQueue.cpp +++ b/Foundation/src/TimedNotificationQueue.cpp @@ -82,6 +82,20 @@ Notification* TimedNotificationQueue::dequeueNotification() } +Notification* TimedNotificationQueue::dequeueNextNotification() +{ + FastMutex::ScopedLock lock(_mutex); + + NfQueue::iterator it = _nfQueue.begin(); + if (it != _nfQueue.end()) + { + Notification::Ptr pNf = it->second; + _nfQueue.erase(it); + return pNf.duplicate(); + } + return 0; +} + Notification* TimedNotificationQueue::waitDequeueNotification() { for (;;) diff --git a/Foundation/testsuite/src/TimedNotificationQueueTest.cpp b/Foundation/testsuite/src/TimedNotificationQueueTest.cpp index e9b8423fc..cf68d51fa 100644 --- a/Foundation/testsuite/src/TimedNotificationQueueTest.cpp +++ b/Foundation/testsuite/src/TimedNotificationQueueTest.cpp @@ -139,6 +139,25 @@ void TimedNotificationQueueTest::testDequeue() } +void TimedNotificationQueueTest::testDequeueNext() +{ + TimedNotificationQueue queue; + assertTrue (queue.empty()); + assertTrue (queue.size() == 0); + Notification* pNf = queue.dequeueNextNotification(); + assertNullPtr(pNf); + Timestamp time; + time += 100000; + queue.enqueueNotification(new Notification, time); + assertTrue (!queue.empty()); + assertTrue (queue.size() == 1); + pNf = queue.dequeueNextNotification(); + assertNotNullPtr(pNf); + assertTrue (queue.empty()); + assertTrue (queue.size() == 0); + pNf->release(); +} + void TimedNotificationQueueTest::testWaitDequeue() { TimedNotificationQueue queue; @@ -264,6 +283,7 @@ CppUnit::Test* TimedNotificationQueueTest::suite() CppUnit::TestSuite* pSuite = new CppUnit::TestSuite("TimedNotificationQueueTest"); CppUnit_addTest(pSuite, TimedNotificationQueueTest, testDequeue); + CppUnit_addTest(pSuite, TimedNotificationQueueTest, testDequeueNext); CppUnit_addTest(pSuite, TimedNotificationQueueTest, testWaitDequeue); CppUnit_addTest(pSuite, TimedNotificationQueueTest, testWaitDequeueTimeout); diff --git a/Foundation/testsuite/src/TimedNotificationQueueTest.h b/Foundation/testsuite/src/TimedNotificationQueueTest.h index 6f6ffe759..921b4d098 100644 --- a/Foundation/testsuite/src/TimedNotificationQueueTest.h +++ b/Foundation/testsuite/src/TimedNotificationQueueTest.h @@ -28,6 +28,7 @@ public: ~TimedNotificationQueueTest(); void testDequeue(); + void testDequeueNext(); void testWaitDequeue(); void testWaitDequeueTimeout(); diff --git a/Util/src/Timer.cpp b/Util/src/Timer.cpp index c3d0e283a..6fcd36b9e 100644 --- a/Util/src/Timer.cpp +++ b/Util/src/Timer.cpp @@ -84,9 +84,15 @@ public: bool execute() { // Check if there's a StopNotification pending. - Poco::AutoPtr pNf = static_cast(queue().dequeueNotification()); - while (pNf) + int numberOfPendingTasks = queue().size(); + while (numberOfPendingTasks > 0) { + Poco::AutoPtr pNf = static_cast(queue().dequeueNextNotification()); + numberOfPendingTasks--; + if (!pNf) + { + continue; + } if (pNf.cast()) { queue().clear(); @@ -98,10 +104,8 @@ public: { pCnf->_finished.set(); } - pNf = static_cast(queue().dequeueNotification()); } - queue().clear(); _finished.set(); return true; } diff --git a/Util/testsuite/src/TimerTest.cpp b/Util/testsuite/src/TimerTest.cpp index 33b3562ed..b5551bdb3 100644 --- a/Util/testsuite/src/TimerTest.cpp +++ b/Util/testsuite/src/TimerTest.cpp @@ -231,11 +231,16 @@ void TimerTest::testCancelAllStop() TimerTask::Ptr pTask = new TimerTaskAdapter(*this, &TimerTest::onTimer); - timer.scheduleAtFixedRate(pTask, 5000, 5000); - - Poco::Thread::sleep(100); + // We are scheduling a timer event in 100ms + Timestamp time; + time += 100000; + timer.schedule(pTask, time); timer.cancel(false); + // Timer should fire in 100ms and onTimer has 100ms sleep in it. + // So we are waiting 2 times that plus a small buffer that to make sure that event was never executed. + bool timerExecuted = _event.tryWait(200 + 50); + assertFalse (timerExecuted); } assertTrue (true); // don't hang @@ -249,11 +254,16 @@ void TimerTest::testCancelAllWaitStop() TimerTask::Ptr pTask = new TimerTaskAdapter(*this, &TimerTest::onTimer); - timer.scheduleAtFixedRate(pTask, 5000, 5000); - - Poco::Thread::sleep(100); + // We are scheduling a timer event in 100ms + Timestamp time; + time += 100000; + timer.schedule(pTask, time); timer.cancel(true); + // Timer should fire in 100ms and onTimer has 100ms sleep in it. + // So we are waiting 2 times that plus a small buffer that to make sure that event was never executed. + bool timerExecuted = _event.tryWait(200 + 50); + assertFalse (timerExecuted); } assertTrue (true); // don't hang