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

## 2 Answers

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.

• 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. May 10 '17 at 11:52
• 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.

• 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. May 10 '17 at 11:52