salhelper/CppunitTest_salhelper_testapi.mk | 1
salhelper/qa/timer.cxx | 67 ++++++++++++++++++++++
salhelper/source/timer.cxx | 87 +++++++++++++++++------------
3 files changed, 120 insertions(+), 35 deletions(-)
New commits:
commit 7397fa7cdfc33f5a079df42e4d6cfa59ae9e062d
Author: Stephan Bergmann <stephan.bergm...@allotropia.de>
AuthorDate: Thu Dec 14 16:40:44 2023 +0100
Commit: Stephan Bergmann <stephan.bergm...@allotropia.de>
CommitDate: Fri Dec 15 07:30:35 2023 +0100
Fix salhelper::Timer
Using it was prone to cause deadlocks on shutdown, when the main thread
during
exit destroys the static salhelper::TimerManager instance, which blocks
destroying its m_notEmpty member because the salhelper::TimerManager::run
thread
is blocking at
> m_notEmpty.wait(pDelay);
Change-Id: If72700cb622e945f5f314a00ded57538961ab8d7
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/160788
Tested-by: Jenkins
Reviewed-by: Stephan Bergmann <stephan.bergm...@allotropia.de>
diff --git a/salhelper/CppunitTest_salhelper_testapi.mk
b/salhelper/CppunitTest_salhelper_testapi.mk
index f3d98aa435bd..abcb5bc02616 100644
--- a/salhelper/CppunitTest_salhelper_testapi.mk
+++ b/salhelper/CppunitTest_salhelper_testapi.mk
@@ -17,6 +17,7 @@ $(eval $(call gb_CppunitTest_CppunitTest,salhelper_testapi))
$(eval $(call gb_CppunitTest_add_exception_objects,salhelper_testapi,\
salhelper/qa/test_api \
+ salhelper/qa/timer \
))
$(eval $(call gb_CppunitTest_use_external,salhelper_testapi,boost_headers))
diff --git a/salhelper/qa/timer.cxx b/salhelper/qa/timer.cxx
new file mode 100644
index 000000000000..53e329b8ce71
--- /dev/null
+++ b/salhelper/qa/timer.cxx
@@ -0,0 +1,67 @@
+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4;
fill-column: 100 -*- */
+/*
+ * This file is part of the LibreOffice project.
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ */
+
+#include <sal/config.h>
+
+#include <condition_variable>
+#include <mutex>
+
+#include <cppunit/TestAssert.h>
+#include <cppunit/TestFixture.h>
+#include <cppunit/extensions/HelperMacros.h>
+#include <rtl/ref.hxx>
+#include <salhelper/timer.hxx>
+
+namespace
+{
+class TestTimer : public salhelper::Timer
+{
+public:
+ TestTimer()
+ : Timer(salhelper::TTimeValue(0, 1))
+ {
+ }
+
+ void SAL_CALL onShot() override
+ {
+ {
+ std::scoped_lock l(mutex);
+ done = true;
+ }
+ cond.notify_all();
+ }
+
+ std::mutex mutex;
+ std::condition_variable cond;
+ bool done = false;
+};
+
+class TimerTest : public CppUnit::TestFixture
+{
+public:
+ void test()
+ {
+ rtl::Reference<TestTimer> t(new TestTimer);
+ t->start();
+ {
+ std::unique_lock l(t->mutex);
+ t->cond.wait(l, [t] { return t->done; });
+ }
+ CPPUNIT_ASSERT(t->isExpired());
+ }
+
+ CPPUNIT_TEST_SUITE(TimerTest);
+ CPPUNIT_TEST(test);
+ CPPUNIT_TEST_SUITE_END();
+};
+
+CPPUNIT_TEST_SUITE_REGISTRATION(TimerTest);
+}
+
+/* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s
cinkeys+=0=break: */
diff --git a/salhelper/source/timer.cxx b/salhelper/source/timer.cxx
index 10f2e7138a61..2af5b6bfdd90 100644
--- a/salhelper/source/timer.cxx
+++ b/salhelper/source/timer.cxx
@@ -19,17 +19,19 @@
#include <salhelper/timer.hxx>
#include <osl/thread.hxx>
-#include <osl/conditn.hxx>
+#include <condition_variable>
#include <mutex>
using namespace salhelper;
-class salhelper::TimerManager : public osl::Thread
+class salhelper::TimerManager final : public osl::Thread
{
public:
TimerManager();
+ ~TimerManager();
+
/// register timer
void registerTimer(salhelper::Timer* pTimer);
@@ -48,10 +50,11 @@ protected:
/// sorted-queue data
salhelper::Timer* m_pHead;
+ bool m_terminate;
/// List Protection
std::mutex m_Lock;
/// Signal the insertion of a timer
- osl::Condition m_notEmpty;
+ std::condition_variable m_notEmpty;
/// "Singleton Pattern"
//static salhelper::TimerManager* m_pManager;
@@ -203,46 +206,60 @@ TTimeValue Timer::getRemainingTime() const
**/
TimerManager::TimerManager() :
- m_pHead(nullptr)
+ m_pHead(nullptr), m_terminate(false)
{
- m_notEmpty.reset();
-
// start thread
create();
}
+TimerManager::~TimerManager() {
+ {
+ std::scoped_lock g(m_Lock);
+ m_terminate = true;
+ }
+ m_notEmpty.notify_all();
+ join();
+}
+
void TimerManager::registerTimer(Timer* pTimer)
{
if (!pTimer)
return;
- std::lock_guard Guard(m_Lock);
+ bool notify = false;
+ {
+ std::lock_guard Guard(m_Lock);
- // try to find one with equal or lower remaining time.
- Timer** ppIter = &m_pHead;
+ // try to find one with equal or lower remaining time.
+ Timer** ppIter = &m_pHead;
- while (*ppIter)
- {
- if (pTimer->expiresBefore(*ppIter))
+ while (*ppIter)
{
- // next element has higher remaining time,
- // => insert new timer before
- break;
+ if (pTimer->expiresBefore(*ppIter))
+ {
+ // next element has higher remaining time,
+ // => insert new timer before
+ break;
+ }
+ ppIter= &((*ppIter)->m_pNext);
}
- ppIter= &((*ppIter)->m_pNext);
- }
- // next element has higher remaining time,
- // => insert new timer before
- pTimer->m_pNext= *ppIter;
- *ppIter = pTimer;
+ // next element has higher remaining time,
+ // => insert new timer before
+ pTimer->m_pNext= *ppIter;
+ *ppIter = pTimer;
- if (pTimer == m_pHead)
- {
+ if (pTimer == m_pHead)
+ {
+ notify = true;
+ }
+ }
+
+ if (notify) {
// it was inserted as new head
// signal it to TimerManager Thread
- m_notEmpty.set();
+ m_notEmpty.notify_all();
}
}
@@ -334,28 +351,28 @@ void TimerManager::run()
while (schedule())
{
- TTimeValue delay;
- TTimeValue* pDelay=nullptr;
-
{
- std::lock_guard a_Guard(m_Lock);
+ std::unique_lock a_Guard(m_Lock);
if (m_pHead != nullptr)
{
- delay = m_pHead->getRemainingTime();
- pDelay=&delay;
+ TTimeValue delay = m_pHead->getRemainingTime();
+ m_notEmpty.wait_for(
+ a_Guard,
+ std::chrono::nanoseconds(
+ sal_Int64(delay.Seconds) * 1'000'000'000 +
delay.Nanosec),
+ [this] { return m_terminate; });
}
else
{
- pDelay=nullptr;
+ m_notEmpty.wait(a_Guard, [this] { return m_terminate ||
m_pHead != nullptr; });
}
-
- m_notEmpty.reset();
+ if (m_terminate) {
+ break;
+ }
}
- m_notEmpty.wait(pDelay);
-
checkForTimeout();
}
