From 222b84f803cfd7fee03163d130d6487af8016835 Mon Sep 17 00:00:00 2001 From: huangyuchen Date: Thu, 20 Oct 2022 10:53:03 +0800 Subject: [PATCH] Fix multithreading-related bugs. 1. Fix a multithreading-related bug, which makes `Timer::Shutdown()` incorrectly skip detach/join process of `thread_`. 2. Fix a multithreading-related bug, which makes `Timer::Shutdown()` fail to stop the loop in `EventReactor::RunLoop()` as expected. Change-Id: I5ccafbfc123a72820155e656a1658683df2623a8 Signed-off-by: huangyuchen --- base/include/timer.h | 16 +++++++++++++++- base/src/event_reactor.cpp | 22 +++++++++++++++------- base/src/event_reactor.h | 19 +++++++++++++++---- base/src/timer.cpp | 12 ++++++++---- 4 files changed, 53 insertions(+), 16 deletions(-) diff --git a/base/include/timer.h b/base/include/timer.h index 8b09ee4..596c414 100644 --- a/base/include/timer.h +++ b/base/include/timer.h @@ -29,7 +29,21 @@ namespace OHOS { namespace Utils { - +/* + * Notice: + * 1. Timer should be set up(via Setup()) before use, and shutdown(via Shutdown()) before its deconstruction. + * + * 2. Timer should be set up first and then shutdown. + * Avoid delegating them to different threads since it may cause multithreading problem. + * + * 3. Set up Timer again would not reset this Timer, but return `TIMER_ERR_INVALID_VALUE`. + * If a reset operation is required, shut Timer down first and then set it up. + * + * 4. Parameter in Shutdown() determines whether the thread in Timer would be detach or join. + * (True(default) --> join; False --> detach). + * + Detach operation would cause possible multithreading problems, thus is not recommended. + * If a detach operation is required, availability of related objects used in `thread_` should be guaranteed. + */ class Timer { public: using TimerCallback = std::function; diff --git a/base/src/event_reactor.cpp b/base/src/event_reactor.cpp index e54d183..fb98bfb 100644 --- a/base/src/event_reactor.cpp +++ b/base/src/event_reactor.cpp @@ -27,7 +27,7 @@ namespace OHOS { namespace Utils { EventReactor::EventReactor() - :stopped_(true), demultiplexer_(new EventDemultiplexer()) + :loopReady_(false), switch_(false), demultiplexer_(new EventDemultiplexer()) { } @@ -51,19 +51,19 @@ void EventReactor::UpdateEventHandler(EventHandler* handler) } } -uint32_t EventReactor::StartUp() +uint32_t EventReactor::SetUp() { if (demultiplexer_ == nullptr) { return TIMER_ERR_INVALID_VALUE; } - uint32_t ret = demultiplexer_->StartUp(); + uint32_t ret = demultiplexer_->StartUp(); // return TIME_ERR_OK, if demultiplexer has been started. if (ret != 0) { UTILS_LOGE("demultiplexer startUp failed."); return ret; } - stopped_ = false; + loopReady_ = true; return TIMER_ERR_OK; } @@ -81,14 +81,22 @@ void EventReactor::RunLoop(int timeout) const UTILS_LOGE("demultiplexer_ is nullptr."); return; } - while (!stopped_) { + + while (loopReady_ && switch_) { demultiplexer_->Polling(timeout); } + + loopReady_ = false; +} + +void EventReactor::SwitchOn() +{ + switch_ = true; } -void EventReactor::StopLoop() +void EventReactor::SwitchOff() { - stopped_ = true; + switch_ = false; } uint32_t EventReactor::ScheduleTimer(const TimerCallback& cb, uint32_t interval, int& timerFd, bool once) diff --git a/base/src/event_reactor.h b/base/src/event_reactor.h index e4889aa..794e44a 100644 --- a/base/src/event_reactor.h +++ b/base/src/event_reactor.h @@ -45,12 +45,22 @@ public: EventReactor& operator=(const EventReactor&&) = delete; virtual ~EventReactor(); - uint32_t StartUp(); + uint32_t SetUp(); void CleanUp(); void RunLoop(int timeout) const; - void StopLoop(); - bool IsStopped() const { return stopped_; } + void SwitchOn(); + void SwitchOff(); + + bool IsLoopReady() const + { + return loopReady_; + } + + bool IsSwitchedOn() const + { + return switch_; + } void UpdateEventHandler(EventHandler* handler); void RemoveEventHandler(EventHandler* handler); @@ -59,7 +69,8 @@ public: void CancelTimer(int timerFd); private: - volatile bool stopped_; + mutable volatile bool loopReady_; // refers to whether reactor is ready to call RunLoop(). + volatile bool switch_; // a switch to enable while-loop in RunLoop(). true: start, false: stop. std::unique_ptr demultiplexer_; std::recursive_mutex mutex_; std::list> timerEventHandlers_; diff --git a/base/src/timer.cpp b/base/src/timer.cpp index cbc1e9d..4fc9a1d 100644 --- a/base/src/timer.cpp +++ b/base/src/timer.cpp @@ -31,6 +31,10 @@ Timer::Timer(const std::string& name, int timeoutMs) : name_(name), timeoutMs_(t uint32_t Timer::Setup() { + if (thread_.joinable()) { // avoid double assign to an active thread + return TIMER_ERR_INVALID_VALUE; + } + reactor_->SwitchOn(); std::thread loop_thread(std::bind(&Timer::MainLoop, this)); thread_.swap(loop_thread); @@ -39,12 +43,12 @@ uint32_t Timer::Setup() void Timer::Shutdown(bool useJoin) { - if (reactor_->IsStopped()) { + if (!thread_.joinable()) { UTILS_LOGD("timer has been stopped already"); return; } - reactor_->StopLoop(); + reactor_->SwitchOff(); if (timeoutMs_ == -1) { std::lock_guard lock(mutex_); if (intervalToTimers_.empty()) { @@ -128,7 +132,7 @@ void Timer::Unregister(uint32_t timerId) void Timer::MainLoop() { prctl(PR_SET_NAME, name_.c_str(), 0, 0, 0); - if (reactor_->StartUp() == TIMER_ERR_OK) { + if (reactor_->SetUp() == TIMER_ERR_OK) { reactor_->RunLoop(timeoutMs_); } reactor_->CleanUp(); @@ -172,7 +176,7 @@ void Timer::OnTimer(int timerFd) continue; } /* if stop, callback is forbidden */ - if (!reactor_->IsStopped()) { + if (reactor_->IsLoopReady() && reactor_->IsSwitchedOn()) { ptr->callback(); } -- Gitee