Timer utilizing std::future

I have a Timer class executing a function in a specified interval:

```
#include
#include
#include
#include
#include
#include
#include

using namespace std;

// Timer
// =====

template
class Timer
{
public:
typedef std::chrono::milliseconds interval_type;
typedef Result result_type;
typedef std::list results_type;

private:
typedef std::function function_type;
typedef std::future future_type;
typedef std::mutex mutex;

public:
template
Timer(Callable&& callable, Arguments&&... arguments)
: m_interval(interval_type::zero()),
m_function(std::bind(
std::move(callable),
std::forward(arguments)...))
{}

Timer(const Timer&) = delete;
Timer& operator = (const Timer&) = delete;

~Timer() { stop(); }

bool running() const { return m_interval != interval_type::zero(); }

template
void start(const Interval& interval, Interval delay = Interval::zero()) {
m_interval = std::chrono::duration_cast(interval);
m_thread = std::thread([this, delay]() {
std::this_thread::sleep_for(delay);
if( ! this->running()) this->m_results.push_back(this->m_function());
else {
m_future = std::async(std::launch::async, this->m_function);
while(true) {
std::this_thread::sleep_for(this->m_interval);
if(this->running()) {
if(this->m_future.wait_for(interval_type::zero()) == std::future_status::ready) {
try {
std::lock_guard guard(this->m_result_mutex);
this->m_results.push_back(this->m_future.get());
}
catch(const std::exception &) {}
m_future = std::async(std::launch::async, this->m_function);
}
}
el

Kudos! This is on the whole a well-written class, and you appear to have written code that is correct in the face of asynchronicity, which is quite difficult!

To start with, I think Timer is a misleading name for this class. When I think of a timer, I think of a clock that counts down a particular interval once. This is something like a PeriodicRunner or PeriodicFunction; these names call out the fact that the function is called multiple times.

There is no particular reason that the period and delay need be the same type of duration. Instead of using a default argument, I might instead write this as two functions:

template 
start(const Duration& period) { start(period, std::chrono::milliseconds(0); }

template 
start(const Duration1& period, const Duration2& delay) {...}

You have made a few design decisions with start that are reasonable decisions, but not the only or obvious choice. These decisions should be called out in documentation:

• If the previous call has not completed when the period next comes up, a call will be dropped silently.

• Exceptions thrown by the function will be dropped silently.

• this->m_results.push_back(...) is within your try-catch block. push_back may throw e.g. because of memory exhaustion; this will also be dropped silently. On the other hand, std::async will throw if the implementation is unable to start a new thread, but it is called outside of any try-catch block, so will caused std::terminate to be called. Likewise, std::this_thread::sleep_for may throw if the duration type or clock is not from the standard library.

There's quite a bit going on in start including some repetition; as an aid to the reader, consider pulling out some functions. For example, you repeat the code snippet

try {
    std::lock_guard guard(...);
    m_results.push_back(m_future.get());
} catch (const std::exception&) {}

which you could pull out into a private method.

Timer::m_results should not be mutable, and Timer::results() const should not be const. One rule of thumb for this is that const methods should be idempotent. You can implement Timer::results() like so:

results_type results() {
    std::lock_guard guard(m_result_mutex);
    return std::move(m_results);
}

I feel that this is clearer than the swap-into-temp implementation you currently have.

I strongly suggest removing the using namespace std; line. You don't even appear to use it, and it can cause problems.

You should document the public interface of your class. One good idea put a large comment at the top of the header file explaining use cases. You should also document most public methods of your class. Questions you should try to answer in the comments include:

• Are there restrictions on the parameters?

• Is the period the time between the start of two function invocations, or between the end of one invocation and the start of the next?

• Is the period a floor, a ceiling, or neither on the time between invocations?

• What happens if you call start twice on the same Timer?

• What happens if you call start and stop in different threads?

• Does stop block? Might the function be called again (or still be running) when stop returns?

• Must stop return before calling result or results?

• What does result return? Is it sensible to call result multiple times? What happens if you call result and no results remain?

Consider marking the class final; I can't imagine a reasonable subclass.

StackExchange Code Review Q#46807, answer score: 7