std::vector::erase() (multithreaded) 'Assertion `px != 0' failed.'

Question

Similar to shared_ptr Assertion px != 0 failed

I'm writing a game server that spawns a new thread to handle each user session. The main thread has a std::vector of UserSession shared pointers. Another thread periodically removes dead sessions from this vector but fails when performing std::vector::erase(). I can't find out what is wrong for the life of me.

The error is:

Prototype2: /usr/include/boost/smart_ptr/shared_ptr.hpp:653: typename boost::detail::sp_member_access::type boost::shared_ptr::operator->() const [with T = UserSession; typename boost::detail::sp_member_access::type = UserSession*]: Assertion `px != 0' failed. Aborted (core dumped)

The relevant code is:

void GameServer::start()
{
    int sessionid;
    boost::asio::io_service io_service;
    tcp::acceptor acceptor(io_service, tcp::endpoint(tcp::v4(), port_));
    boost::thread(&GameServer::session_monitor, this);

    for (;;)
    {   
        socket_shptr socket(new tcp::socket(io_service));
        acceptor.accept(*socket);
        sessionid = numsessions_++;
        UserSession* usession = new 
            UserSession(socket, sessionid, io_service);
        session_shptr session(usession);

        sessions_mutex_.lock();
        sessions_.push_back(session);
        sessions_mutex_.unlock();

        std::cout << "Starting session for client " <<
            get_client_ip(*socket) << std::endl;

        session->start();
    }   
}

void GameServer::session_monitor()
{
    for (;;)
    {   
        boost::this_thread::sleep(boost::posix_time::seconds(10));
        std::cout << "Removing dead user sessions" << std::endl;

        sessions_mutex_.lock();
        for (std::vector<session_shptr>::iterator it = sessions_.begin();
            it != sessions_.end(); ++it)
        {
            if ((*it)->is_dead())
            {
                std::cout << "Removing session: " << (*it)->id() <<
                    std::endl;

                sessions_.erase(it);
            }
        }
        sessions_mutex_.unlock();
    }
}

I also realise there a other things wrong with this code such as the concurrent use of std::cout. — coffeebean, Jul 22 '13 at 18:26
Unrelated: Always use `make_shared`. Use a `std::lock_guard` instead of locking directly. — Casey, Jul 22 '13 at 18:26

score 5 · Accepted Answer · answered Jul 22 '13 at 18:26

Calling erase on an iterator invalidates it. You then attempt to continue to use it for iterating through the list. You need to use the return value of erase to continue iterating through the list.

    for (std::vector<session_shptr>::iterator it = sessions_.begin(); it != sessions_.end(); )
    {
        if ((*it)->is_dead())
        {
            std::cout << "Removing session: " << (*it)->id() <<
                std::endl;

            it = sessions_.erase(it);
        }
        else
          ++it;
    }

Yakk - Adam Nevraumont · Answer 2 · 2013-07-22T20:30:27.517

The right way to remove things from a std::vector is to use the remove-erase idiom. Looping over a container and manually removing elements is both annoying and is not going to be any more efficient, plus it is error prone because erase invalidates iterators on you.

std::remove and std::remove_if are already pretty slick implementations of that, and we can bundle them up with a call to erase such that the only code you write is the code that differs from one erasure to another.

Here are container-based versions of this idiom:

template<typename Container, typename Lambda>
Container&& remove_erase_if( Container&& c, Lambda&& test ) {
  using std::begin; using std::end;
  auto it = std::remove_if( begin(c), end(c), std::forward<Lambda>(test) );
  c.erase(it, c.end());
  return std::forward<Container>(c);
}
template<typename Container, typename T>
Container&& remove_erase( Container&& c, T&& test ) {
  using std::begin; using std::end;
  auto it = std::remove( begin(c), end(c), std::forward<T>(test) );
  c.erase(it, c.end());
  return std::forward<Container>(c);
}

Now your code reads like this:

sessions_mutex_.lock();
remove_erase_if( sessions_, []( session_shptr& ptr )->bool {
  if (ptr->is_dead()) {
    std::cout << "Removing session: " << ptr->id() << std::endl;
    ptr.reset(); // optional
    return true;
  } else {
    return false;
  }
});
sessions_mutex_.unlock();

or, shorter:

sessions_mutex_.lock();
remove_erase_if( sessions_, []( session_shptr& ptr ) {
  return ptr->is_dead();
});
sessions_mutex_.unlock();

As a final gotcha, note that if your destructor can be reentrant, you have to be extremely careful about the state of your std::vector -- if the destructor code causes the vector you are working on to be changed, you are in trouble.

If this is a concern, you can create a temporary vector to stuff the dead processes into:

std::vector<session_shptr> tmp;
sessions_mutex_.lock();
remove_erase_if( sessions_, []( session_shptr& ptr ) {
  if (!ptr->is_dead())
    return false;
  tmp.emplace_back( std::move(ptr) );
  return true;
});
sessions_mutex_.unlock();
tmp.clear();

which moves the destruction of sessions out of the lock (good!), and moves it out of code that is iterating over a nearly globally accessible vector (great!).

This does use a few C++11 constructs, but most can be stripped out if your compiler is pure C++03 without causing much damage. (strip out forwards, replace && with & on the Container, and replace && with const & on the Lambda and T).

You do have to write up the lambda as a function or a function object or as a bind that the erase_remove_if calls.

std::vector::erase() (multithreaded) 'Assertion `px != 0' failed.'

2 Answers2