c++pointersmemoryvectormemory-management

Is it safe to .pop_back() from an std::vector in order to avoid pointers/memory shifting?


I have this class:

class Socket
{
    [...]
    pollfd* fdPtr; 
};

And I have a Manager class that creates the actual pollfd objects and adds them to a std::vector named pollFdsVec, to then poll on them. Meanwhile, the sockets themselves are kept in another std::vector named socketsVec.

When a socket is created (accepted) the fdPtr is set like this:

class Manager
{
    std::vector<pollfd> pollFdsVec
    std::vector<std::shared_ptr<Socket>> socketsVec;

    void Accept()
    {
        // When a new connection is created
        std::shared_ptr<Socket> sock = Accept(...); // listener returns a sharedptr
        socketsVec.push_back(sock);

        // Add the new connection to the pfds
        pollfd newPfd;
        [...]
        m_pollFdsVec.push_back(newPfd);

        // Set the pointer
        sock->fdPtr = &pollFdsVec[pollFdsVec.size() - 1]
    }

    void Poll()
    {
        int res = poll(pollFdsVec);
        [...]
    }
};

This works well, because I've made sure to call pollFdsVec.reserve(MAX_CLIENTS_CONECTED) during initialization, and because I make sure to never call push_back on the vector if the reserved size is reached, so that a relocation cannot happen.

However, I need to be able to delete elements from pollFdsVec when connections are dropped, and with this current design I'm facing the issue where after a pollFdsVec.erase() all the other fdPtr pointers lose meaning. Strangely enough, I don't get a crash, but I believe the issues I do get subsequently from the deletion come from this.

So, to fix this, at some point I collect the indexes of the elements inside pollFdsVec that I want to delete, and run this at the end of a poll cycle:

if (toRemove.size() > 0)
{
    // Remove socket from list and fds, in reverse order to avoid index shift
    std::sort(toRemove.begin(), toRemove.end(), std::greater<int>());
    for (int idx : toRemove)
    {
        std::swap(pollFdsVec[idx], pollFdsVec.back());
        std::swap(socketsVec[idx], socketsVec.back());

        // Update the socket that was sitting at the back and suffered the swap
        socketsVec[idx]->SetPfd(&m_poll_fds[idx]);

        m_poll_fds.pop_back();
        m_list.pop_back();
    }
}

But, this is my first time doing something like this. Even though it seems to work, I'm wondering if this is actually safe, and if some of you more experienced developers have ever seen situations where this could lead to potential issue in the future.

This polling cycle happens in a single thread, and yes I need each Socket to know its pollfd so I can dynamically adjust it to listen to POLLIN and/or POLLOUT events.

I've thought about saving and fixing indexes instead of a pollfd*`, but in that way I'd have to update (in the worst case) ALL the connections for every single removal, which seems less efficient.


Solution

  • You are looking for rules for iterator invalidation, specifically, for std::vector::erase:

    Iterators (including the end() iterator) and references to the elements at or after the point of the erase are invalidated.

    While for std::vector::pop_back:

    Iterators and references to the last element are invalidated. The end() iterator is also invalidated.

    Hence, it is safe to store pointers to elements in the vector and use them after calling pop_back. Only pointers to the last element are invalidated.


    For the sake of completeness, I also mention std::vector::push_back:

    If after the operation the new size() is greater than old capacity() a reallocation takes place, in which case all iterators (including the end() iterator) and all references to the elements are invalidated. Otherwise only the end() iterator is invalidated.

    Hence, your approach to reserve enough capacity upfront is sufficient to ensure push_back does not invalidate pointers to elements.


    You can consider to use a std::list which is often inferior to std::vector performance wise, but it allows insertion without invalidating iterators and even std::list::erase only invalidates iterators to the erased element.