Safely release a resource on a different thread

Question

I have a class similiar to:

class A{
    private:
    boost::shared_ptr< Foo > m_pFoo;
}

Instances of A are destroyed on the GUI thread where they may hold the last reference to a Foo. The destructor for Foo is potentially long running, causing an undesirable pause on my GUI thread. I would like for Foo to be destroyed on a separate thread in this case, Foo's are self contained and it is not critical they get released immediately.

Currently, I use a pattern like this:

A::~A(){
    auto pMtx = boost::make_shared<boost::mutex>();
    boost::unique_lock<boost::mutex> destroyerGate(*pMtx);
    auto pFoo = m_pFoo;
    auto destroyer = [pMtx,pFoo](){
        boost::unique_lock<boost::mutex> gate(*pMtx);
    };

    m_pFoo.reset();
    pFoo.reset();
    s_cleanupThread->post(destroyer);
}

Essentially, capture it in a lambda and lock until released from the object. Is there a better way to accomplish this? This just seems more complicated than it needs to be.

Answer 1

As Mark Ransom has already suggested in the comments, you could use a dedicated destruction thread that takes the to-be-destructed objects from a work queue and then simply drops them on the floor. This works under the assumption that if you move away from an object, destructing the moved-away-from object will be very cheap.

I'm proposing a destruction_service class here, templatized on the type of object you want it to destroy. This can be any kind of object, not just shared pointers. In fact, shared pointers are even the most tricky ones because you'll have to be careful, that you only submit the std::shared_ptr for destruction if its reference count has reached one. Otherwise, destroying the std::shared_ptr on the destruction thread will basically be a no-op, except for decrementing the reference count. Nothing really bad will happen in this case, though. You'll only end up destroying the object on a thread that was not supposed to do it and therefore might be blocked longer than ideal. For debugging, you could assert in your destructor that you are not on the main thread.

I'll require that the type has a non-throwing destructor and move construction operator, though.

A destruction_service<T> maintains a std::vector<T> of the objects that are to be destructed. Submitting an object for destruction push_back()s it on that vector. The worker thread waits for the queue to become non-empty and then swap()s it with its own empty std::vector<T>. After leaving the critical section, it clear()s the vector thereby destroying all objects. The vector itself is kept around so it can be swap()ped back the next time reducing the need for dynamic memory allocations. If you're worried about the std::vectors never shrinking, consider using a std::deque instead. I'd refrain from using a std::list because it allocates memory for each item and it's somewhat paradox to allocate memory to destroy an object. The usual advantage of using a std::list as work queue is that you don't have to allocate memory in the critical section but destroying the objects is probably a low-priority task and I don't care if the worker thread is blocked a little longer than needed as long as the main thread stays responsive. There is no standard way to set the priority of a thread in C++ but if you want to, you could try giving the worker thread a low priority via the std::thread's native_handle (in the constructor of destruction_service), given your platform allows this.

The destructor of the destruction_service will join() the worker thread. As written, the class is not copyable and not movable. Put it in a smart pointer if you need to move it around.

#include <cassert>             // assert
#include <condition_variable>  // std::condition_variable
#include <mutex>               // std::mutex, std::lock_guard, std::unique_lock
#include <thread>              // std::thread
#include <type_traits>         // std::is_nothrow_{move_constructible,destructible}
#include <utility>             // std::move
#include <vector>              // std::vector


template <typename T>
class destruction_service final
{

  static_assert(std::is_nothrow_move_constructible<T>::value,
                "The to-be-destructed object needs a non-throwing move"
                " constructor or it cannot be safely delivered to the"
                " destruction thread");

  static_assert(std::is_nothrow_destructible<T>::value,
                "I'm a destruction service, not an ammunition disposal"
                " facility");

public:

  using object_type = T;

private:

  // Worker thread destroying objects.
  std::thread worker_ {};

  // Mutex to protect the object queue.
  mutable std::mutex mutex_ {};

  // Condition variable to signal changes to the object queue.
  mutable std::condition_variable condvar_ {};

  // Object queue of to-be-destructed items.
  std::vector<object_type> queue_ {};

  // Indicator that no more objects will be scheduled for destruction.
  bool done_ {};

public:

  destruction_service()
  {
    this->worker_ = std::thread {&destruction_service::do_work_, this};
  }

  ~destruction_service() noexcept
  {
    {
      const std::lock_guard<std::mutex> guard {this->mutex_};
      this->done_ = true;
    }
    this->condvar_.notify_all();
    if (this->worker_.joinable())
      this->worker_.join();
    assert(this->queue_.empty());
  }

  void
  schedule_destruction(object_type&& object)
  {
    {
      const std::lock_guard<std::mutex> guard {this->mutex_};
      this->queue_.push_back(std::move(object));
    }
    this->condvar_.notify_all();
  }

private:

  void
  do_work_()
  {
    auto things = std::vector<object_type> {};
    while (true)
      {
        {
          auto lck = std::unique_lock<std::mutex> {this->mutex_};
          if (this->done_)
            break;
          this->condvar_.wait(lck, [this](){ return !queue_.empty() || done_; });
          this->queue_.swap(things);
        }
        things.clear();
      }
    // By now, we may safely modify `queue_` without holding a lock.
    this->queue_.clear();
  }

};

Here is a simple use case:

#include <atomic>   // std::atomic_int
#include <thread>   // std::this_thread::{get_id,yield}
#include <utility>  // std::exchange

#include "destruction_service.hxx"


namespace /* anonymous */
{

  std::atomic_int example_count {};
  std::thread::id main_thread_id {};

  class example
  {

  private:

    int id_ {-1};

  public:

    example() : id_ {example_count.fetch_add(1)}
    {
      std::this_thread::yield();
    }

    example(const example& other) : id_ {other.id_}
    {
    }

    example(example&& other) noexcept : id_ {std::exchange(other.id_, -1)}
    {
    }

    ~example() noexcept
    {
      assert(this->id_ < 0 || std::this_thread::get_id() != main_thread_id);
      std::this_thread::yield();
    }

  };

}  // namespace /* anonymous */


int
main()
{
  main_thread_id = std::this_thread::get_id();
  destruction_service<example> destructor {};
  for (int i = 0; i < 12; ++i)
    {
      auto thing = example {};
      destructor.schedule_destruction(std::move(thing));
    }
}

Thanks to Barry for reviewing this code and making some good suggestions for improving it. Please see my question on Code Review for a less trimmed-down version of the code but without his suggestions incorporated.