diff options
author | Stephan Bergmann <stephan.bergmann@allotropia.de> | 2023-12-14 16:40:44 +0100 |
---|---|---|
committer | Stephan Bergmann <stephan.bergmann@allotropia.de> | 2023-12-15 07:30:35 +0100 |
commit | 7397fa7cdfc33f5a079df42e4d6cfa59ae9e062d (patch) | |
tree | 914126191fe9a958ea66523064e3028fa0ebac25 /salhelper | |
parent | 250213166762bceedc052d80cf6ed5d64e286000 (diff) |
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.bergmann@allotropia.de>
Diffstat (limited to 'salhelper')
-rw-r--r-- | salhelper/CppunitTest_salhelper_testapi.mk | 1 | ||||
-rw-r--r-- | salhelper/qa/timer.cxx | 67 | ||||
-rw-r--r-- | salhelper/source/timer.cxx | 87 |
3 files changed, 120 insertions, 35 deletions
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(); } |