Correct Way to Use a Mutex

Question

What I would like to know about the code below, is: Are the use of try catch blocks around method calls good practice. What are the follies in the code below?

#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::mutex mListMutex;
    boost::thread mTimerThread;
    std::vector<boost::function<void()> > mHandlerList;

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

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

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

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

template <uint32_t tMilliSeconds>
Timer<tMilliSeconds>::~Timer()  {
    mListMutex.lock();
    try {
        mTimerThread.detach();
    } catch (...) {
        mListMutex.unlock();
    }
    mListMutex.unlock();
}

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));
        mListMutex.lock();
        for (std::vector<boost::function<void()> >::iterator vect_it =
            mHandlerList.begin(); vect_it != mHandlerList.end();
            ++vect_it) {
            try {
                (*vect_it)();
            } catch (...) {
                mListMutex.unlock();
            }
        }
        mListMutex.unlock();
    }
}

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

    mListMutex.lock();
    try {
        mHandlerList.push_back(tmpBoostFunction);
    } catch (...) {
        mListMutex.unlock();
    }
    mListMutex.unlock();
}
#endif // TIMER_H

Answer 1

No, your use of catch blocks is wrong. If exceptions do occur, you are causing more unlocks then you should be. If lock() is successful then call unlock() once and only once. You should use a lock wrapper that manages the unlock for you, eg:

class mutex_lock
{
private:
    boost::mutex &mListMutex; 
public:
    mutex_lock(boost::mutex &aListMutex) : mListMutex(aListMutex) { mListMutex.lock(); }
    ~mutex_lock() { mListMutex.unlock(); }
};

Then you can do this:

template <uint32_t tMilliSeconds> 
Timer<tMilliSeconds>::~Timer()
{ 
    mutex_lock lock(mListMutex); 
    mTimerThread.detach(); 
} 

template <uint32_t tMilliSeconds> 
void Timer<tMilliSeconds>::Run()
{ 
    while(true) { 
        boost::this_thread::sleep(
            boost::posix_time::milliseconds(mMilliSeconds)); 
        mutex_lock lock(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)
{ 
    mutex_lock lock(mListMutex); 
    mHandlerList.push_back(tmpBoostFunction); 
} 

Update: boost has its own scoped_lock class for this exact same purpose:

#include <boost/interprocess/sync/scoped_lock.hpp>

template <uint32_t tMilliSeconds> 
Timer<tMilliSeconds>::~Timer()
{ 
    boost::interprocess::scoped_lock<boost::mutex> lock(mListMutex); 
    mTimerThread.detach(); 
} 

template <uint32_t tMilliSeconds> 
void Timer<tMilliSeconds>::Run()
{ 
    while(true) { 
        boost::this_thread::sleep(
            boost::posix_time::milliseconds(mMilliSeconds)); 
        boost::interprocess::scoped_lock<boost::mutex> lock(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::interprocess::scoped_lock<boost::mutex> lock(mListMutex); 
    mHandlerList.push_back(tmpBoostFunction); 
} 

Answer 2

Since you're using boost, I would look into using a mutex in conjuction with a boost::scoped_lock, so that when the scoped_lock object goes out of scope, the mutex is "automagically" unlocked via its destructor call. You then won't need to worry about interleaving mutex unlocking with your try and catch blocks, since the unwinding of the stack via an exception will release the lock on the mutex via the scoped_lock object.