How should I delete a child object from within a parent's slot? Possibly boost::asio specific

Question

I have written a network server class that maintains a std::set of network clients. The network clients emit a signal to the network server on disconnect (via boost::bind). When a network client disconnects, the client instance needs to be removed from the Set and eventually deleted. I would think this is a common pattern, but I am having problems that might, or might not, be specific to ASIO.

I've tried to trim down to just the relevant code:

/** NetworkServer.hpp **/
class NetworkServices : private boost::noncopyable
{
public:
    NetworkServices(void);
    ~NetworkServices(void);

private:
    void run();
    void onNetworkClientEvent(NetworkClientEvent&); 

private:
    std::set<boost::shared_ptr<const NetworkClient>> clients;
};


/** NetworkClient.cpp **/
void NetworkServices::run()
{
    running = true;

    boost::asio::io_service::work work(io_service); //keeps service running even if no operations

    // This creates just one thread for the boost::asio async network services
    boost::thread iot(boost::bind(&NetworkServices::run_io_service, this));

    while (running)
    {
        boost::system::error_code err;
        try
        {
            tcp::socket* socket = new tcp::socket(io_service);
            acceptor->accept(*socket, err);

            if (!err)
            {
                NetworkClient* networkClient = new NetworkClient(io_service, boost::shared_ptr<tcp::socket>(socket));
                networkClient->networkClientEventSignal.connect(boost::bind(&NetworkServices::onNetworkClientEvent, this, _1));

                clients.insert(boost::shared_ptr<NetworkClient>(networkClient));

                networkClient->init(); //kicks off 1st asynch_read call
            }
        }
        // etc...

    }
}

void NetworkServices::onNetworkClientEvent(NetworkClientEvent& evt)
{
    switch(evt.getType())
    {
        case NetworkClientEvent::CLIENT_ERROR :
        {
            boost::shared_ptr<const NetworkClient> clientPtr = evt.getClient().getSharedPtr();

            // ------ THIS IS THE MAGIC LINE -----
            // If I keep this, the io_service hangs. If I comment it out,
            // everything works fine (but I never delete the disconnected NetworkClient). 

            // If actually deleted the client here I might expect problems because it is the caller
            // of this method via boost::signal and bind.  However, The clientPtr is a shared ptr, and a
            // reference is being kept in the client itself while signaling, so
            // I would the object is not going to be deleted from the heap here. That seems to be the case.
            // Never-the-less, this line makes all the difference, most likely because it controls whether or not the NetworkClient ever gets deleted.
            clients.erase(clientPtr);

            //I should probably put this socket clean-up in NetworkClient destructor. Regardless by doing this,
            // I would expect the ASIO socket stuff to be adequately cleaned-up after this.

            tcp::socket& socket = clientPtr->getSocket();
            try {
                socket.shutdown(boost::asio::socket_base::shutdown_both);
                socket.close();
            }
            catch(...) {
                CommServerContext::error("Error while shutting down and closing socket.");
            }
            break;

        }
        default :
        {
            break;
        }
    }
}



/** NetworkClient.hpp **/
class NetworkClient : public boost::enable_shared_from_this<NetworkClient>, Client
{
    NetworkClient(boost::asio::io_service& io_service,
                  boost::shared_ptr<tcp::socket> socket);
    virtual ~NetworkClient(void);

    inline boost::shared_ptr<const NetworkClient> getSharedPtr() const
    {
        return shared_from_this();
    };

    boost::signal <void (NetworkClientEvent&)>    networkClientEventSignal;

    void onAsyncReadHeader(const boost::system::error_code& error,
                           size_t bytes_transferred);

};

/** NetworkClient.cpp - onAsyncReadHeader method called from io_service.run()
    thread as result of an async_read operation. Error condition usually 
    result of an unexpected client disconnect.**/
void NetworkClient::onAsyncReadHeader(  const boost::system::error_code& error,
                        size_t bytes_transferred)
{
    if (error)
    {

        //Make sure this instance doesn't get deleted from parent/slot deferencing
        //Alternatively, somehow schedule for future delete?
        boost::shared_ptr<const NetworkClient> clientPtr = getSharedPtr();

        //Signal to service that this client is disconnecting
        NetworkClientEvent evt(*this, NetworkClientEvent::CLIENT_ERROR);
        networkClientEventSignal(evt);

        networkClientEventSignal.disconnect_all_slots();

        return;
    }

I believe it's not safe to delete the client from within the slot handler because the function return would be ... undefined? (Interestingly, it doesn't seem to blow up on me though.) So I've used boost:shared_ptr along with shared_from_this to make sure the client doesn't get deleted until all slots have been signaled. It doesn't seem to really matter though.

I believe this question is not specific to ASIO, but the problem manifests in a peculiar way when using ASIO. I have one thread executing io_service.run(). All ASIO read/write operations are performed asynchronously. Everything works fine with multiple clients connecting/disconnecting UNLESS I delete my client object from the Set per the code above. If I delete my client object, the io_service seemingly deadlocks internally and no further asynchronous operations are performed unless I start another thread. I have try/catches around the io_service.run() call and have not been able to detect any errors.

Questions:

1. Are there best practices for deleting child objects, that are also signal emitters, from within parent slots?

2. Any ideas as to why the io_service is hanging when I delete my network client object?

Answer 1

you can store weak_ptr's in the set, so shared_ptr will be kept only by asio and released on disconnection automatically. remove corresponding weak_ptr from set in ~Client()

Answer 2

I've finally figured it out, and the short answer is that it was primarily a coding/threading mistake on my part. I determined this by creating a simpler standalone code example, and found that it didn't exhibit the same behavior. I should've done this in the first place and I'm sorry to have wasted anyone's time. To answer my original questions in full though:

1 - Are there best practices for deleting child objects, that are also signal emitters, from within parent slots?

No one has really answered this. I think my suggestion and code example above where I use shared_from_this works fine.

Also, as pointed out by villintehaspam in comments above, it might be better to use boost::signal2 which seemingly has better support for managing signal lifetime.

2 - Any ideas as to why the io_service is hanging when I delete my network client object

My mistake was that the destructor in my NetworkClient was triggering an operation that caused the current thread (and only thread available to handle asych IO operations) to block indefinitely. I didn't realize that was happening. New clients were still able to connect because of how I handle the acceptor in it's own thread independent of the io_service asynch operations. Of course even though I scheduled asynch operations for the new client, they never fired because the one thread I made available to the io_service was still blocked.

Thanks to everyone that took the time to look at this.