Singleton Boost-based timer

Question

I would like a review on this code, and how I could make it better.

#include <boost/bind/bind.hpp>
#include <boost/date_time/posix_time/posix_time.hpp>
#include <boost/function.hpp>
#include <boost/thread/thread.hpp>
#include <boost/signals2/mutex.hpp>

#include <stdint.h>
#include <iostream>
#include <vector>

class CallBack1000 {
public:
    void Method() {
        std::cout << "1000 Millis called" << std::endl;
    }
};

class CallBack2000 {
public:
    void Method() {
        std::cout << "2000 Millis called" << std::endl;
    }
};

template <uint32_t tMilliSeconds>
class Timer {
private:
    static Timer *_instance;
    uint32_t mMilliSeconds;
    boost::mutex mListMutex;
    std::vector<boost::function<void()> > mHandlerList;

    Timer() {
        mMilliSeconds = tMilliSeconds;
    }
    Timer(const Timer &other) {}
    Timer &operator=(const Timer &other) {}

public:
    static Timer *Instance() {
        if (_instance == NULL) {
            _instance = new Timer();
        }
        return _instance;
    }

    void Run() {
        while(true) {
            boost::this_thread::sleep(
                boost::posix_time::milliseconds(mMilliSeconds));
            mListMutex.lock();
            for (std::vector<boost::function<void()> >::iterator vect_it =
                mHandlerList.begin(); vect_it != mHandlerList.end();
                ++vect_it) {

                (*vect_it)();
            }
            mListMutex.unlock();
        }
    }

    void AddHandler(boost::function<void()> tmpBoostFunction) {
        mListMutex.lock();
        mHandlerList.push_back(tmpBoostFunction);
        mListMutex.unlock();
    }
};

template<> Timer<1000> *Timer<1000>::_instance = NULL;
template<> Timer<2000> *Timer<2000>::_instance = NULL;

int main(int /*argc*/, char ** /*argv*/) {
    CallBack1000 tmpCB1000;
    boost::shared_ptr<Timer<1000> > mPtrTimer1000(Timer<1000>::Instance());
    mPtrTimer1000->AddHandler(boost::bind(&CallBack1000::Method, tmpCB1000));

    CallBack2000 tmpCB2000;
    boost::shared_ptr<Timer<2000> > mPtrTimer2000(Timer<2000>::Instance());
    mPtrTimer2000->AddHandler(boost::bind(&CallBack2000::Method, tmpCB2000));

    boost::thread tmpThread1000(
        boost::bind(&Timer<1000>::Run, mPtrTimer1000));
    boost::thread tmpThread2000(
        boost::bind(&Timer<2000>::Run, mPtrTimer2000));

    tmpThread1000.join();
    tmpThread2000.join();
}

---

EDIT

#include <boost/shared_ptr.hpp>

#include "singletontimer.h"
#include <assert.h>

class CallBack100 {
public:
    void Method() {
        std::cout << "100 Millis called" << std::endl;
    }
};

class CallBack20 {
public:
    void Method() {
        std::cout << "20 Millis called" << std::endl;
    }
};

int main(int /*argc*/, char ** /*argv*/) {
    boost::shared_ptr<Timer<20> > tmpPtrTimer_20 = Timer<20>::Instance();
    boost::shared_ptr<Timer<100> > tmpPtrTimer_100 = Timer<100>::Instance();

    boost::shared_ptr<Timer<20> > tmpPtrTimer2_20 = Timer<20>::Instance();

    CallBack20 tmpCallBack1;
    tmpPtrTimer_20->AddHandler(boost::bind(&CallBack20::Method, tmpCallBack1));
    CallBack20 tmpCallBack2;
    tmpPtrTimer2_20->AddHandler(boost::bind(&CallBack20::Method, tmpCallBack2));

    CallBack100 tmpCallBack3;
    tmpPtrTimer_100->AddHandler(boost::bind(&CallBack100::Method, tmpCallBack3));

    assert(tmpPtrTimer_20 == tmpPtrTimer2_20 && "Smart Pointers do Not Equal");

    while(true) {
        boost::this_thread::sleep(
            boost::posix_time::milliseconds(100000));
    }
}

#ifndef TIMER_H
#define TIMER_H

#include <boost/bind/bind.hpp>
#include <boost/date_time/posix_time/posix_time.hpp>
#include <boost/function.hpp>
#include <boost/thread/thread.hpp>
#include <boost/thread/recursive_mutex.hpp>
#include <boost/shared_ptr.hpp>
#include <boost/make_shared.hpp>

#include <stdint.h>
#include <iostream>
#include <vector>

template <uint32_t tMilliSeconds>
class Timer {
private:
    static Timer *_instance;
    uint32_t mMilliSeconds;
    boost::recursive_mutex mListMutex;
    boost::thread mTimerThread;
    std::vector<boost::function<void()> > mHandlerList;

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

public:
    static boost::shared_ptr<Timer<tMilliSeconds> > Instance();
    void Run();
    void AddHandler(boost::function<void()> tmpBoostFunction);
};

template <uint32_t tMilliSeconds>
Timer<tMilliSeconds>::Timer() {
    mMilliSeconds = tMilliSeconds;

    mTimerThread = boost::thread(
        boost::bind(&Timer<tMilliSeconds>::Run, this));
}

template <uint32_t tMilliSeconds>
boost::shared_ptr<Timer<tMilliSeconds> >
Timer<tMilliSeconds>::Instance() {
    if (!_instance) {
        _instance = new Timer<tMilliSeconds>();
    }
    return boost::shared_ptr<Timer<tMilliSeconds> >(_instance);
}

template <uint32_t tMilliSeconds>
void Timer<tMilliSeconds>::Run() {
    while(true) {
        boost::this_thread::sleep(
            boost::posix_time::milliseconds(mMilliSeconds));
        boost::lock_guard<boost::recursive_mutex> tmpLockGuard(mListMutex);
        for (std::vector<boost::function<void()> >::iterator vect_it =
            mHandlerList.begin(); vect_it != mHandlerList.end();
            ++vect_it) {

            (*vect_it)();
        }
    }
}

template <uint32_t tMilliSeconds>
void Timer<tMilliSeconds>::AddHandler(
    boost::function<void()> tmpBoostFunction) {

    boost::lock_guard<boost::recursive_mutex> tmpLockGuard(mListMutex);
    mHandlerList.push_back(tmpBoostFunction);
}
#endif // TIMER_H

#include "singletontimer.h"

template<> Timer<20> *Timer<20>::_instance = NULL;
template<> Timer<100> *Timer<100>::_instance = NULL;

Answer 1

If you are going to disable the copy and assignment operators then don't define the body:

Timer(const Timer &other);             // don't do this {} 
Timer &operator=(const Timer &other);  // {}

This way if you accidentally use them (inside your class or from a friend) you will get a compiler error.

Using a manual locking scheme when running code that is not yours is very dangerous.

        mListMutex.lock();

            (*vect_it)(); // Call unknown function that may throw

        mListMutex.unlock();

You need to make this exception safe with RAII

Again here:

    mListMutex.lock();
    mHandlerList.push_back(tmpBoostFunction);
    mListMutex.unlock();

vector will not break but the copy constructor of tmpBoostFunction my throw an exception.

I don't like singeltons where you use(return) pointers. The resource is not controlled. and you make it dangerous to use. Your code should be designed so that it is imposable to use incorrectly.

This it is quite legal for me to do (and potentially quite likely given a team of programmers):

boost::shared_ptr<Timer<1000> > timeer1(Timer<1000>::Instance());

// Lots of code

boost::shared_ptr<Timer<1000> > timer2(Timer<1000>::Instance());

Now the code is broken with a double delete (shared pointers do not automatically know about each other. If you wanted your singleton to controlled by smart pointers you should have returned one from the call to Instance().

I prefer the singelton to control its own lifespan:

static Timer& Instance()
{
    static Timer instance;  // lazily allocated and correctly destroyed.
    return instance;
}

Also I don't like your design of the run method.

boost::thread tmpThread1000(
    boost::bind(&Timer<1000>::Run, mPtrTimer1000));

So I have to create a thread to use your class. That breaks the rule of making it obvious to use. Your class is a timer class why am I making the thread. The call to run() should start a thread to do the dirty work then return immediately. The destructor should wait for the thread to exit (potentially sending a kill signal to the thread).

I see you are using a singeltong to try and prevent too many actual Timer objects (and thus threads) being created. But I think your design of the singelton for each period will actually result in many more timers being created.

I would have made the queue inside the Timer object a priority queue. With the job needing to go first at the head of the list. Then the timer sleeps until this jobs needs to be done executes it and moves it to the back of the list (or the appropriate place in the list). You then examine the next job and see if you need to sleep before executing again.

Comments on changed code:

You fixed the constructor/assignment operator. You fixed the RAII locking.

You have not got the singelton correct. You have just moved the problem.

template <uint32_t tMilliSeconds>
boost::shared_ptr<Timer<tMilliSeconds> >
Timer<tMilliSeconds>::Instance()
{
    if (!_instance) {
        _instance = new Timer<tMilliSeconds>();
    }
    return boost::shared_ptr<Timer<tMilliSeconds> >(_instance);
}

Every time you call Instance() you create a new shared pointer using a RAW pointer. None of these shared pointers know about each other. When creating shared pointers you need (must) create them using another shared pointer (otherwise they each individually own the pointer). i.e. Once you put a RAW pointer into a shared_pointer you should never use that pointer again (it is owned by the shared pointer) you should only use it via the owning shared pointer (or another shared pointer created from the original shared pointer (as they all share ownership)).

If you want to use shared pointers you need to do it more like this:

class Timer
{
    .....

    static boost::shared_ptr<Timer<tMilliSeconds>>   _instance;

    static boost::shared_ptr<Timer<tMilliSeconds>>   Instance()
    {
        if (!_instance.get()) {
            _instance.reset(new Timer<tMilliSeconds>());
        }
        return _instance;
    }
    ...
 }

But at this point you may as well use a unique pointer internally and return a reference.

class Timer
{
    .....

    static boost::unique_ptr<Timer<tMilliSeconds>>   _instance;

    static Timer<tMilliSeconds>&   Instance()
    {
        if (!_instance.get()) {
            _instance.reset(new Timer<tMilliSeconds>());
        }
        return *_instance;
    }
    ...
 }

But I still prefer the simpler form of singelton:

static Timer& Instance()
{
    static Timer instance;  // lazily allocated and correctly destroyed.
    return instance;
}