3
\$\begingroup\$

I have made a simple Timer class used for scheduling tasks. While defining the interface, I have followed the interface of a java.util.Timer class. The class is being written by using only C++11.

C++17 version is provided here

timer.hpp

#include <atomic>
#include <chrono>
#include <future>
#include <utility>

template<typename F, typename... Args>
inline auto invoke(F f, Args&&... args)
    -> decltype(std::ref(f)(std::forward<Args>(args)...)) {
    return std::ref(f)(std::forward<Args>(args)...);
}

class Timer {
public:
    using Timestamp = std::chrono::time_point<
        std::chrono::system_clock,
        std::chrono::microseconds
    >;

    using Delay = std::chrono::milliseconds;
    using Period = std::chrono::milliseconds;

public:
    Timer() = delete;
    Timer(std::atomic<bool>& is_running) : is_running_(is_running) {}
    Timer(Timer const& other) = default;
    Timer(Timer&& other) = default;

    Timer& operator=(Timer const& other) = default;
    Timer& operator=(Timer&& other) = default;

    ~Timer() = default;

    template <typename TimerTask, typename ... Args>
    std::future<void> schedule(TimerTask task, Timestamp const& time, Args... args)  {
        using namespace std::chrono;
        return std::async(std::launch::async, [=]() {
            while (time_point_cast<Timestamp::duration>(system_clock::now()) < time) {
                // wait for the time
            }
            invoke(task, args...);
        });
    }

    template <typename TimerTask, typename Period, typename ... Args>
    std::future<void> schedule(TimerTask task, Timestamp const& time, Period const& period, Args... args)  {
        using namespace std::chrono;
        return std::async(std::launch::async, [=]() {
            while (time_point_cast<Timestamp::duration>(system_clock::now()) < time) {
                // wait for the time
            }
            while (is_running_) {
                invoke(task, args...);
                std::this_thread::sleep_for(period);
            }
        });
    }

    template <typename TimerTask, typename ... Args>
    std::future<void> schedule(TimerTask task, Delay const& delay, Args... args)  {
        using namespace std::chrono;
        return std::async(std::launch::async, [=]() {
            auto start = system_clock::now();
            while (duration_cast<Delay>(system_clock::now() - start) < delay) {
                // wait for the delay
            }
            invoke(task, args...);
        });
    }

    template <typename TimerTask, typename ... Args>
    std::future<void> schedule(TimerTask task, Delay const& delay, Period const& period, Args... args)  {
        using namespace std::chrono;
        return std::async(std::launch::async, [=]() {
            auto start = system_clock::now();
            while (duration_cast<Delay>(system_clock::now() - start) < delay) {
                // wait for the delay
            }
            while (is_running_) {
                invoke(task, args...);
                std::this_thread::sleep_for(period);
            }
        });
    }

    void cancel() {
        is_running_ = false;
    }

private:
    std::atomic<bool>& is_running_;
};

main.cpp

#include <iostream>
#include "timer.hpp"

int main() {
    std::atomic<bool> is_running{true};
    Timer timer(std::ref(is_running));

    Timer::Timestamp ts = std::chrono::time_point_cast<Timer::Timestamp::duration>(
        std::chrono::system_clock::now()
    );
    ts = ts + std::chrono::milliseconds{10000};

    auto future = timer.schedule([] {
        std::cout << "." << std::endl;
    }, ts, Timer::Period{5000});

    std::cout << "... in main ..." << std::endl;
    std::this_thread::sleep_for(std::chrono::seconds{20});
    std::cout << "... cancel ..." << std::endl;
    timer.cancel();

    return 0;
}

Let me hear your opinion about this implementation, code style, performance, possible improvements or possible pitfalls etc.

You may also note all the things that can be simplified or changed by using C++17 or even C++20.

\$\endgroup\$
2
\$\begingroup\$

Your usage of std::ref is unnecessary (both in main and invoke). It is only needed when you are passing arguments to a templated function that would normally throw away the reference qualifier, but this is not the case where you are using it.


You are storing an external reference to a std::atomic<bool>. This seems very bizarre to me because you are forcing the user of the Timer to (1) create it and (2) ensure that the reference stays valid for the life of the Timer instance. That can certainly be an annoyance with seemingly no benefit. You already have cancel() and if you want to see if it is running, just add a function for getting it.

Just make is_running_ a non-reference. (You did this in your linked version but not here)


while (time_point_cast<Timestamp::duration>(system_clock::now()) < time) {
    // wait for the time
}

This will pin the thread at 100% utilization until the condition is met. This is very bad since it could be waiting for a long time, whatever the user wants. A hot loop like this you could cool down by adding micro-sleeps while (condition) { // sleep for a few milliseconds }.

You're already using sleep_for() but there's also sleep_until(). So your while loop can be replaced with:

std::this_thread::sleep_until(time);

and your timer will consume much fewer resources. (You updated your linked version so I'm guessing you already knew this)


You should rethink your design.

Returning a std::future<void> is probably a bad idea. The destructor of std::future (when created from std::async) will block execution until the function ends. This would not allow for fire-and-forget calls, the caller has to wait for the function to be called. In a different situation you might use a future like this to return a value from the async function, but you aren't doing that here.

However, not returning it has the same problem, it just moves the waiting into schedule(). What you would need to do is hold a std::vector of std::future<void>s. This would give perspective for what all functions the Timer is juggling and forces the Timer's destructor to wait for any outstanding functions to finish.

However, that's not a good idea either; the internal list would grow for the life of the Timer with no way to purge completed functions (at least not cleanly). Instead lets look at your source of inspiration: the java.util.Timer documentation says it is:

A facility for threads to schedule tasks for future execution in a background thread...

Corresponding to each Timer object is a single background thread that is used to execute all of the timer's tasks, sequentially.

You should follow their lead. Instead of juggling multiple handles to asynchronous functions, you should have one std::thread ready to do the work, hold a queue of functions-with-times that need to be called (a std::priority_queue perhaps), and probably a std::mutex for synchronize adding and removing stuff from the queue.


This detracts from the Java version, but if you want to pass arguments to the function through additional arguments to schedule(), I'd recommend moving the task argument towards the end. This keeps the function call closer to the arguments in the parameter list.


You have no mechanism for stopping a periodic function besides stopping the whole timer. The Java version accomplishes this by using a special TimerTask type that serves as a wrapper around a Callable that adds a cancel() function. For C++ however, I'd recommend implementing cancellation tokens that a user can use to stop a periodic function, should they wish to.


Your code was very easy to review: good code style, good names, good use of using, good organization

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.