For learning purposes I am making my own TCP Socket class.
The class is intended for handling multiple clients. Each client is stored in a vector. I am having the issue of properly removing the client from the vector when it disconnects.
How can I properly remove the client on a disconnect from the vector and how can I handle incoming data accordingly? (see else branch).
Currently the console gets spammed with the std::cout of the else case on a disconnect.
bool socks::start() {
if (listen(this->master_socket, this->backlog) !=0){
std::cerr << "Failed to start listening." << std::endl;
return false;
}
std::cout << "Listening for connections on port " << this->listening_port << std::endl;
int max_sd;
addrlen = sizeof(address);
while (true) {
//clear the socket set
FD_ZERO( & readfds);
//add master socket to set
FD_SET(master_socket, & readfds);
max_sd = master_socket;
// Add child sockets to set
for (int i = 0; i < this->clients.size();
i++){
//socket descriptor
int sd = clients[i];
// If valid socket descriptor then add to read list
if (sd > 0)
FD_SET(sd, & readfds);
//highest file descriptor number, need it for the select function
if (sd > max_sd)
max_sd = sd;
}
// Wait indefinitely for an activity on one of the sockets
int activity = select(max_sd + 1, & readfds, NULL, NULL, NULL);
if ((activity < 0) && (errno != EINTR)) {
std::cerr << "select() failed" << std::endl;
return false;
}
// Handle incoming connections
if (FD_ISSET(master_socket, & readfds)){
if ((new_socket = accept(master_socket, (struct sockaddr *) & address,(socklen_t *) & addrlen)) <0){
std::cerr << "Failed to accept incoming connection." << std::endl;
return false;
}
// Information about the new connection
std::cout << "New connection : "
<< "[SOCKET_FD : " << new_socket
<< " , IP : " << inet_ntoa(address.sin_addr)
<< " , PORT : " << ntohs(address.sin_port)
<< "]" << std::endl;
// Add connection to vector
this->clients.push_back(new_socket);
}
// Hande client disconnections / incoming data?
else{
std::cout << "Disconnect??? Or what happens here?" << std::endl;
}
}
}
EDIT : I added this to the else case:
else {
for (int j = 0; j < this->clients.size(); ++j) {
if (this->clients.at(j) == -1) {
continue; // eventually vector.erase() ?
}
if (FD_ISSET(this->clients.at(j), &this->readfds)) {
char buf[256];
ssize_t rc = recv(this->clients.at(j), buf, 256, 0);
if (rc == 0) {
std::cout << "Client disconnected! [SOCKET_FD: "
<< this->clients.at(j) << "]"
<< std::endl;
close(this->clients.at(j));
this->clients.erase(this->clients.begin() + j);
} else {
std::cout << "Client " << this->clients.at(j)
<< " sent: " << buf << std::endl;
}
}
}
}
Your select() call is asking for readable sockets only, so upon exit it will have modified your readfds to remove all non-readable sockets. So, you simply need to iterate through your clients list calling FD_ISSET() on each socket, just like you do with your master_socket. And you shouldn't be doing that iteration in an else block anyway, as the listening socket could be receiving new inbound clients at the same time that established clients are also receiving data.
Once you have determined if a given client is readable, you can then recv() data from that client, and if the recv calls returns -1 (error) or 0 (peer disconnected gracefully), close() that client and remove it from the clients list. Otherwise, act on the data as needed.
Other things to consider:
your clients list should NEVER have items with a value of -1 in it. If it does, you have bigger problems with your code that you need to fix.
don't use clients.at() in your loop, it is just wasted overhead. Use the list's operator[] instead.
if you want to modify the clients list while looping through it, DON'T increment j on every loop iteration, or you will skip a client whenever you erase a client. Otherwise, use iterators instead of indexes, as erase() returns an iterator to the next element in the list. Consider using iterators anyway, since you are erasing an iterator, not an index.
you are not handling the case where recv() may return -1 on error. You need to close() and remove failed clients, not just disconnected clients.
you are assuming recv() returns null-terminated data, which is not guaranteed even if the sender actually sends null-terminated data. TCP is a streaming transport, any given read may return fewer bytes than requested. You must pay attention to the return value of recv() to know how many bytes were actually received, otherwise you risk going out of bounds of your buffer.
Try something more like this:
bool socks::start() {
if (listen(master_socket, backlog) < 0) {
std::cerr << "Failed to start listening." << std::endl;
return false;
}
std::cout << "Listening for connections on port " << listening_port << std::endl;
fd_set readfds;
char buf[256];
while (true) {
//clear the socket set
FD_ZERO(&readfds);
//add master socket to set
FD_SET(master_socket, &readfds);
int max_sd = master_socket;
// Add child sockets to set
for (size_t i = 0; i < clients.size(); ++i) {
//socket descriptor
int sd = clients[i];
FD_SET(sd, &readfds);
//highest file descriptor number, need it for the select function
if (sd > max_sd)
max_sd = sd;
}
// Wait indefinitely for an activity on one of the sockets
int activity = select(max_sd + 1, &readfds, NULL, NULL, NULL);
if (activity < 0) {
if (errno == EINTR) continue;
std::cerr << "select() failed" << std::endl;
return false;
}
// Handle incoming connections
if (FD_ISSET(master_socket, &readfds)) {
sockaddr_in address;
socklen_t addrlen = sizeof(address);
int new_socket = accept(master_socket, (sockaddr *) &address, &addrlen);
if (new_socket < 0) {
std::cerr << "Failed to accept incoming connection." << std::endl;
return false;
}
// Information about the new connection
std::cout << "New connection : "
<< "[SOCKET_FD : " << new_socket
<< " , IP : " << inet_ntoa(address.sin_addr)
<< " , PORT : " << ntohs(address.sin_port)
<< "]" << std::endl;
// Add connection to vector
clients.push_back(new_socket);
}
// Handle client disconnections / incoming data?
size_t j = 0;
while (j < clients.size()) {
int sd = clients[j];
if (FD_ISSET(sd, &readfds)) {
ssize_t rc = recv(sd, buf, sizeof(buf), 0);
if (rc <= 0) {
std::cout << "Client " << (rc < 0) ? "read error" : "disconnected" << "! [SOCKET_FD: " << sd << "]" << std::endl;
close(sd);
clients.erase(clients.begin() + j);
continue;
}
std::cout << "Client " << sd << " sent: ";
std::cout.write(buf, rc);
std::cout << std::endl;
}
++j;
}
}
return true;
}
Do note that select() has a maximum number of sockets it can handle at a time. If you end up with more clients than select() can handle, you will have to break up the list into multiple calls to select() (possible calling them in worker threads for parallel processing), or switch to (e)poll() instead:
bool socks::start() {
if (listen(master_socket, backlog) < 0) {
std::cerr << "Failed to start listening." << std::endl;
return false;
}
std::cout << "Listening for connections on port " << listening_port << std::endl;
std::vector<pollfd> readfds;
char buf[256];
pollfd pfd;
//add master socket to set
pfd.fd = master_socket;
pfd.events = POLLIN;
pfd.revents = 0;
readfds.push_back(pfd);
while (true) {
// Wait indefinitely for an activity on one of the sockets
int activity = poll(&readfds[0], readfds.size(), -1);
if (activity < 0) {
if (errno == EINTR) continue;
std::cerr << "poll() failed" << std::endl;
return false;
}
// Handle incoming connections, client disconnections, and incoming data
size_t j = 0;
while (j < readfds.size()) {
if (readfds[j].revents == 0) {
++j;
continue;
}
int sd = readfds[j].fd;
if (readfds[j].revents & POLLIN) {
if (sd == master_socket) {
sockaddr_in address;
socklen_t addrlen = sizeof(address);
int new_socket = accept(master_socket, (struct sockaddr *) &address, &addrlen);
if (new_socket < 0) {
std::cerr << "Failed to accept incoming connection." << std::endl;
return false;
}
// Information about the new connection
std::cout << "New connection : "
<< "[SOCKET_FD : " << new_socket
<< " , IP : " << inet_ntoa(address.sin_addr)
<< " , PORT : " << ntohs(address.sin_port)
<< "]" << std::endl;
// Add connection to vectors
clients.push_back(new_socket);
pfd.fd = new_socket;
pfd.events = POLLIN | POLLRDHUP;
pfd.revents = 0;
readfds.push_back(pfd);
}
else {
ssize_t rc = recv(sd, buf, sizeof(buf), 0);
if (rc > 0) {
std::cout << "Client " << sd << " sent: ";
std::cout.write(buf, rc);
std::cout << std::endl;
}
else if (rc == 0) {
readfds[j].revents |= POLLHUP;
} else {
readfds[j].revents |= POLLERR;
}
}
}
if (readfds[j].revents != POLLIN) {
if (sd == master_socket) {
...
}
else {
std::cout << "Client " << (readfds[j].revents & POLLERR) ? "read error" : "disconnected" << "! [SOCKET_FD: " << sd << "]" << std::endl;
close(sd);
clients.erase(std::find(clients.begin(), clients.end(), sd));
readfds.erase(readfds.begin() + j);
continue;
}
}
++j;
}
}
return true;
}