1. 2
    1. 2

      Or… don’t perform a generative stabilization loop in your destructor. This action is sufficiently meaningful to deserve its own method.

    2. 1

      I missed the problem here on first read through, because I hadn’t realised that the cleanup loop was in the destructor of the pImpl, not of the wrapper. The problem is:

      1. You reach the destructor of the public class.
      2. The class’s destructor runs to completion.
      3. The class’s fields are destroyed. At this point the public class is gone, calling methods on it is UB.
      4. The field has a destructor that keeps a reference to the public class and calls methods on it.
      5. Badness.

      The root cause has nothing to do with unique_ptr, it is about not calling methods on an object after it has been deallocated. The snark towards the libc++ implementation is completely undeserved because it is giving a hard failure in a place where you may otherwise experience memory corruption.

      This pattern needs to drain the task queue before the public class is destroyed. Something like:

      class task_manager
        std::unique_ptr<task_manager_impl> pImpl;

      This ensures that all of the tasks that hold a reference to the task_manager object are gone before the task_manager is deallocated. It’s then fine for the unique_ptr to deallocate the pImpl.

      1. 1

        I agree 100%. While there is a defined order for field destructors to be called, it’s quite error-prone (human follies) to try depending on it like this. Even if things are done perfectly right now, at some point in the future someone is going to make a small unrelated change and things will blow up. I say this having been burned by being clever in destructors before ;)

    3. 1

      I would have added an explicit “drain” method, and then call that from the destructor. The actual destruction of the impl pointer can then happen automatically. Bonus point is you can now drain without destroying.