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.
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 oldcapacity()
a reallocation takes place, in which case all iterators (including theend()
iterator) and all references to the elements are invalidated. Otherwise only theend()
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.