small ticker class to execute a function at a regular inverval

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
#include
#include
#include
#include
#include

// 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 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 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 lock( stop_mutex_ );
stop_cv_.wait_for( lock, sleep_time );
current_time = std::chrono::high_resolution_clock::now();
if ( !running_flag_ )
{

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. 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 lock( stop_mutex_ );
    if (running_flag_ )
        return;
    thread_ = std::thread( [this]() { thread_function(); } );
    running_flag_ = true;
}

void stop() {
    std::unique_lock 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 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.

StackExchange Code Review Q#162906, answer score: 4