I've implemented a timer thread whose job is to call functions at a certain interval. The functions may be called multiple times if their interval > 0 millisec otherwise only one time. Even if the timer thread is running, still a new function can be registered. Could you please provide your feedback and improvement factors?
class TimerThread{ using ClockType = std::chrono::high_resolution_clock;public: TimerThread() = default; TimerThread(TimerThread const &) = delete; TimerThread & operator=(TimerThread const &) = delete; ~TimerThread() noexcept { try { stop(); } catch(std::exception const &ex) { std::cout << "Exception: " << ex.what() << std::endl; } catch(...) { } } void registerCallback(std::function<void()> func, uint32_t const interval=0) { std::unique_lock<std::mutex> lock{mt_}; timers_.emplace_back(std::move(func), ClockType::now(), interval); cv_.notify_all(); } void start(uint32_t const delay=0) { if (! start_) { std::this_thread::sleep_for(std::chrono::milliseconds(delay)); workerThread_ = std::thread{&TimerThread::run, this}; start_ = true; } }private: void run() { for (auto &t: timers_) t.prevFireTime_ = ClockType::now(); while (startTimerFlag_.load(std::memory_order_acquire)) { std::unique_lock<std::mutex> lock{mt_}; cv_.wait(lock, [this]() -> bool { return ! timers_.empty(); }); for (auto &t: timers_) if (t.isReady()) t.func_(); } } void stop() { startTimerFlag_.store(false, std::memory_order_release); cv_.notify_all(); if (workerThread_.joinable()) workerThread_.join(); } struct TimerInfo { TimerInfo() = default; TimerInfo(std::function<void()> func, ClockType::time_point prevFireTime, uint32_t const interval): func_{std::move(func)}, prevFireTime_{prevFireTime}, intervalMilliSec_{interval} { } bool isReady() { if (!isFiredFirstTime) { isFiredFirstTime = true; return true; } else if (intervalMilliSec_ != 0) { auto current = ClockType::now(); uint32_t const duration = std::chrono::duration_cast<std::chrono::milliseconds>(current - prevFireTime_).count(); if (duration >= intervalMilliSec_) { prevFireTime_ = current; return true; } } return false; } std::function<void()> func_; ClockType::time_point prevFireTime_; uint32_t intervalMilliSec_; bool isFiredFirstTime{false}; }; std::vector<TimerInfo> timers_; std::thread workerThread_; std::mutex mt_; std::condition_variable cv_; std::atomic<bool> startTimerFlag_{true}; bool start_{false};};int main(){ TimerThread timer; timer.registerCallback([](){ std::cout << "Timer 1 - " << std::chrono::duration_cast<std::chrono::nanoseconds>(std::chrono::high_resolution_clock::now().time_since_epoch()).count() << std::endl; }, 1000); timer.registerCallback([](){ std::cout << "Timer 2 - " << std::chrono::duration_cast<std::chrono::nanoseconds>(std::chrono::high_resolution_clock::now().time_since_epoch()).count() << std::endl; }, 2000); timer.registerCallback([](){ std::cout << "Timer 3 - " << std::chrono::duration_cast<std::chrono::nanoseconds>(std::chrono::high_resolution_clock::now().time_since_epoch()).count() << std::endl; }); timer.start(); std::this_thread::sleep_for(std::chrono::seconds(5)); LOG("Terminating main()..."); return 0;}
1 Answer1
Don't handle exceptions in the destructor
You should indeed not have a destructor throw exceptions, but what you are doing is unsafe as well. If you call something that throws, only handle the exceptions that you can actually handle. If you just print a warning and continue, bad stuff might happen later because you did nothing to solve the actual issue. It's much better to let the exception propagate then, if that causes the program to terminate that's probably for the best.
Don't mix atomics and mutexes
There is a race condition when callingstop(): consider thatrun() has just checked ifstartTimerFlag is stilltrue, and is locking the mutex but has not enteredcv_.wait() yet. Thenstop() setsstartTimerFlag tofalse and notifies all threads. Thencv.wait() will not see that notification, and it will only check iftimers_ is empty. If it's not empty, it will wait indefinitely.
If you are using mutexes, the best thing to do is not to use any atomics, but just regular variables that are protected by those mutexes, and treat them as such. To fix the issue inrun(), write it like so:
void run(){ ... std::unique_lock<std::mutex> lock{mt_}; while (startTimerFlag) { for (auto &t: timers_) if (t.isReady()) t.func_(); cv_.wait(lock, [this]() -> bool { return !timers_.empty() || !startTimerFlags; }); ... }}Lock outside the loop
You should take the lock inrun() outside thewhile-loop. This avoids unnecessary unlocking and relocking every iteration of the loop. Note thatcv_.wait() will unlock the mutex while it is sleeping, so other threads will not be blocked. Except of course if it doesn't sleep:
Avoid busy-looping
If there are no timers registered,run() will sleep insidecv_.wait(). However, if there is a timer registered, it will never wait inside the loop, and instead it will check over and over again if the registered timer(s) expired. This wastes a lot of CPU cycles. It is better to calculate when the next timer is about to expire, and thenwait_until() that time.
Store and pass durations asClockType::duration
Instead of storing intervals as 32-bit integers representing milliseconds, just useClockType::duration to store and pass durations around. This gives the caller more flexibility. Consider:
timer.registerCallback([]{std::cout << "Slow timer\n";}, std::chrono::seconds(1));timer.registerCallback([]{std::cout << "Fast timer\n";}, std::chrono::microseconds(42));It also makesisReady() a bit simpler. Consider that you can then just write:
auto current = ClockType::now();auto duration = current - prevFireTime;if (duration >= interval_){ prevFireTime_ = current; return true;}Ensuring timer accuracy
There is no guarantee thatisReady() will be called exactly when a timer has expired. If you have a timer that repeats a lot, errors in the expiry time are accumulated. To avoid this, write this inisReady() when a timer expired:
prevFireTime_ += interval_;This keepsprevFireTime_ advancing at exactly the specified interval.
You mustlog in to answer this question.
Explore related questions
See similar questions with these tags.