Timer thread implementation

Question

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;
}

Answer 1

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 calling stop(): consider that run() has just checked if startTimerFlag is still true, and is locking the mutex but has not entered cv_.wait() yet. Then stop() sets startTimerFlag to false and notifies all threads. Then cv.wait() will not see that notification, and it will only check if timers_ 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 in run(), 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 in run() outside the while-loop. This avoids unnecessary unlocking and relocking every iteration of the loop. Note that cv_.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 inside cv_.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 then wait_until() that time.

Store and pass durations as ClockType::duration

Instead of storing intervals as 32-bit integers representing milliseconds, just use ClockType::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 makes isReady() 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 that isReady() 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 in isReady() when a timer expired:

prevFireTime_ += interval_;

This keeps prevFireTime_ advancing at exactly the specified interval.