Is it a good idea to shut down a class's thread member in the class's destructor?

It is a good idea to release resources a class creates when the class is destroyed, even if one of the resources is a thread. If the resource is created explicitly via a user call, such as Worker::Start(), then there should also be an explicit way to release it, such as Worker::Stop(). It would also be a good idea to either perform cleanup in the destructor in the event that the user does not call Worker::Stop() and/or provide the user a scoped helper class that implements the RAII-idiom, invoking Worker::Start() in its constructor and Worker::Stop() in its destructor. However, if resource allocation is done implicitly, such as in the Worker constructor, then the release of the resource should also be implicit, leaving the destructor as the prime candidate for this responsibility.

Destruction

Lets examine Worker::~Worker(). A general rule is to not throw exceptions in destructors. If a Worker object is on a stack that is unwinding from another exception, and Worker::~Worker() throws an exception, then std::terminate() will be invoked, killing the application. While Worker::~Worker() is not explicitly throwing an exception, it is important to consider that some of the functions it is invoking may throw:

• m_Condition.notify_one() does not throw.
• m_pThread->join() could throw boost::thread_interrupted.

If std::terminate() is the desired behavior, then no change is required. However, if std::terminate() is not desired, then catch boost::thread_interrupted and suppress it.

Worker::~Worker(){  m_Running = false;  m_Condition.notify_one();  try  {    m_pThread->join();  }  catch ( const boost::thread_interrupted& )  {    /* suppressed */   }}

Concurrency

Managing threading can be tricky. It is important to define the exact desired behavior of functions like Worker::Wake(), as well as understand the behavior of the types that facilitate threading and synchronization. For example, boost::condition_variable::notify_one() has no effect if no threads are blocked in boost::condition_variable::wait(). Lets examine the possible concurrent paths for Worker::Wake().

Below is a crude attempt to diagram concurrency for two scenarios:

• Order-of-operation occurs from top-to-bottom. (i.e. Operations at the top occur before operations at the bottom.
• Concurrent operations are written on the same line.
• < and > are used to highlight when one thread is waking up or unblocking another thread. For example A > B indicates that thread A is unblocking thread B.

Scenario: Worker::Wake() invoked while Worker::ThreadProc() is blocked on m_Condition.

Other Thread                       | Worker::ThreadProc-----------------------------------+------------------------------------------                                   | lock( m_Mutex )                                   | `-- m_Mutex.lock()                                   | m_Condition::wait( lock )                                   | |-- m_Mutex.unlock()                                   | |-- waits on notificationWorker::Wake()                     | ||-- lock( m_Mutex )                | ||   `-- m_Mutex.lock()             | ||-- m_Condition::notify_one()      > |-- wakes up from notification`-- ~lock()                        | `-- m_Mutex.lock() // blocks    `-- m_Mutex.unlock()           >     `-- // acquires lock                                   | // do some work here                                   | ~lock() // end of for loop's scope                                   | `-- m_Mutex.unlock()

Result: Worker::Wake() returns fairly quickly, and Worker::ThreadProc runs.

Scenario: Worker::Wake() invoked while Worker::ThreadProc() is not blocked on m_Condition.

Other Thread                       | Worker::ThreadProc-----------------------------------+------------------------------------------                                   | lock( m_Mutex )                                   | `-- m_Mutex.lock()                                   | m_Condition::wait( lock )                                   | |-- m_Mutex.unlock()Worker::Wake()                     > |-- wakes up                                   | `-- m_Mutex.lock()Worker::Wake()                     | // do some work here|-- lock( m_Mutex )                | // still doing work...|   |-- m_Mutex.lock() // block    | // hope we do not block on a system call|   |                              | // and more work...|   |                              | ~lock() // end of for loop's scope|   |-- // still blocked           < `-- m_Mutex.unlock()|   `-- // acquires lock           | lock( m_Mutex ) // next 'for' iteration.|-- m_Condition::notify_one()      | `-- m_Mutex.lock() // blocked`-- ~lock()                        |     |-- // still blocked    `-- m_Mutex.unlock()           >     `-- // acquires lock                                   | m_Condition::wait( lock )                                       | |-- m_Mutex.unlock()                                   | `-- waits on notification                                   |     `-- still waiting...

Result: Worker::Wake() blocked as Worker::ThreadProc did work, but was a no-op, as it sent a notification to m_Condition when no one was waiting on it.

This is not particularly dangerous for Worker::Wake(), but it can cause problems in Worker::~Worker(). If Worker::~Worker() runs while Worker::ThreadProc is doing work, then Worker::~Worker() may block indefinitely when joining the thread, as the thread may not be waiting on m_Condition at the point in which it is notified, and Worker::ThreadProc only checks m_Running after it is done waiting on m_Condition.

Working Towards a Solution

In this example, lets define the following requirements:

• Worker::~Worker() will not cause std::terminate() to be invoked.
• Worker::Wake() will not block while Worker::ThreadProc is doing work.
• If Worker::Wake() is called while Worker::ThreadProc is not doing work, then it will notify Worker::ThreadProc to do work.
• If Worker::Wake() is called while Worker::ThreadProc is doing work, then it will notify Worker::ThreadProc to perform another iteration of work.
• Multiple calls to Worker::Wake() while Worker::ThreadProc is doing work will result in Worker::ThreadProc performing a single additional iteration of work.

Code:

#include <boost/thread.hpp> class Worker{public:  Worker();  ~Worker();  void Wake();private:  Worker(Worker const& rhs);             // prevent copying  Worker& operator=(Worker const& rhs);  // prevent assignment  void ThreadProc();   enum state { HAS_WORK, NO_WORK, SHUTDOWN };    state                            m_State;  boost::mutex                     m_Mutex;  boost::condition_variable        m_Condition;  boost::thread                    m_Thread;}; Worker::Worker()  : m_State(NO_WORK)  , m_Mutex()  , m_Condition()  , m_Thread(){  m_Thread = boost::thread(&Worker::ThreadProc, this);} Worker::~Worker(){  // Create scope so that the mutex is only locked when changing state and  // notifying the condition.  It would result in a deadlock if the lock was  // still held by this function when trying to join the thread.  {    boost::lock_guard<boost::mutex> lock(m_Mutex);    m_State = SHUTDOWN;    m_Condition.notify_one();  }  try { m_Thread.join(); }  catch ( const boost::thread_interrupted& ) { /* suppress */ };} void Worker::Wake(){  boost::lock_guard<boost::mutex> lock(m_Mutex);  m_State = HAS_WORK;  m_Condition.notify_one();} void Worker::ThreadProc(){  for (;;)  {    // Create scope to only lock the mutex when checking for the state.  Do    // not continue to hold the mutex wile doing busy work.    {      boost::unique_lock<boost::mutex> lock(m_Mutex);      // While there is no work (implies not shutting down), then wait on      // the condition.      while (NO_WORK == m_State)      {        m_Condition.wait(lock);        // Will wake up from either Wake() or ~Worker() signaling the condition        // variable.  At that point, m_State will either be HAS_WORK or        // SHUTDOWN.      }      // On shutdown, break out of the for loop.      if (SHUTDOWN == m_State) break;      // Set state to indicate no work is queued.      m_State = NO_WORK;    }     // do some work here  }}

Note: As a personal preference, I opted to not allocated boost::thread on the heap, and as a result, I do not need to manage it via boost::scoped_ptr. boost::thread has a default constructor that will refer to Not-a-Thread, and it is move-assignable.