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

Question

What's the best way to shut down a Boost thread managed by a C++ class when it's time for an object of that class to be destroyed? I have a class which creates and starts a thread on construction and provides a public Wake() method which wakes the thread when it's time to do some work. The Wake() method uses a Boost mutex and a Boost condition variable to signal the thread; the thread procedure waits on the condition variable, then does the work and goes back to waiting.

At the moment, I shut this thread down in the class's destructor, using a boolean member variable as a "running" flag; I clear the flag and then call notify_one() on the condition variable. The thread procedure then wakes up, notices that "running" is false, and returns. Here's the code:

class Worker { public:     Worker();     ~Worker();     void Wake(); private:     Worker(Worker const& rhs);             // prevent copying     Worker& operator=(Worker const& rhs);  // prevent assignment     void ThreadProc();     bool m_Running;     boost::mutex               m_Mutex;     boost::condition_variable  m_Condition;     boost::scoped_ptr<boost::thread> m_pThread; };  Worker::Worker()     : m_Running(true)     , m_Mutex()     , m_Condition()     , m_pThread() {     m_pThread.reset(new boost::thread(boost::bind(&Worker::ThreadProc, this))); }  Worker::~Worker() {     m_Running = false;     m_Condition.notify_one();     m_pThread->join(); }  void Worker::Wake() {     boost::lock_guard<boost::mutex> lock(m_Mutex);     m_Condition.notify_one(); }  void Worker::ThreadProc() {     for (;;)     {         boost::unique_lock<boost::mutex> lock(m_Mutex);         m_Condition.wait(lock);         if (! m_Running) break;         // do some work here     } } 

Is it a good idea to shut down the thread in the class's destructor like this, or should I provide a public method which lets the user do this before the object is destroyed, when there's more potential for error handling and/or forcibly destroying the thread if the thread procedure fails to return cleanly or in good time?

Cleaning up my object's mess in its destructor is appealing as it will require less attention to detail from the user (abstraction, hurrah!) but it seems to me that I should only do things in a destructor if I can guarantee to take full responsibility for cleaning things up successfully and thoroughly, and there's a small chance that code outside the class might one day need to know whether or not the thread was shut down cleanly.

Also, is the mechanism I'm using - writing to a member variable in an object on the stack of one thread and reading that variable in another thread - safe and sane?

Answer 1

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 notification Worker::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.