Timer utilizing std::future

Question

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

#include <chrono>
#include <functional>
#include <future>
#include <iostream>
#include <list>
#include <mutex>
#include <thread>

using namespace std;

// Timer
// =====

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

    private:
    typedef std::function<Result ()> function_type;
    typedef std::future<result_type> future_type;
    typedef std::mutex mutex;

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

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

    ~Timer() {  stop(); }

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

    template <typename Interval>
    void start(const Interval& interval, Interval delay = Interval::zero()) {
        m_interval = std::chrono::duration_cast<interval_type>(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<mutex> 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);
                        }
                    }
                    else {
                        this->m_future.wait();
                        try {
                            std::lock_guard<mutex> guard(this->m_result_mutex);
                            this->m_results.push_back(this->m_future.get());
                        }
                        catch(const std::exception &) {}
                        break;
                    }
                }
            }
        });
    }

    void stop() {
        m_interval = interval_type::zero();
        if(m_thread.joinable())
            m_thread.join();
    }

    // Results
    // =======

    bool empty() const { return m_results.empty(); }

    results_type results() const {
        results_type results;
        {
            std::lock_guard<mutex> guard(m_result_mutex);
            results.swap(m_results);
        }
        return results;
    }

    result_type result() {
        std::lock_guard<mutex> guard(m_result_mutex);
        result_type result = m_results.front();
        m_results.pop_front();
        return result;
    }

    private:
    interval_type m_interval;
    function_type m_function;
    std::thread m_thread;
    future_type m_future;

    mutable mutex m_result_mutex;
    mutable results_type m_results;
};

template <>
class Timer<void>
{
    // Implementation details for the simpler case omitted.
};


// Test
// ====

char f() {
    return '.';
}

int main()
{
    using std::chrono::milliseconds;

    Timer<char> timer(f);
    timer.start(milliseconds(10));
    for(unsigned i = 0; i <= 10; ++i) {
        std::this_thread::sleep_for(milliseconds(100));
        if(i == 10) timer.stop();
        auto results = timer.results();
        for(const auto& r : results)
            std::cout << r;
        std::cout << '\n';
    }
}

Answer 1

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 <typename Duration>
start(const Duration& period) { start(period, std::chrono::milliseconds(0); }

template <typename Duration1, typename Duration2>
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<mutex> 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<mutex> 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.