C++ Poll Socket Wrapper Linux

Question

So I created a C++ socket wrapper class using poll and events. The creation of the listening socket and binding is already handled in the initialize() method. I'd mainly like to get feedback on the socketLoop() method, as it is the most complex one. If there are any bad habits, unhandled exceptions, bugs or anything else I can improve, please let me know. I appreciate strong criticism, feel free to be picky!

bool NetworkSocket::socketLoop(){

    // Start listening on specified port
    if (listen(this->listeningSocket, this->backlog) < 0) {
        this->raiseSocketExceptionEvent("Failed to start listening.");
        return false;
    }

    std::cout << "Listening for connections on port " << this->listening_port << std::endl;

    // Setup pollfd set and add listening socket to the set
    this->pfd.fd = this->listeningSocket; //
    this->pfd.events = POLLIN;
    this->pfd.revents = 0;
    this->readfds.push_back(this->pfd);

    while(this->run){

        char buf[256];

        // Poll on all file descriptors
        if (poll(&this->readfds[0], this->readfds.size(), -1) < 0) {
            this->raiseSocketExceptionEvent("poll() failed");
            return false;
        }

        // Loop through all descriptors and check which ones are readable
        for (int j = 0; j <  this->readfds.size(); j++) {

            // If its the listening socket continue
            if (this->readfds[j].revents == 0)
                continue;

            int sd = this->readfds[j].fd;

            // There is data to read?
            if (this->readfds[j].revents & POLLIN) {

                // If its on the listening socket its an incomin connection
                if (sd == this->listeningSocket) {
                    sockaddr_in address;
                    socklen_t addrlen = sizeof(address);

                    // Accept new connection
                    int new_socket = accept(this->listeningSocket, (struct sockaddr *) &address, &addrlen);
                    if (new_socket < 0) {
                        this->raiseSocketExceptionEvent("Failed to accept incoming connection.");
                        return false;
                    }

                    // Information about the new connection
                    this->raiseClientConnectedEvent(new_socket);

                    // Add new connection to fdset
                    this->pfd.fd = new_socket;
                    this->pfd.events = POLLIN | POLLRDHUP;
                    this->pfd.revents = 0;
                    this->readfds.push_back(this->pfd);

                    // Add new connection to vectors
                    this->clients.push_back(new_socket);

                } else {

                    // Recieve data on this connection until it fails
                    ssize_t rc = recv(sd, buf, sizeof(buf), 0);
                    if (rc > 0) {

                        std::vector<char> data(static_cast<unsigned long>(rc));
                        for (int i = 0; i < rc; i++) {
                            data[i] = buf[i];
                        }

                        this->raiseDataReceivedEvent(data); // ToDo replace with Packet structure later
                    }
                    // Connection was closed by the client
                    else if (rc == 0) {
                        this->readfds[j].revents |= POLLHUP;
                    }
                    else {
                        this->readfds[j].revents |= POLLERR;
                    }
                }
            }

            // If revents is not POLLIN its an unexpected result, exit
            if (this->readfds[j].revents != POLLIN) {
                if (sd == this->listeningSocket) {
                    std::cout << "Unknown exception..?" << std::endl;
                } else {
                    if (this->readfds[j].revents & POLLERR) {
                        this->raiseSocketExceptionEvent("Error reading client");
                    } else {
                        this->raiseClientDisconnectedEvent(sd);
                    }
                    close(sd);
                    this->clients.erase(std::find(this->clients.begin(),this->clients.end(), sd));
                    this->readfds.erase(this->readfds.begin() + j);
                    continue;
                }
            }
        }
    }
    return true;
}

Alright as requested, here is the code of the class header so you get a better understanding of the class structure:

#ifndef NETWORKSOCKET_H
#define NETWORKSOCKET_H

#include <sys/ioctl.h>
#include <iostream>
#include <sys/socket.h>
#include <netinet/in.h>
#include <vector>
#include <cstring>
#include <arpa/inet.h>
#include <unistd.h>
#include <cerrno>
#include "NetworkEventListener.h"
#include <algorithm>
#include <poll.h>
#include <thread>

class NetworkSocket
{
public:
    NetworkSocket(uint16_t port);
    ~NetworkSocket();

    bool initialize();
    void start();
    bool socketLoop();
    void stop();
    void pollThread();

    void addEventListener(NetworkEventListener *listener);
    void removeEventListener(NetworkEventListener *listener);
   bool run = true; // testing only will be replaced with getter/setter
private:

    void raiseDataSendEvent(const std::vector<char> &data) const;
    void raiseDataReceivedEvent(const std::vector<char> &data) const;
    void raiseClientConnectedEvent(int socket) const;
    void raiseClientDisconnectedEvent(int socket) const;
    void raiseSocketExceptionEvent(std::string message) const;

    std::vector<NetworkEventListener *> listeners;

    uint16_t listening_port = 0;
    int listeningSocket;
    int socketopts = 1;
    int backlog;

    std::thread socketLoopThread;

    fd_set master_fd_set;
    struct sockaddr_in address;
    std::vector<int> clients;
    std::vector<pollfd> readfds;
    pollfd pfd;

};

void socketLoopAuxiliary(NetworkSocket * socket);

#endif // NETWORKSOCKET_H

Answer 1

Clarity and Readability

I'd lose the this-> as a rule. At least in my opinion, it adds visual noise without contributing anything positive to understanding the code.

Misleading comments

    // If its the listening socket continue
    if (this->readfds[j].revents == 0)
        continue;

This comment doesn't look to me like it matches what the code is doing. Rather than checking whether this is the listening socket, it's checking whether this socket actually has an event to process (and if not, basically ignore it).

Use constructors where appropriate

For example, in this code:

                // Add new connection to fdset
                this->pfd.fd = new_socket;
                this->pfd.events = POLLIN | POLLRDHUP;
                this->pfd.revents = 0;

...The NetworkSocket knows all about the internals of the pfd. As a rule, I'd prefer code that encapsulated the details of a pfd into the pfd itself, so this ended up something like:

readfds.push_back(new pfd(new_socket, POLLIN | POLLRDHUP));
clients.push_back(new_socket);

Variable Locality

At least from the looks of things, NetworkSocket::pfd is really only needed as a kind of temporary item while accepting a new connection. If that's correct, then it should probably be local to the code that accepts a new connection, rather than being part of the whole object.

Code Structure

My immediate reaction is that socketLoop is really too large, with too many pieces that are only somewhat related all packed together into one place.

I'd rather some of the pieces of functionality inside of socketLoop were broken out into functions of their own, and it mostly called them to do the dirty work, so to speak. That would reduce the code for socketLoop to something like this:

while(run){
    char buf[256];

    // Poll on all file descriptors
    if (poll(&readfds[0], readfds.size(), -1) < 0) {
        raiseSocketExceptionEvent("poll() failed");
        return false;
    }

    for (int j = 0; j <  readfds.size(); j++) {

        if (readfds[j].revents == 0)
            continue;

        int sd = readfds[j].fd;

        if (readfds[j].revents & POLLIN) {
            if (sd == listeningSocket) {
                accept_connection(sd);
            }
            else {
                read_data(sd);
            }
        }
        else {
            handle_error(sd);
        }
    }
}

Maybe I'm not not terribly bright, but I find something like this quite a bit easier to follow.

Use range-based for loop where applicable

A loop like this:

for (int j = 0; j <  readfds.size(); j++)

...can usually be changed to a range-based for loop like this:

for (auto &fd : readfds)

...then inside the loop, fd will be equivalent to readfds[j] in your loop, so you'd use (for example):

for (auto &fd : readfds)

   if (fd.revents == 0)
       continue;

Among other things, this tends to be less prone to errors (such as using the size of one collection, but indexing into a different collection).

Prefer Data to Code

For example, instead of this:

                else if (rc == 0) {
                    readfds[j].revents |= POLLHUP;
                }
                else {
                    readfds[j].revents |= POLLERR;
                }

I'd usually use something like this:

int values = { POLLERR, POLLHUP};

readfds[j].revents = values[rc == 0];