shared_from_this causing bad_weak_ptr

Question

I am trying to keep a list of connected clients in asio. I have adapted the chat server example from the docs (http://www.boost.org/doc/libs/1_57_0/doc/html/boost_asio/example/cpp03/chat/chat_server.cpp) and here's the important part of what I ended up with:

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

using boost::asio::ip::tcp;

class tcp_connection;

std::set<boost::shared_ptr<tcp_connection>> clients;

void add_client(boost::shared_ptr<tcp_connection> client)
{
    clients.insert(client);
}

class tcp_connection : public boost::enable_shared_from_this<tcp_connection>
{
public:
    tcp_connection(boost::asio::io_service& io_service) : socket_(io_service)
    {
    }

    tcp::socket socket_;

    void start()
    {
        add_client(shared_from_this());
    }

    tcp::socket& socket()
    {
        return socket_;
    }
};

class tcp_server
{
public:
    tcp_server(boost::asio::io_service& io_service)
        : io_service_(io_service),
        acceptor_(io_service, tcp::endpoint(tcp::v4(), 6767))
    {
        tcp_connection* new_connection = new tcp_connection(io_service_);
        acceptor_.async_accept(new_connection->socket(),
                             boost::bind(&tcp_server::start_accept, this, new_connection,
                                         boost::asio::placeholders::error));
    }

private:
    void start_accept(tcp_connection* new_connection,
                      const boost::system::error_code& error)
    {
        if (!error)
        {
            new_connection->start();
            new_connection = new tcp_connection(io_service_);
            acceptor_.async_accept(new_connection->socket(),
                                   boost::bind(&tcp_server::start_accept, this, new_connection,
                                               boost::asio::placeholders::error));
        }
    }

    boost::asio::io_service& io_service_;
    tcp::acceptor acceptor_;
};

int main()
{
    try
    {
        boost::asio::io_service io_service;
        tcp_server server(io_service);
        io_service.run();
    }
    catch (std::exception& e)
    {
        std::cerr << "Exception: " << e.what() << "\n";
    }

    return 0;
}

At the call to shared_from_this(), my server crashes with the message:

Exception: tr1::bad_weak_ptr

I have done some searching and it appears shared_from_this() is pretty particular, but I can't seem to find exactly what I need to change.

Answer 1

John Zwinck's essential analysis is spot on:

The bug is that you're using shared_from_this() on an object which has no shared_ptr pointing to it. This violates a precondition of shared_from_this(), namely that at least one shared_ptr must already have been created (and still exist) pointing to this.

However, his advice seems completely beside the point and dangerous in Asio code.

You should solve this by - indeed - not handling raw pointers to tcp_connection in the first place but always using shared_ptr instead.

boost::bind has the awesome feature that it binds to shared_ptr<> just fine so it automagically keeps the pointed to object alive as long as some asynchronous operation is operating on it.

This - in your sample code - means you don't need the clients vector, going the opposite way from John's answer:

void start_accept()
{
    tcp_connection::sptr new_connection = boost::make_shared<tcp_connection>(io_service_);
    acceptor_.async_accept(new_connection->socket(),
            boost::bind(
                &tcp_server::handle_accept,
                this, new_connection, asio::placeholders::error
            )
        );
}

void handle_accept(tcp_connection::sptr client, boost::system::error_code const& error)
{
    if (!error)
    {
        client->start();
        start_accept();
    }
}

I've included a sample that makes tcp_connection do some trivial work (it loops writing 'hello world' to the client each second, until the client drops the connection. When it does, you can see the destructor of the tcp_connection operation being run:

Live On Coliru

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

namespace asio = boost::asio;
using asio::ip::tcp;

class tcp_connection : public boost::enable_shared_from_this<tcp_connection>
{
public:
    typedef boost::shared_ptr<tcp_connection> sptr;

    tcp_connection(asio::io_service& io_service) : socket_(io_service), timer_(io_service)
    {
    }

    void start()
    {
        std::cout << "Created tcp_connection session\n";

        // post some work bound to this object; if you don't, the client gets
        // 'garbage collected' as the ref count goes to zero
        do_hello();
    }

    ~tcp_connection() {
        std::cout << "Destroyed tcp_connection\n";
    }

    tcp::socket& socket()
    {
        return socket_;
    }

  private:
    tcp::socket socket_;
    asio::deadline_timer timer_;

    void do_hello(boost::system::error_code const& ec = {}) {
        if (!ec) {
            asio::async_write(socket_, asio::buffer("Hello world\n"),
                    boost::bind(&tcp_connection::handle_written, shared_from_this(), asio::placeholders::error, asio::placeholders::bytes_transferred)
                );
        }
    }

    void handle_written(boost::system::error_code const& ec, size_t /*bytes_transferred*/) {
        if (!ec) {
            timer_.expires_from_now(boost::posix_time::seconds(1));
            timer_.async_wait(boost::bind(&tcp_connection::do_hello, shared_from_this(), asio::placeholders::error));
        }
    }
};

class tcp_server
{
public:
    tcp_server(asio::io_service& io_service)
        : io_service_(io_service),
          acceptor_(io_service, tcp::endpoint(tcp::v4(), 6767))
    {
        start_accept();
    }

private:
    void start_accept()
    {
        tcp_connection::sptr new_connection = boost::make_shared<tcp_connection>(io_service_);
        acceptor_.async_accept(new_connection->socket(),
                boost::bind(
                    &tcp_server::handle_accept,
                    this, new_connection, asio::placeholders::error
                )
            );
    }

    void handle_accept(tcp_connection::sptr client, boost::system::error_code const& error)
    {
        if (!error)
        {
            client->start();
            start_accept();
        }
    }

    asio::io_service& io_service_;
    tcp::acceptor acceptor_;
};

int main()
{
    try
    {
        asio::io_service io_service;
        tcp_server server(io_service);

        boost::thread(boost::bind(&asio::io_service::run, &io_service)).detach();

        boost::this_thread::sleep_for(boost::chrono::seconds(4));
        io_service.stop();
    }
    catch (std::exception& e)
    {
        std::cerr << "Exception: " << e.what() << "\n";
    }
}

Typical output:

sehe@desktop:/tmp$ time (./test& (for a in {1..4}; do nc 127.0.0.1 6767& done | nl&); sleep 2; killall nc; wait)
Created tcp_connection session
Created tcp_connection session
     1  Hello world
Created tcp_connection session
     2  Hello world
Created tcp_connection session
     3  Hello world
     4  Hello world
     5  Hello world
     6  Hello world
     7  Hello world
     8  Hello world
     9  Hello world
    10  Hello world
    11  Hello world
    12  Hello world
    13  
Destroyed tcp_connection
Destroyed tcp_connection
Destroyed tcp_connection
Destroyed tcp_connection
Destroyed tcp_connection

real    0m4.003s
user    0m0.000s
sys 0m0.015s

Answer 2

// Do not forget to ----v---- publicly inherit :)
class tcp_connection : public boost::enable_shared_from_this<tcp_connection>

Answer 3

The bug is that you're using shared_from_this() on an object which has no shared_ptr pointing to it. This violates a precondition of shared_from_this(), namely that at least one shared_ptr must already have been created (and still exist) pointing to this.

The root cause of your troubles seems to be the fact that you're storing the result of new in a raw pointer initially. You should store the result of new in a smart pointer (always, basically). Perhaps you can store the smart pointer in your clients list straight away, then.

Another approach which I mentioned in the comments is to stop using shared_from_this() entirely. You don't need it. As for this bit of code you mentioned:

if ((boost::asio::error::eof == ec) || (boost::asio::error::connection_reset == ec))
{
    clients.erase(shared_from_this());
}

You can replace it by:

if ((boost::asio::error::eof == ec) || (boost::asio::error::connection_reset == ec))
{
    boost::shared_ptr<tcp_connection> victim(this, boost::serialization::null_deleter());
    clients.erase(victim);
}

That is, create a "dumb" smart pointer which will never deallocate (https://stackoverflow.com/a/5233034/4323) but which will give you what you need to delete it from the list of clients. There are other ways to do it too, such as by searching the std::set using a comparison function which takes one shared_ptr and one raw pointer and knows to compare the addresses to which they point. It doesn't matter much which way you choose, but you escape the shared_from_this() situation entirely.

Answer 4

The answers here are great, and revealed the solution to my problem. However,

1. I searched for "bad_weak_ptr shared_from_this" and this was the top result, even though I made no mention of boost. I'm using standard C++17.
2. The OP is a specific example; a bit involved, and you have to dig through some layers of irrelevant boost socket code to get to the core issue.

In light of these two points, I thought it might be helpful to post an MRE of the root issue, and what's needed to fix it. This code is sourced from cppreference:

Problematic Code

#include <iostream>
#include <memory>
 
struct Foo : public std::enable_shared_from_this<Foo> {
    Foo() { std::cout << "Foo::Foo\n"; }
    ~Foo() { std::cout << "Foo::~Foo\n"; } 
    std::shared_ptr<Foo> getFoo() { return shared_from_this(); }
};
 
int main()
{
    try
    {
        Foo *f = new Foo;
        // Oops! this throws std::bad_weak_ptr. f is a raw pointer to Foo (not
        // managed by a shared pointer), and calling getFoo tries to create a
        // shared_ptr from the internal weak_ptr. The internal weak_ptr is nullptr,
        // so this cannot be done, and the exception is thrown. Note, the
        // cppreference link above says that prior to C++17, trying to do this
        // is undefined behavior. However, in my testing on godbolt, an exception
        // is thrown for C++11, 14, and 17 with gcc.
        std::shared_ptr<Foo> sp = f->getFoo();
    }
    catch(const std::bad_weak_ptr& bwp)
    {
        // the exception is caught, and "bad_weak_ptr" is printed to stdout
        std::cout << bwp.what();
        exit(-1);
    }

    return 0;
}

Output:

Foo::Foo
bad_weak_ptr

Potential Fix

Make sure f is managed by a shared_ptr:

try
{
    Foo *f = new Foo;
    // this time, introduce a shared pointer
    std::shared_ptr<Foo> sp(f);
    // now, f is managed by a shared pointer. Its internal weak_ptr is valid,
    // and so the retrieval of a shared_ptr via shared_from_this works as
    // desired. We can get a weak_ptr or a shared_ptr
    std::weak_ptr<Foo> wp = f->getFoo();
    std::shared_ptr<Foo> sp2 = f->getFoo();
    // all pointers go out of scope and the Foo object is deleted once
    // the reference count reaches 0
}
catch(const std::bad_weak_ptr& bwp)
{
    std::cout << bwp.what();
    exit(-1);
}

Output:

Foo::Foo
Foo::~Foo

As the other answers state and as illustrated above, you must have an object managed by a shared pointer prior to calling shared_from_this, or you will get the bad_weak_ptr exception or UB, depending on your C++ standard. Here's a playground for anyone interested.