Correct Clean-Up of Parent and Children with Callbacks (C++)

I only scratched the surface, but one can think of other potential issues.

What I'm looking for is some advice on how to handle clean destruction of the parent in the face of threads, locks, and dynamic addition/removal of children. If anybody has come up with an elegant solution that is robust under multi-threaded deployment, please share. The keyword here is robust: it's easy to design a structure that comes with some huge caveats (child never calls parent, parent never calls child, no separate thread for callbacks, etc.), the challenge is to put as few restrictions on the programmer as possible.

Often a big part of the problem with multithreading is the failure to properly separate processing (the worker thread i.e. Child) and state. Locking should be done via thread safe data structures not the threads themselves. Message queues, state machines and other such tools are intended to allow you to manage such data in a controlled way that is independent of the processes used to update them. You can almost always rework such a lifetime management problem so that it becomes a (thread safe) data management problem. The parent can be thought of as the owner of the state and the all threads update the state itself. Reference counting to manage lifetime of objects is a common paradigm also.

If there are locks in both the parent and the children (very likely in a multi-threaded non-trivial application), locking order becomes an issue: the parent invokes a method on the child, which in turn experiences a state transition and tries to notify the parent: dead-lock.

It's not clear to me why notifying the parent would cause a deadlock unless

1. the parent lock is held in thread A
2. thread A is waiting for the child to return a signal via some means
3. the child is signalling the parent in thread B
4. the parent when it receives the signal from the child in (3) attempts to obtain its lock

That's a lot of ifs. And it's a naturally problematic design: one thread (A) is holding a lock, and waiting for another thread (B) to do something.

There's no magic solution to avoid this problem - you just have to avoid it. The best answer is probably to not signal back to the parent from a separate thread; or, to distinguish between signals which will or will not be called with the parent lock already held.

During destruction of the parent, it must be on the lookout for ongoing callbacks from its children. Especially if those callbacks are fired off in a separate thread. If it is not, it might be gone by the time the callback is invoked.

The trick here is probably that the children should have a method (perhaps the destructor) which gaurentees that, after it returns, the child will make no further callbacks. When the parent is being destroyed it calls that method for each of its children.

I know you asked for "as few restrictions as possible" but realistically, when working in a multi-threaded environment, you have to have rules to prevent deadlocks and races.

Set a flag to notify the ChildDone function that the object is being deleted and wait for any running client thread to finish before returning from the destructor. This ensures that the object does not become invalid when a thread is executing ChildDone and no further calls to that function is accepted once the destructor is called. (Also see Should destructors be threadsafe?).

// Pseudocode, not compilable C++.
class Parent {

     // ....

    ~Parent() {
        mutex_.acquire();
        shuttingDown_ = true;
        mutex_.release();

        foreach (Child child in children_)
            child->detachParent();
       waitForRunningClientThreadToExit();
    }

    void ChildDone(const string& child) {
      mutex_.acquire(); 
      if (!shuttingDown_)
          cout << "Child is DONE: " << child << endl;
      mutex_.release();
    }

    bool volatile shuttingDown_ = false;
    Mutex mutex_;

    // ....

};

class Child {

    // ...

    Foo(int guess) {
       if (guess == 42) done_ = true;
       if (parent)
           parent->ChildDone(name_);
    } 

    void detachParent() {
        parent = NULL;
    }
};

There's a solution with shared_ptr, enable_shared_from_this & weak_ptr trio. Take a look at your code modified:

class Parent : public std::enable_shared_from_this<Parent> {

    ~Parent()
    {
        shuttingDown = true;
    }

    void addChild()
    {
        Child c{ "some_name", weak_from_this() };
        c.foo();
    }

    void childDone(const std::string& child)
    {
        if (!shuttingDown)
            std::cout << "Child is DONE: " << child << std::endl;
    }

    std::atomic_bool shuttingDown = false;

    struct Child {
        std::string name;
        std::weak_ptr<Parent> parent;

        void foo()
        {
            //do smth
            if (auto shared_parent = parent.lock()) {
                shared_parent->childDone(name);
            }
        }
    };
};

This code works but has one serious drawback - the parent object can't be allocated on stack, it always has to be created by making shared_ptr<Parent>. Here's a link to another question about how to get rid of that limitation.