title: Is it a good idea to shut down a class's thread member in the class's destructor?
body: <p>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 <code>Wake()</code> method which wakes the thread when it's time to do some work.  The <code>Wake()</code> 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.</p>
  
  <p>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:</p>
  
  <pre><code>class Worker
  {
  public:
      Worker();
      ~Worker();
      void Wake();
  private:
      Worker(Worker const&amp; rhs);             // prevent copying
      Worker&amp; operator=(Worker const&amp; rhs);  // prevent assignment
      void ThreadProc();
      bool m_Running;
      boost::mutex               m_Mutex;
      boost::condition_variable  m_Condition;
      boost::scoped_ptr&lt;boost::thread&gt; m_pThread;
  };
  
  Worker::Worker()
      : m_Running(true)
      , m_Mutex()
      , m_Condition()
      , m_pThread()
  {
      m_pThread.reset(new boost::thread(boost::bind(&amp;Worker::ThreadProc, this)));
  }
  
  Worker::~Worker()
  {
      m_Running = false;
      m_Condition.notify_one();
      m_pThread-&gt;join();
  }
  
  void Worker::Wake()
  {
      boost::lock_guard&lt;boost::mutex&gt; lock(m_Mutex);
      m_Condition.notify_one();
  }
  
  void Worker::ThreadProc()
  {
      for (;;)
      {
          boost::unique_lock&lt;boost::mutex&gt; lock(m_Mutex);
          m_Condition.wait(lock);
          if (! m_Running) break;
          // do some work here
      }
  }
  </code></pre>
  
  <p>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?  </p>
  
  <p>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.</p>
  
  <p>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? </p>