1
\$\begingroup\$

I have written a timer class.

Here it is in all its "glory" with some functions around it to test it: https://rextester.com/LQSXUA43758

Below is the class on its own, note it also relies on another class called stop_watch (which can be seen in the link), but stop watch is quite simple and just relies on std::chrono::steady_clock::now(); to get time differences.

The items I want to check (but happy for all of it to be reviewed!) are:

  • With cv.wait_for(... do i need the loop around it to catch spurious wakes? I was reading this: https://stackoverflow.com/questions/31871267/how-does-condition-variablewait-for-deal-with-spurious-wakeups, but I could not quite fathom the answer. It suggests that with the predicate I do not need the spurious-catch loop around it?
  • I am re-calculating the amount of time to wait each time based on the total time so that if the timer is running continuously (i.e. not one shot) it does not lose time - just want to check this approach.
  • The timeout handler function is called within the timer thread. This is ok if the handler just sets a flag or puts an event on a queue - but what if someone wants to do a load of work here? Like what if I set timeout to 100ms and the work takes 110ms? - should I try to run that in another thread detatched?
  • Finally, the wait time calculation is working well, but I am not sure if I need to worry about if the time calculated is negative. In the previous point I mentioned if the work took longer then the timeout, that means on the next iteration the time to wait calculation might -10ms! - what then?

Timer class:

/// @brief timer class to call a callback function after a specified amount of time has expired
class timer
{
private:
    /// the timer thread
    std::thread timer_thread;
    /// atomic bool used to stop the timer
    std::atomic<bool> timer_running;
    /// condition var mutex
    std::mutex mtx;
    /// condition var used for waiting
    std::condition_variable cv;
public:
    /// @brief Construct a new timer object
    timer() = default;
    /// @brief Destroy the timer object - ensures the timer has stopped
    ~timer() { stop(); }

    /// @brief Starts the timer
    /// @param timeout_ms the amount of time until the timer expires in milliseconds
    /// @param timeout_handler the function callback which is called if/when the timer expires
    template <typename Functor>
    void start(unsigned int timeout_ms, const Functor &timeout_handler, bool oneshot = true)
    {
        /// Start the
        timer_running = true;
        timer_thread = std::thread([timeout_handler, timeout_ms, oneshot, this]() {
            stopwatch sw;
            uint64_t interval_ms = static_cast<uint64_t>(timeout_ms);
            // Keep a running total of the time required time to wait
            uint64_t total_time_ms = 0;
            // Keep running the timer until it is no longer running
            while (timer_running)
            {
                // increment the total time required to wait by the interval
                total_time_ms += interval_ms;

                // Keep waiting until we have reached the elapsed time (in case of spurious wake)
                // or the timer is stopped. Note the wait_for will handle timer_running = false
                // so we don't need to check that in this loop
                while (sw.get_elapsed_time() < total_time_ms)
                {
                    std::unique_lock<std::mutex> lock{mtx};
                    // Re-calculate the time we need to wait for so that we are not losing time
                    // returns true if timer was stopped, returns false if timer expired
                    if (cv.wait_for(lock,
                                    std::chrono::milliseconds{total_time_ms - sw.get_elapsed_time()},
                                    [this] { return (bool)!timer_running; }))
                    {
                        // timer stopped
                        return;
                    }
                }
                // Timer expired - Call timeout handler
                timeout_handler();

                // if oneshot stop the timer
                if (oneshot)
                {
                    return;
                }
            }
        });
    }

    /// @brief Stops the timer - the callback will not be called.
    void stop()
    {
        // Set the running flag to false so the timer does not continue
        timer_running = false;
        // wake the timer
        cv.notify_all();
        // Join the thread
        if (timer_thread.joinable())
        {
            timer_thread.join();
        }
    }
};

All hints / tips most welcome :)

\$\endgroup\$
1
\$\begingroup\$

I see two main problems in this approach

  1. Creating a thread for each timer is extremely wasteful
  2. Running the callback synchronously with the timer can lead to all sorts of problems as you mention.

The usual design for a high-performance timer is to have a single thread that waits for the next timer to fire and queues the callback to some work-queue. The examples would be boost::asio::deadline_timer or windows' timerQueueTimer

Your time calculation of course would not work, there is no way you can expect a clock time to match exactly with any constant and you are going to get negative values.

\$\endgroup\$
2
  • \$\begingroup\$ ah, that makes sense. The "worker" queue (I assume) would be on another thread - I could implement that in another class and add it as a member into the timer class. Then I assume that when the timer is stopped I would need to also empty the queue of any remaining items (at least these are my first thoughts) thanks \$\endgroup\$ May 22 '20 at 9:47
  • \$\begingroup\$ Also with regard to the time calculation. Just added some tests to try a worker that takes 1000ms with a timer of 100, of course the timer can't keep up, but it handles the negative value (by luck, not design) due to the inner while loop: while (sw.get_elapsed_time() < total_time_ms) - that was meant to stop spurious wakes, but seems to go a good job catching potentially negative time calcs :) \$\endgroup\$ May 22 '20 at 9:49
1
\$\begingroup\$

With cv.wait_for(... do i need the loop around it to catch spurious wakes?

You should've read the answer better or checked the C++ reference for std::condition_variable. When you supply a predicate (the condition lambda) to wait, wait_for, wait_until the function already has a loop inside it to deal with spurious wake ups. It will exit only once the condition is met or the timeout was reached.

Finally, the wait time calculation is working well, but I am not sure if I need to worry about if the time calculated is negative. In the previous point I mentioned if the work took longer then the timeout, that means on the next iteration the time to wait calculation might -10ms! - what then?

You decide what to do when the time passed, either trigger the handler immediately or skip for the next round. The timer class is meaningless if executing the handler takes more time than the trigger time.

Note 1: To avoid dealing with negative times - use wait_until and supply a time point instead of the unnecessary work around calculation. Just store a steady_clock::time_point and increment by the duration.

Another solution is to have access to an executor class (a thread pool) and request the executor class to execute the task given by the handler. So timer's thread doesn't do anything besides waiting.

Note 2: It is advisable to not mix synchronizations of std::atomic with std::mutex as from time to time there are timing issues easy to miss. You should have timer_running be just a bool and use the mutex mtx for synchronizing it. This is a general rule. In this timer class it might cause only mild issues but in some cases it might result in serious and hard-to-catch errors as they are extremely rare and hard to reproduce. In your timer class it might cause the timer to wait the whole timeout even if stop method was called and only then exiting.


Overall design: it is wasteful to have a separate thread for just a timer. Consider making a singleton class (alarm clock?) with a single thread that deals with all the timers you run and wakes them up on when asked. It should only triggers waking up not executing the handlers. It can also be utilized for other classes and services.

Note: Honestly, I don't like singletons and prefer context pattern but it requires a throughout integration of the context into the code.

\$\endgroup\$
6
  • \$\begingroup\$ Thanks ALX23z. Some really interesting points and concepts here for me to start adding into my timer. I have a few questions though - i'll add each as a separate comment... \$\endgroup\$ May 25 '20 at 11:59
  • \$\begingroup\$ 1. I can't find any reference to issues with mixing atomic and mutex. I don't see atomics as a synchronisation mechnism at all, it just guarantees you can write / read to the value in multiple threads without corruption. \$\endgroup\$ May 25 '20 at 11:59
  • \$\begingroup\$ 2. (and more importantly for me) I just don't see how you can have a timer without a thread. I totally get your alarm clock idea, timer can say alarm_clock, wake me in 100ms, that is all good... but now the timer actually has to sleep. If the timer has to sleep, that means its thread has to sleep. If the timer has no thread of its own, then this must be the thread that owns the timer object. Lets call it the main thread. So in main thread if I do m_timer.start(1000) (milliseconds) if the timer has no thread, it will block main for 1000 ms. Can you explain your concept a bit further? thanks! \$\endgroup\$ May 25 '20 at 12:02
  • \$\begingroup\$ @code_fodder (1) I didn't write it accurately. It isn't about mixing mutexes and atomics. It is about condition_variable. condition_variable is meant to utilize a given mutex for data synchronization. But you modify the data it accesses via an unrelated atomic variable that no clue about mutex. This is what leads to the issues. The mutex is needed to ensure that condition variable operates properly and it is needed that data of the condition isn't modified while the mutex is locked. \$\endgroup\$
    – ALX23z
    May 25 '20 at 19:46
  • \$\begingroup\$ @code_fodder (2) Yeah, you still need some thread to wait for the alarm clock - sometimes this is the main thread as this is what you want. However, you can also utilize the idea of executors. Make a thread pool, give the alarm clock access to the executor and make it schedule a task in the thread pool once the time is reached. This way you have a fixed number of threads for all timers and whoever else utilizes the thread pool (1 for alarm clock and #core_count for the executors) for the whole duration of the program. That being said, you'll need some API to check for status of executed tasks. \$\endgroup\$
    – ALX23z
    May 25 '20 at 20:11

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.