How can I prevent a deadline timer from calling a function in a deleted class?

Question

I have problem in a piece of real-life code, where a function belonging to a deleted class is called by a boost::asio::deadline_timer, occasionally leading to a segmentation fault.

The issue I'm having is that the deletion of the deadline_timer is run from another timer on the same io_service. The deletion of the first deadline_timer will trigger one final call to the function to be run, with a boost::asio::error::operation_aborted error. However this can only be scheduled on the (same) io_service after the delete has finished, but by then the object is already deleted and thus no longer valid.

So my question is: how can I prevent this from happening?

The following is a simplified example with the same fault:

//============================================================================
// Name        : aTimeToKill.cpp
// Author      : Pelle
// Description : Delete an object using a timer, from a timer
//============================================================================

#include <iostream>
#include <boost/function.hpp>
#include <boost/bind.hpp>
#include <boost/asio.hpp>
#include <boost/thread.hpp>

using namespace std;
using namespace boost;

struct TimeBomb
{
    bool m_active;
    asio::deadline_timer m_runTimer;

    TimeBomb(boost::asio::io_service& ioService)
    : m_active(true)
    , m_runTimer(ioService)
    {
        cout << "Bomb placed @"<< hex << (int)this << endl;
        m_runTimer.expires_from_now(boost::posix_time::millisec(1000));
        m_runTimer.async_wait(boost::bind(&TimeBomb::executeStepFunction, this, _1));
    }

    ~TimeBomb()
    {
        m_active = false;
        m_runTimer.cancel();
        cout << "Bomb defused @"<< hex << (int)this << endl;
    }

    void executeStepFunction(const boost::system::error_code& error)
    {
        // Canceled timer
        if (error ==  boost::asio::error::operation_aborted)
        {
            std::cout << "Timer aborted: " << error.message() << " @" << std::hex << (int)this << std::endl;
            return;
        }
        if (m_active)
        {
            // Schedule next step
            cout << "tick .." <<endl;
            m_runTimer.expires_from_now(
                    boost::posix_time::millisec(1000));
            m_runTimer.async_wait(boost::bind(&TimeBomb::executeStepFunction, this, _1));
        }
    }
};

struct BomberMan
{
    asio::deadline_timer m_selfDestructTimer;
    TimeBomb* myBomb;

    BomberMan(boost::asio::io_service& ioService)
    : m_selfDestructTimer(ioService)
    {
        cout << "BomberMan ready " << endl;
        myBomb = new TimeBomb(ioService);
        m_selfDestructTimer.expires_from_now(boost::posix_time::millisec(10500));
        m_selfDestructTimer.async_wait(boost::bind(&BomberMan::defuseBomb, this, _1));
    }

    void defuseBomb(const boost::system::error_code& error)
    {
        cout << "Defusing TimeBomb" << endl;
        delete myBomb;
    }
};


int main()
{
    boost::asio::io_service m_ioService;
    BomberMan* b = new BomberMan(m_ioService);
    m_ioService.run();
    return 0;
}


./aTimeToKill
BomberMan ready 
Bomb placed @9c27198
tick ..
tick ..
tick ..
tick ..
tick ..
tick ..
tick ..
tick ..
tick ..
tick ..
Defusing TimeBomb
Bomb defused @9c27198
Timer aborted: Operation canceled @9c27198

The last line is printed after the delete, illustrating my problem.

Answer 1

For a really simple fix, how about this? (I've only included the bits you need to change)

It works because you only access stack variables on a timer cancel. You don't really need to callback the handler at all in the destructor of course, but I'm assuming your real code requires this for whatever reason.

~TimeBomb()
{
    m_active = false;
    executeStepFunction(boost::asio::error::interrupted);
    m_runTimer.cancel();
    cout << "Bomb defused @"<< hex << (int)this << endl;
}

void executeStepFunction(const boost::system::error_code& error)
{
    // Canceled timer
    if (error ==  boost::asio::error::operation_aborted)
    {
        return;
    }
    if (error ==  boost::asio::error::interrupted)
    {
        std::cout << "Timer aborted: " << error.message() << " @" << std::hex << (int)this << std::endl;
        return;
    }
...

Answer 2

The shared_ptr answer from Sam Miller works because the use of a shared_ptr keeps the TimeBomb hanging around for the lifetime of BomberMan. This might be ok for you, or it might not.

A suggestion for a more complete solution would be to obtain your TimeBomb instances from a factory which you then release them back to when finished, rather than newing and deleting them explicitly (holding them as standard pointers, not shared_ptrs as you don't own them even though you are controlling the lifecycle). The factory can keep them hanging around until they are cancelled and then delete them for you. Keep Sam Miller's stop() function as is.

To implement this, derive the factory from an interface along the lines of

class ITimeBombObserver 
{
public:
    virtual void AllOperationsComplete(TimeBomb& TmBmb)=0;
};

Pass your factory to each TimeBomb as the ITimeBombObserver on construction, and have the cancellation of the TimeBomb call this function. The factory can clean up "used" TimeBombs each time one is created or released, or using a scheduled cleanup, or some other method, whichever seems most appropriate for your application.

Using this method your BomberMan doesn't even need to explicitly release the TimeBomb in defuseBomb() if it doesn't want, the call to stop() can auto release (although in this case you should still null the pointer as it becomes effectively unusable at this point). Whether this is a good idea or not depends on your real problem, so I'll leave it to you to decide.

Answer 3

The typical recipe to solve this problem is to use a shared_ptr

#include <boost/asio.hpp>
#include <boost/bind.hpp>
#include <boost/enable_shared_from_this.hpp>
#include <boost/shared_ptr.hpp>

#include <iostream>

using namespace std;

struct TimeBomb : public boost::enable_shared_from_this<TimeBomb>
{
    bool m_active;
    boost::asio::deadline_timer m_runTimer;

    TimeBomb(boost::asio::io_service& ioService)
    : m_active(true)
    , m_runTimer(ioService)
    {
        cout << "Bomb placed @"<< hex << this << endl;
        m_runTimer.expires_from_now(boost::posix_time::millisec(1000));
    }

    void start()
    {
        m_runTimer.async_wait(boost::bind(&TimeBomb::executeStepFunction, shared_from_this(), _1));
    }

    void stop()
    {
        m_runTimer.cancel();
    }

    ~TimeBomb()
    {
        m_active = false;
        m_runTimer.cancel();
        cout << "Bomb defused @"<< hex << this << endl;
    }

    void executeStepFunction(const boost::system::error_code& error)
    {
        // Canceled timer
        if (error ==  boost::asio::error::operation_aborted)
        {
            std::cout << "Timer aborted: " << error.message() << " @" << std::hex << this << std::endl;
            return;
        }
        if (m_active)
        {
            // Schedule next step
            cout << "tick .." <<endl;
            m_runTimer.expires_from_now(
                    boost::posix_time::millisec(1000));
            m_runTimer.async_wait(boost::bind(&TimeBomb::executeStepFunction, shared_from_this(), _1));
        }
    }
};

struct BomberMan
{
    boost::asio::deadline_timer m_selfDestructTimer;
    boost::shared_ptr<TimeBomb> myBomb;

    BomberMan(boost::asio::io_service& ioService)
    : m_selfDestructTimer(ioService)
    {
        cout << "BomberMan ready " << endl;
        myBomb.reset( new TimeBomb(ioService) );
        myBomb->start();
        m_selfDestructTimer.expires_from_now(boost::posix_time::millisec(10500));
        m_selfDestructTimer.async_wait(boost::bind(&BomberMan::defuseBomb, this, _1));
    }

    void defuseBomb(const boost::system::error_code& error)
    {
        cout << "Defusing TimeBomb" << endl;
        myBomb->stop();
    }
};


int main()
{
    boost::asio::io_service m_ioService;
    BomberMan* b = new BomberMan(m_ioService);
    m_ioService.run();
    return 0;
}

Answer 4

This is why you have boost::shared_ptr and boost::enable_shared_from_this. Inherit TimeBomb class from boost::enable_shared_from_this like this:

struct TimeBomb : public boost::enable_shared_from_this< TimeBomb >
{
...
}

Instantiate a shared ptr instead of bare ptr:

boost::shared_ptr< TimeBomb > myBomb;
...
myBomb.reset( new TimeBomb(ioService) );

And finally in TimeBomb use shared_from_this() instead of this to construct handlers.

m_runTimer.async_wait( boost::bind( &TimeBomb::executeStepFunction, shared_from_this(), _1));

And of course TimeBomb class should expose a cancel method, through which you cancel an async operation, not by deleting, or in this case, resetting a shared_ptr.