Creating a ticker thread

Question

A new question about a revised version of the code in this question, based on the advice in the accepted answer can be found here.

I need to signal my program to do something every X seconds. So I decided to make a separate thread that sends such a signal using ZeroMQ. All other behaviour is also triggered by ZeroMQ messages, so this seems like a safe way to do it, with respect to multithreading.

I have never used threads in C++ before though, so I'd like some feedback on my code. Note that in the code below I have replaced the actual task with a simple cout statement.

EDIT: I just realized the thread could very well terminate between setting run = false and calling join, which causes a std::system_error to be thrown. Putting the join statement in an if-block with isjoinable still suffers from the same problem. How should I handle this? try-catch? Using a locking mechanism instead of an atomic bool?

Ticker.hpp

#include <thread>
#include <chrono>
#include <atomic>
class Ticker {
private:
 const unsigned int interval;
 std::atomic_bool run;
 std::thread tickthread;
public:
 Ticker(unsigned int interval);
 Ticker(const Ticker& orig) = delete;
 virtual ~Ticker();
 void stop();
private:
 void tickfunction();
};

Ticker.cpp

#include "Ticker.hpp"
#include <iostream>
using namespace std;
Ticker::Ticker(unsigned int interval)
: interval(interval), run(true), tickthread(&Ticker::tickfunction, this) {
}
Ticker::~Ticker() {
 if (tickthread.joinable()) {
 stop();
 }
}
void Ticker::stop() {
 cout << "stopping..." << endl;
 run = false;
 // interrupt sleep?
 tickthread.join();
 cout << "joined" << endl;
}
void Ticker::tickfunction() {
 while (true) {
 this_thread::sleep_for(chrono::seconds(interval));
 if (run) {
 cout << "tick" << endl;
 } else {
 break;
 }
 }
}

There are also two specific things I'd like recommendations on:

1. How can I change this class to accept any std::chrono::duration object as input for the interval, instead of just an integer? I tried this, but couldn't figure out how to do it with the templates.

2. Is there a way to interrupt the sleep_for when I call stop()? Especially when interval is large, it'd be nice to not have to wait for the sleeping thread when stopping it.

• Vogel612: Welcome to Code Review! Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers .
• Oebele: @Vogel612: apologies. I knew I shouldn't change the existing code, but was unsure whether to append or add a new question.
• Vogel612: This is not a forum. Instead of 'appending' a new question (threads don't exist here) it's more conventional to ask a new question (equivalent to starting a new thread) ; )

Answer 1

I see some things that may help you improve your program.

Use include guards

You probably already know this, but the Ticker.hpp file should have include guards to allow it to be included in multiple files within the same project. The ordinary form would be something like this:

#ifndef TICKER_HPP
#define TICKER_HPP
// contents of Ticker.hpp go here
#endif //TICKER_HPP

Avoid data races

The tickfunction accesses std::cout but that's a single resource that might simultaneously be used by other threads. One fix for this is to use a std::mutex like this:

std::mutex cout_mutex;
// wherever cout is used:
some_function() {
 std::lock_guard<std::mutex> lock(cout_mutex);
 std::cout << "Now we can do this safely!\n";
}

Note that std::lock_guard is intended to be used via RAII so that the lock is obtained when the object is created and released when it's destroyed, making it easy to avoid the bug of forgetting to unlock the mutex.

If the actual function called within your code each "tick" uses shared resources, you should apply the same technique.

Use standard durations

To answer your question, one way to use a standard duration would be to specify a particular one. For example, if you decided that millisecond resolution was enough for this program (and it probably is), the class declaration might look like this:

class Ticker {
private:
 const std::chrono::milliseconds interval;
 std::atomic_bool run;
 std::thread tickthread;
public:
 Ticker(std::chrono::milliseconds intval);
 Ticker(const Ticker& orig) = delete;
 virtual ~Ticker();
 void stop();
private:
 void tickfunction();
};

Use the chrono literals for more readable code

With the code modified to use a standards time interval, we can now use it with std::chrono_literals and make the code even easier to write and to read.

int main()
{
 using namespace std::chrono_literals;
 Ticker t(500ms);
 Ticker t10(10s);
 std::this_thread::sleep_for(15s);
}

Allow for thread cancellation

For various reasons, there is no simple standard way of cancelling a thread. However, there are almost always ways you can accomplish the equivalent in your own code. I'll show how this might be done in the next suggestion.

Prefer async to raw threads

C++ has a std::async mechanism that is a bit higher level than the more fundamental thread and mutex mechanism. The advantage to using it is that you can spend more time thinking about the problem you're trying to solve rather than the details of threads and locks. Here's one way of refactoring your code that doesn't use thread explicitly:

#include <iostream>
#include <future>
#include <chrono>
static std::mutex cout_mutex;
class Ticker 
{
public:
 // create but don't execute yet
 Ticker(std::shared_future<void> f, std::chrono::milliseconds interval) 
 : interval{interval}, done{f}
 { }
 // run the thread
 std::future<void> run() {
 return std::async(std::launch::async, &Ticker::tickWrapper, this);
 }
private:
 void tickWrapper() {
 std::future_status status;
 do {
 status = done.wait_for(interval); // waits for the signal from main()
 if (status == std::future_status::timeout) {
 tickfunction();
 }
 } while (status != std::future_status::ready);
 }
 void tickfunction() {
 std::lock_guard<std::mutex> lock(cout_mutex);
 std::cout << "tick (" << std::this_thread::get_id() << ")" << std::endl;
 }
 const std::chrono::milliseconds interval;
 std::shared_future<void> done;
};

Note that I've separated tickWrapper from tickfunction. The latter is now just a simple function that doesn't contain any references to timers or futures. This allows us to separate the actual function from the mechanics of a recurring timer implementation. Here's a main function showing how to create and drive this new Ticker object:

int main()
{ 
 // for convenience in specifying time durations
 using namespace std::chrono_literals;
 // create a promise and associated shared_future
 std::promise<void> done;
 std::shared_future<void> done_future(done.get_future());
 // create the Ticker objects 
 Ticker t1(done_future, 300ms);
 Ticker t2(done_future, 500ms);
 // start the threads
 auto x = t1.run();
 auto y = t2.run();
 // delay for a while 
 std::this_thread::sleep_for(3s);
 // tell the other threads to stop
 done.set_value();
 std::cout << "Done\n";
}

• Oebele: Thanks for you response. I knew async existed, but I hadn't realized futures could be useful in this case. I do feel that the main function is getting a little bloated though: 4 lines to start one ticker instead of one, and with less self-documentation (set_value is not very clear), but I think I can hide that in the Ticker. I suppose you did not do that to demonstrate the reuse of the promise?
• Oebele: Regarding your other points: Seems like I didn't copy the include guards - they are in the file. The normal implementation intentionally does not use shared resources, so that's not going to be a problem. Too bad there is not an easy way to allow any duration to be provided. I believe chrono literals are not available on gcc 4.8.5, which I am restricted to - I should add the tag c++11.
• Oebele: Alright, I adapted your example implementation and added it to the question. I'd appreciate it if you'd want to give it a look.
• Oebele: Oh, whoops, that was not allowed, so I put it in a new question: codereview.stackexchange.com/q/126017/102775