5
\$\begingroup\$

I have written the following ticker class. You can find a description and example usage in its comments. However, it kind of feels like I am using too many thread synchronization tools (std::atomic, std::mutex and std::condition_variable). Is there a way to simplify this?

#include <atomic>
#include <chrono>
#include <condition_variable>
#include <cstdint>
#include <iostream>
#include <thread>

// Executes a function f in a fixed interval (given in microseconds).
// Call ticker::start() to run.
// The ticker stops when ticker::stop() is called
// or the instance runs out of scope.
//
// Example usage:
//
// void say_hi()
// {
//     std::cout << "hi " << std::endl;
// }
// int main()
// {
//     ticker hi_ticker( say_hi, 2 * 1000 * 1000 );
//     hi_ticker.start();
//     std::this_thread::sleep_for( std::chrono::milliseconds( 4500 ) );
// }
class ticker
{
public:
    typedef std::function<void()> function;
    void start()
    {
        if ( running_flag_ )
        {
            return;
        }
        running_flag_ = true;
        thread_ = std::thread( [this]() { thread_function(); } );
    }
    ticker( const function& f, std::int64_t interval_us ) :
        f_( f ),
        interval_us_( interval_us ),
        running_flag_( false ),
        thread_(),
        stop_mutex_(),
        stop_cv_()
    {}
    void stop()
    {
        if ( !running_flag_ )
        {
            return;
        }
        running_flag_ = false;
        {
            std::lock_guard<std::mutex> lock( stop_mutex_ );
            stop_cv_.notify_all();
        }
        if ( thread_.joinable() )
        {
            thread_.join();
            thread_ = std::thread();
        }
    }
    ~ticker()
    {
        stop();
    }
private:
    void thread_function()
    {
        auto last_time = std::chrono::high_resolution_clock::now();
        while ( running_flag_ )
        {
            auto current_time = std::chrono::high_resolution_clock::now();
            const auto wake_up_time =
                last_time + std::chrono::microseconds{ interval_us_ };
            while ( current_time < wake_up_time )
            {
                const auto sleep_time = wake_up_time - current_time;
                std::unique_lock<std::mutex> lock( stop_mutex_ );
                stop_cv_.wait_for( lock, sleep_time );
                current_time = std::chrono::high_resolution_clock::now();
                if ( !running_flag_ )
                {
                    return;
                }
            }
            const auto elapsed = current_time - last_time;
            last_time = current_time;
            const auto elapsed_us =
                std::chrono::duration_cast<std::chrono::microseconds>(
                    elapsed).count();
            f_();
        }
    }
    const function f_;
    const std::int64_t interval_us_;
    std::atomic<bool> running_flag_;
    std::thread thread_;
    std::mutex stop_mutex_;
    std::condition_variable stop_cv_;
};

edit: Here is the improved version, based on the excellent reviews here: https://gist.github.com/Dobiasd/99f3b5b0632d57d1f66c94658760c986

\$\endgroup\$
4
\$\begingroup\$

I have some doubts about you member variable running_flag_ and its use in ticker::start(). If its purpose is to avoid calling ticker::start() from the same thread then it can be just bool, not std::atomic<bool>. If its purpose is to avoid calling ticker::start() from the different threads for the same object then you seem to have a data race when two threads might both see running_flag_ equal to false and go ahead with creating two threads later:

    if ( running_flag_ )
    {
        return;
    }
    running_flag_ = true;
    thread_ = std::thread( [this]() { thread_function(); } );

As far as I know std::atomic::compare_exchange_weak must be used in order to avoid such data race when using atomic variable. However as far as I understand you do not expect high contention in ticker::start() and ticker::stop(). So it seems that a more simple approach could be to have running_flag_ as bool

class ticker {
    // ...
    const function f_;
    const std::int64_t interval_us_;
    bool running_flag_;
    std::thread thread_;
    std::mutex stop_mutex_;
    std::condition_variable stop_cv_;

and rewrite functions in this way:

void start() {
    std::lock_guard<std::mutex> lock( stop_mutex_ );
    if (running_flag_ )
        return;
    thread_ = std::thread( [this]() { thread_function(); } );
    running_flag_ = true;
}

void stop() {
    std::unique_lock<std::mutex> lk(std::mutex);
    if (!running_flag_)
        return;
    running_flag_ = false;
    lk.unlock(); 
    stop_cv_.notify_one(); // why all? notify_one seems to be enough
    thread_.join();
}

Next, why you write explicitly thread_(),stop_mutex_(), stop_cv_()? It is unnecessary.

Then thread_function() seems to be little bit complicated. I have choosen using a predicate in order to stop:

void thread_function() {
    auto last_time = std::chrono::steady_clock::now();
    while ( true ) {
        const auto wake_up_time = last_time + std::chrono::microseconds{ interval_us_ };
        const auto sleep_time = wake_up_time - last_time;
        std::unique_lock<std::mutex> lk(stop_mutex_);
        if (!running_flag_)
            break;
        stop_cv_.wait_for(lk, sleep_time, [this](){return !running_flag_;});
        if (!running_flag_)
            break;
        lk.unlock();
        last_time = wake_up_time;
        f_();
    }
}

And finally. I think you should use std::chrono::steady_clock instead of std::chrono::high_resolution_clock since they cannot be adjusted and according to http://en.cppreference.com/w/cpp/chrono/steady_clock:

This clock is not related to wall clock time (for example, it can be time since last reboot), and is most suitable for measuring intervals.

\$\endgroup\$
  • \$\begingroup\$ Sergei, thank you very much for this great review. I did not think about making the ticker suited to be used from multiple threads yet. Of course there should be a remark in my docs clarifying this. Your suggestion using steady_clock is also very good. I will change my code accordingly. The unnecessary member initilizations are there, because GCC with -Werror=effc++ errors otherwise. Here is the improved version. \$\endgroup\$ – Tobias Hermann May 10 '17 at 11:52
3
\$\begingroup\$
  • Clock drift

    When thread_function escapes from the waiting loop, current_time most likely is larger than wake_up_time. Because of last_time = current_time;, the next call will be scheduled later than expected, and the delays will accumulate. To avoid the drift, do last_time = wake_up_time instead. You should also define a policy of dealing with extreme delays, e.g. execute outdated calls immediately, or drop them completely.

  • Use std::timed_mutex.

    Let the main thread lock it, and only unlock it when it wants the ticker to quit. The ticker will try_lock_for() it, and quit as soon as the mutex is successfully acquired. No condition_variable, no atomic_flag is necessary anymore.

\$\endgroup\$
  • \$\begingroup\$ vnp, thanks a lot for your well thought out review. The time drift is a problem I did not yet think about. I will change my code and let it execute outdated calls immediately. timed_mutex is very nice. This makes my code a lot simpler. Here is the improved version. \$\endgroup\$ – Tobias Hermann May 10 '17 at 11:52

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.