2
\$\begingroup\$

I created a watchdog/notifier in C++ and wanted to make it better so it could be used by multiple people. The idea is that there is a timer class and event. client create events and pass a lambda function, timeout in seconds and repeat mode i.e. whether to keep the event active after a timeout or deactivate once the timeout is reached (default mode). The event can be activated and deactivated by the client on-demand or has done in the lambda function.

timer.hpp

#pragma once

#include <functional>
#include <chrono>
#include <vector>
#include <utility>
#include <set>
#include <stack>
#include <thread>
#include <mutex>
#include <condition_variable>
#include <algorithm>

struct Event
{
  unsigned int id {0};
  std::chrono::time_point<std::chrono::steady_clock> startTimepoint;
  std::chrono::seconds timeout {std::chrono::seconds::zero()};
  std::function<void(unsigned int id)> function {nullptr}; // handler
  bool isRepeated {false};
  bool isActive {false};
  bool isExecuted {false};

  Event(unsigned int p_id,
        std::chrono::seconds p_timeout,
        std::function<void(unsigned int p_id)>&& p_function, bool p_isRepeated)
    : id(p_id),
      timeout(p_timeout),
      function(p_function),
      isRepeated(p_isRepeated)
  {
  }
};

struct TimeEvent
{
  std::chrono::time_point<std::chrono::steady_clock> nextTimepoint;
  unsigned int eventID;
};

inline bool operator<(const TimeEvent& l, const TimeEvent& r)
{
  return l.nextTimepoint < r.nextTimepoint;
}


class Timer
{

public:
  Timer();
  virtual ~Timer();

  unsigned int RegisterEvent(std::function<void(unsigned int p_id)>&& p_function,
                             std::chrono::seconds timeout = std::chrono::seconds::zero(),
                             bool isRepeated = false);
  bool RemoveEvent(unsigned int id);
  bool ActivateEvent(unsigned int id, int timeout = 0);
  bool IsActivated(unsigned int id) const;
  bool DeactiveEvent(unsigned int id);
  bool DeactivateAllEvent();
  bool IsExecuted(unsigned int id) const;

private:
  void Run();

  std::mutex m_mutex {};
  std::condition_variable m_condition {};
  std::thread m_workerThread {};
  bool m_isTimerActive {true};

  std::vector<Event> m_eventsList {};
  std::set<TimeEvent> m_activeTimeEventSet {}; //Auto ordered due to set
  std::stack<unsigned int> m_freeEventIds {};
};

timer.cpp

#include "timer.hpp"
void Timer::Run()
{
  std::unique_lock<std::mutex> lock(m_mutex);

  while (m_isTimerActive)
  {
    if (m_activeTimeEventSet.empty())
    {
      m_condition.wait(lock);
    }
    else
    {
      TimeEvent te = *m_activeTimeEventSet.begin();
      if (std::chrono::steady_clock::now() >= te.nextTimepoint)
      {
        m_activeTimeEventSet.erase(m_activeTimeEventSet.begin());
        lock.unlock();        
        m_eventsList[te.eventID].function(te.eventID);
        lock.lock();
        m_eventsList[te.eventID].isExecuted = true;
        if (m_eventsList[te.eventID].isActive && m_eventsList[te.eventID].isRepeated)
        {
          te.nextTimepoint += std::chrono::duration_cast<std::chrono::seconds>(m_eventsList[te.eventID].timeout);
          m_activeTimeEventSet.insert(te);
        }
        else
        {
          m_eventsList[te.eventID].isActive = false;
        }
      }
      else
      {
        m_condition.wait_until(lock, te.nextTimepoint);
      }
    }
  }
}

Timer::Timer()
{
  std::unique_lock<std::mutex> lock(m_mutex);
  m_workerThread = std::thread([this]
  {
    Run();
  });
}

Timer::~Timer()
{
  std::unique_lock<std::mutex> lock(m_mutex);
  lock.unlock();
  m_isTimerActive = false;
  m_condition.notify_all();
  m_workerThread.join();
  m_eventsList.clear();
  m_activeTimeEventSet.clear();
  while (!m_freeEventIds.empty())
  {
    m_freeEventIds.pop();
  }
}

unsigned int Timer::RegisterEvent(std::function<void(unsigned int p_id)>&& p_function,
                                  std::chrono::seconds timeout,
                                  bool isRepeated)
{
  unsigned int id;
  std::unique_lock<std::mutex> lock(m_mutex);
  if (m_freeEventIds.empty())
  {
    id = m_eventsList.size();
    Event e(id,
            timeout,
            std::move(p_function),
            isRepeated);

    m_eventsList.push_back(std::move(e));
  }
  else
  {
    id = m_freeEventIds.top();
    Event e(id,
            timeout,
            std::move(p_function),
            isRepeated);
    m_freeEventIds.pop();
    m_eventsList[id] = std::move(e);
  }

  lock.unlock();
  m_condition.notify_all();
  return id;
}


bool Timer::ActivateEvent(unsigned int id, int timeout)
{
  std::unique_lock<std::mutex> lock(m_mutex);

  if (m_eventsList.size() == 0 || m_eventsList.size() < id)
  {
    return false;
  }
  if(timeout)
  {
    m_eventsList[id].timeout = std::chrono::seconds(timeout);
  }

  if (m_eventsList[id].timeout > std::chrono::seconds::zero())
  {
    m_eventsList[id].isActive = true;
    m_eventsList[id].isExecuted = false;
    m_eventsList[id].startTimepoint = std::chrono::steady_clock::now();

    auto it = std::find_if(m_activeTimeEventSet.begin(), m_activeTimeEventSet.end(),
                           [&](const TimeEvent & te)
    {
      return te.eventID == id;
    });

    if (it != m_activeTimeEventSet.end())
    {
      m_activeTimeEventSet.erase(it);
    }

    m_activeTimeEventSet.insert(TimeEvent {m_eventsList[id].startTimepoint + std::chrono::duration_cast<std::chrono::seconds>(m_eventsList[id].timeout),
                                           id
                                          });
  }

  lock.unlock();
  m_condition.notify_all();
  return true;
}


bool Timer::IsActivated(unsigned int id) const
{
  return m_eventsList[id].isActive;
}



bool Timer::DeactiveEvent(unsigned int id)
{
  std::unique_lock<std::mutex> lock(m_mutex);

  if (m_eventsList.size() == 0 || m_eventsList.size() < id)
  {
    return false;
  }
  m_eventsList[id].isActive = false;
  auto it = std::find_if(m_activeTimeEventSet.begin(), m_activeTimeEventSet.end(),
                         [&](const TimeEvent & te)
  {
    return te.eventID == id;
  });

  if (it != m_activeTimeEventSet.end())
  {
    m_activeTimeEventSet.erase(it);
  }

  lock.unlock();
  m_condition.notify_all();
  return true;
}


bool Timer::RemoveEvent(unsigned int id)
{
  std::unique_lock<std::mutex> lock(m_mutex);
  if (m_eventsList.size() == 0 || m_eventsList.size() < id)
  {
    return false;
  }
  m_eventsList[id].isActive = false;
  auto it = std::find_if(m_activeTimeEventSet.begin(), m_activeTimeEventSet.end(),
                         [&](const TimeEvent & te)
  {
    return te.eventID == id;
  });

  if (it != m_activeTimeEventSet.end())
  {
    m_freeEventIds.push(it->eventID);
    m_activeTimeEventSet.erase(it);
//  Note: Do not erase from eventsList, else the other ids becomes invalid
  }
  lock.unlock();
  m_condition.notify_all();
  return true;
}

bool Timer::IsExecuted(unsigned int id) const
{
  return m_eventsList[id].isExecuted;
}


bool Timer::DeactivateAllEvent()
{
  std::unique_lock<std::mutex> lock(m_mutex);
  if (m_eventsList.size() == 0)
  {
    return true;
  }

  for (unsigned int i = 0; i < m_eventsList.size(); ++i)
  {
    m_eventsList[i].isActive = false;
    m_eventsList[i].isExecuted = false;
    m_freeEventIds.push(m_eventsList[i].id);
  }

  m_activeTimeEventSet.erase(m_activeTimeEventSet.begin(), m_activeTimeEventSet.end());

  lock.unlock();
  m_condition.notify_all();
  return true;
}

client.cpp

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

using namespace std;

int quickFunc(bool &done)
{
    std::this_thread::sleep_for(std::chrono::seconds(10));
    std::cout << "MAIN: quickFunc() executed" << std::endl;
    done = true;    
}


int slowFunc(bool &done)
{
    for(size_t i = 0; i < 10; ++i)
    {
        std::this_thread::sleep_for(std::chrono::seconds(1));
        std::cout << "MAIN: slowFunc() executing" << std::endl;
    }
    std::cout << "MAIN: slowFunc() executed" << std::endl;
    done = true;    
}


int main()
{
    Timer* timer = new Timer();
    bool quickFuncDone = false;
    bool slowFuncDone = false;
    
    unsigned int id1 = timer->RegisterEvent([&](unsigned int)
    {   
            if(quickFuncDone == false)
            {
                std::cout << "MAIN: quickFunc() not executed, will not observe" << std::endl;
            }
    }, std::chrono::seconds(2));
    timer->ActivateEvent(id1);
    quickFunc(quickFuncDone);

    unsigned int id2 = timer->RegisterEvent([&](unsigned int)
    {   
            if(slowFuncDone == false)
            {
                std::cout << "WATCHDOG: slowFunc() is not yet completed, continue observing" << std::endl;
            }
            else
            {
                std::cout << "WATCHDOG: slowFunc() is completed, deactivating myself" << std::endl;
                timer->DeactiveEvent(id2);
            }

    }, std::chrono::seconds(2), true);
    timer->ActivateEvent(id2);
    slowFunc(slowFuncDone);

    std::this_thread::sleep_for(std::chrono::seconds(30));
    delete timer;
    return 0;
}

Looking forward to some good edits and suggestions so it becomes generic and can be released to the public for wider use.

\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

Move Event and TimeEvent inside class Timer

These types are just implementation details of your Timer class, and are not part of the public API. So by moving them into class Timer, you avoid polluting the global namespace. It's simply:

class Timer {
    struct Event {
        ...
    };

    struct TimeEvent {
        ...
    };

    ...
};

One issue is the operator<() for TimeEvents. You can't move that into class Timer like it is, because then it might think it's overloading Timer's own comparison operator. You can either make it friend, or just move it into struct TimeEvent; there is no reason here why it should be a free function.

Consider creating an alias for the clock

You are using std::chrono::steady_clock in a few places. It's the right clock to use, but you can avoid typing that long name and make it easier to switch to another clock later by creating an alias for it:

class Timer {
    using clock = std::chrono::steady_clock;

    struct Event {
        ...
        clock::time_point startTimepoint;
        ...
    };
    ...
};

Simplify initializing variables to zero

You are explicitly writing out the zero value for each variable you initialize, like in this line:

std::chrono::seconds timeout {std::chrono::seconds::zero()};

But you can use the empty brace syntax to do this:

std::chrono::seconds timeout {};

The same goes for 0, false, nullptr and so on. The benefit is that if you ever change the type of something, you don't need to change the value you initialized it with (unless it was a non-zero value of course).

Some types like std::mutex, as well as most containers, don't need to be explicitly "zeroed" at all, so these don't even need the {}.

No need to explicitly clear containers in the destructor

In Timer::~Timer(), you explicitly clear all the containers, but the destructor of those containers will be called automatically after your destructor, and those will take care of clearing themselves.

Ensure you handle events with identical expiration times

What if two events at some point will have the same value for nextTimePoint? The problem is that this will result in a conflict when trying to add them both to a std::set. Either ensure you disambiguate between two events in the comparison function (for example, check first if the timepoints are equal, if so compare eventID instead), or use std::multiset.

Remove useless member variables

There are some member variables that are not used at all, or don't seem to have a very useful purpose:

  • Event::startTimepoint is set at construction time but never read from.
  • Event::isExecuted is not used by the client code. Is it even necessary? If some flag is necessary, the callback function could set it itself. Also, it is reset in Timer::DeactivateAllEvent(), which is confusing to me: why would deactivating an event that has executed before reset that flag?
  • Event::isActive is unnecessary, why not just call RemoveEvent() and add it back with RegisterEvent(), or alternatively deactivate it by setting the timeout to effectively infinity.

Removing unnecessary member variables keeps structs and classes small, which is especially important if you need lots of them.

Once you remove the support for registered but inactive events, you could also consider getting rid of Timer::m_eventsList, move nextTimepoint from TimeEvent into Event, so you just have a set of Events sorted on next expiration time.

ID bookkeeping

There are some issues having an integer ID for events. You need some way to do the bookkeeping of which IDs are free. If you create a thousand events and then unregister them all, then you suddenly have Timer::m_freeEventIds containing a thousand integers. Also, despite looking up an event by ID in m_eventsList being \$\mathcal O(1)\$, you still need to use std::find_if() to find the corresponding element in m_activeTimeEventSet when deregistering an event. This is an \$\mathcal O(N)\$ operation.

If you could use C++17, I would just store the Events this way:

struct EventCmp {
    bool operator()(const std::unique_ptr<Event> &a, const std::unique_ptr<Event> &b) {
        if (a->nextTimepoint == b->nextTimepoint)
            return a.get() < b.get();
        else 
            return a->nextTimepoint == b->nextTimepoint ? ;
    }
};

std::set<std::unique_ptr<Event>, EventCmp> m_events;

And then use the raw pointer as the ID. So to register an event:

Event *Timer::RegisterEvent(...)
{
    // Allocate the event
    auto event = std::make_unique<Event>(timeout, p_function, ...);

    // Move the event into the set of events
    std::unique_lock<std::mutex> lock(m_mutex);
    m_events.insert(std::move(event));

    // Return a pointer to the event
    return event.get();
}

You could make this safer by wrapping the pointer in a class, possibly named EventHandle, to prevent the caller from modifying the event.

When you run the event loop, you move the Event out of the set using std::set::extract() and insert it back in after modifying its nextTimepoint, like so:

void Timer::Run() {
    ...
    auto node = m_events.extract(m_events.begin());
    Event *event = node.value().get();

    if (event->isRepeated)
        event->nextTimePoint += event->timeout;
    else
        event->nextTimePoint = /* infinity */;

    event->function(event);
    m_events.insert(node);
    ...
};

This requires C++17 though, as using std::set::extract() is the only way to move a std::unique_ptr in and out of a set. You can make it work with C++11 if you store raw pointers instead of std::unique_ptrs in the set, and then you first make a copy of the pointer, then erase() it from the set, and insert it afterwards again:

std::set<Event *, EventCmp> m_events;
...
void Timer::Run() {
    ...
    Event *event = *m_events.begin();
    m_events.erase(m_events.begin());

    if (event->isRepeated)
        event->nextTimePoint += event->timeout;
    else
        event->nextTimePoint = /* infinity */;

    event->function(event);
    m_events.insert(event);
    ...
};
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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