2
\$\begingroup\$

This timer can add tasks running for one-shot, several times, or loop every time and delete task by marking its id. It's simply implemented by a priority queue.

My purpose is easy to use and implement, thread safety, and not many errors.

How can I find out bad place and improve this code?

timer.h

#include <functional>
#include <queue>
#include <map>
#include <chrono>
#include <atomic>
#include <memory>
#include <thread>
#include <future>

namespace easy {
typedef std::function<void()>                               callback_type;
typedef std::chrono::time_point<std::chrono::system_clock>  time_type;
typedef std::chrono::duration<int, std::milli>              msec_type;
struct timer_event
{
    timer_event(int id, const callback_type & callback, const time_type & when, const msec_type & duration = msec_type(0),
        bool loop = false)
        : callback(callback), when(when), duration(duration), loop(loop), id(id)
    {}
    timer_event(const timer_event & other)
        : callback(other.callback), when(other.when), duration(other.duration), loop(other.loop), id(other.id)
    {}
    timer_event(timer_event && other)
        : callback(other.callback), when(other.when), duration(other.duration), loop(other.loop), id(other.id)
    {}
    timer_event & operator=(timer_event && other)
    {
        swap(other);
        return *this;
    }
    ~timer_event() {}
    timer_event & next() { when += duration; return *this; }
    void operator()() const { callback(); }
    void swap(timer_event & other)
    {
        std::swap(callback, other.callback);
        std::swap(when, other.when);
        std::swap(duration, other.duration);
        std::swap(loop, other.loop);
        std::swap(id, other.id);
    }
    callback_type   callback;
    time_type       when;
    msec_type       duration;
    bool            loop;
    int             id;
};

struct event_less : public std::less<timer_event>
{
    bool operator()(const timer_event &e1, const timer_event &e2) const
    {
        return (e2.when < e1.when);
    }
};

class Timer
{
    std::priority_queue<timer_event, std::vector<timer_event>, event_less>  event_queue;
    std::unique_ptr<std::thread>            handler;
    std::atomic<bool>                       _stop;
    std::atomic<int>                        taskid{ 0 };
    std::map<int, bool>                     invalid;
public:
    Timer()
    {
        handler = std::make_unique<std::thread>([this]()
        {
            using namespace std::chrono;
            auto start = system_clock::now();

            while (!_stop)
            {
                time_type now = system_clock::now();

                while (!event_queue.empty() &&
                    (event_queue.top().when < now))
                {
                    auto evt = std::move(event_queue.top());

                    if (!invalid[evt.id])
                    {
                        // async/thread pool
                        if (evt.loop)
                        {
                            event_queue.emplace(evt.next());
                        }

                        std::async(std::launch::async, [evt]() { evt(); });
                    }

                    event_queue.pop();
                }

                std::this_thread::sleep_for(milliseconds(10));
            }
        });
    }
    ~Timer() { stop(); if (handler) { handler->join(); } }

    int add(const callback_type & cb, const time_type & when, const msec_type & duration = msec_type(0), int times = 0)
    {
        auto id = ++taskid;

        if (times > 0)
        {
            timer_event task{ id, cb, when, duration };
            event_queue.emplace(task);

            while (--times)
            {
                event_queue.emplace(task.next());
            }
        }
        else if (duration > msec_type(0))
        {
            event_queue.emplace(id, cb, when, duration, true);
        }
        else
        {
            event_queue.emplace(id, cb, when);
        }

        return id;
    }
    void del(int id) { invalid[id] = true; }
    void stop() { _stop = true; }
};
}

timer_test.cpp

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

using namespace std::chrono;
static auto now = system_clock::now();

void foo()
{
    std::cout << (duration<double>(system_clock::now() - now)).count() << "s => func foo" << std::endl;
}

void done()
{
    std::cout << "> " << (duration<double>(system_clock::now() - now)).count() << "s => Done!" << std::endl;
}

struct bar
{
    void hello()
    {
        std::cout << (duration<double>(system_clock::now() - now)).count() << "s => func bar::hello" << std::endl;
    }
};

int test_loop_timer()
{
    easy::Timer g_timer;
    g_timer.add([ = ]()
    {
        easy::Timer timer;
        bar b;
        std::vector<int> taskids;
        auto curr = system_clock::now();
        std::cout << "> timer " << ": " << (duration<double>(curr - now)).count() << "s => Start!" << std::endl;
        taskids.push_back(timer.add(foo, curr + seconds(2)));
        taskids.push_back(timer.add(std::bind(&bar::hello, b), curr + seconds(3), seconds(3), 3));
        taskids.push_back(timer.add(
        [ = ]() { std::cout << (duration<double>(system_clock::now() - now)).count() << "s => func lambda" << std::endl; },
        curr + seconds(4), seconds(2)));
        std::this_thread::sleep_for(seconds(10));

        for (auto id : taskids)
        {
            std::cout << "> timer " << ": del " << id << std::endl;
            timer.del(id);
        }

        timer.stop();
        curr = system_clock::now();
        std::cout << "> timer " << ": " << (duration<double>(curr - now)).count() << "s => Stop!" << std::endl;
    },
    system_clock::now() + seconds(4), seconds(15));
    std::this_thread::sleep_for(seconds(60));
    return 0;
}
int test_timer()
{
    std::vector<std::thread> timers;

    for (auto i = 0; i < 5; i++)
    {
        timers.emplace_back([i]()
        {
            std::this_thread::sleep_for(seconds(12 * i));
            easy::Timer timer;
            bar b;
            std::vector<int> taskids;
            auto curr = system_clock::now();
            std::cout << "> timer " << i << ": " << (duration<double>(curr - now)).count() << "s => Start!" << std::endl;
            taskids.push_back(timer.add(foo, curr + seconds(2)));
            taskids.push_back(timer.add(std::bind(&bar::hello, b), curr + seconds(3), seconds(3), 3));
            taskids.push_back(timer.add(
            [ = ]() { std::cout << (duration<double>(system_clock::now() - now)).count() << "s => func lambda" << std::endl; },
            curr + seconds(4), seconds(2)));
            std::this_thread::sleep_for(seconds(10));

            for (auto id : taskids)
            {
                std::cout << "> timer " << i << ": del " << id << std::endl;
                timer.del(id);
            }

            timer.stop();
            curr = system_clock::now();
            std::cout << "> timer " << i << ": " << (duration<double>(curr - now)).count() << "s => Stop!" << std::endl;
        });
    }

    for (auto & th : timers)
    {
        th.join();
    }

    done();
    return 0;
}
int main()
{
    test_loop_timer();
    //test_timer();
    return 0;
}
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! It seems like the header will need to link to pthread on linux, and I believe having a header only library with link dependencies is not a good idea in general. I've dealt with some of those, and without proper CMake it is quite hard to deal with, although this case is very simple. \$\endgroup\$ – Incomputable Oct 30 '17 at 5:28
  • \$\begingroup\$ I use visual studio to compile this code. I supposed that std::thread should be implemented above platforms. On msys2 Mingw64, this code compiled by g++, but not work!? \$\endgroup\$ – walker zheng Oct 30 '17 at 8:42
1
\$\begingroup\$

Race conditions

  • Accesses to Timer::event_queue and Timer::invalid aren't synchronized. Maybe add a std::mutex and lock it to synchronize accesses?

Implementation

  • Timer::invalid could be a std::unordered_map<int, bool>. This would reduce lookup and insertion time complexity to \$O(1)\$ (down from \$O(\log n)\$).
  • Entries in Timer::invalid never get erased. This means that in a long running program where new events are added frequently it will claim more and more memory. Even worse, if(!invalid[evt.id]) creates a new entry in invalid if invalid[evt.id] doesn't already exist.
  • start in the Timer::handler thread routine is never used.
  • Many member functions of Timer, timer_event and event_less could be marked const and/or noexcept.
  • Parameters id and loop of the first timer_eventconstructor, parameter id of Timer::del and now in the Timer::handler thread routine can be marked const.
\$\endgroup\$
  • \$\begingroup\$ Timer::invalid always increases, I supposed this timer only lives one day, only one lives every day. \$\endgroup\$ – walker zheng Oct 30 '17 at 7:45

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.