7
\$\begingroup\$

For learning purposes I am trying to develop an event-based library/wrapper to communicate with many clients by using epoll.

The wrapper was designed with the following criteria in mind:

  1. An Object-Oriented design
  2. Very error robust - Handling of most socket exceptions
  3. Lightweight and compact

This code is based on what I've learned by studying multiple other projects and by reading the official docs. I like to know if my wrapper is using epoll as it is supposed to be used and if I can fine-tune my lib any further.

To simulate incoming client connections I used putty - for example to listen for incoming connections on port 1337 simply do: putty 127.0.0.1 -P 1337 Once the connection is established all the clients can send data to the server.

I am by no means an expert with sockets, so feel free to be very nit-picky. That helps the most to improve the code :)

This is how I realized my wrapper:

Socket.cpp

#include <iostream>
#include <fcntl.h>
#include "Socket.h"

Socket::Socket(uint16_t port) {
    this->listeningPort = port;
}


bool Socket::initialize() {

    // Setup address structs
    address.sin_family = AF_INET;
    address.sin_addr.s_addr = INADDR_ANY;
    address.sin_port = htons(listeningPort);

    // Check if port was set
    if (listeningPort == 0) {
        raiseSocketExceptionEvent("Invalid port choice.");
        return false;
    }

    // Create socket
    if (0 == (masterSocket = socket(AF_INET, SOCK_STREAM, 0))) {
        raiseSocketExceptionEvent("Failed to create socket.");
        return false;
    }

    // Set the listening socket to be non-blocking
    setNonBlocking(masterSocket);


    // Specify socket options
    if (setsockopt(masterSocket, SOL_SOCKET, SO_REUSEADDR, &this->socketOptions, sizeof(socketOptions)) == -1) {
        raiseSocketExceptionEvent("Failed to set socket options.");
        return false;
    }

    // Bind socket to port
    if (bind(masterSocket, (struct sockaddr *) &address, sizeof(address)) != 0) {
        raiseSocketExceptionEvent("Failed to bind socket.");
        return false;
    }

    return true;
}


bool Socket::start() {

    if (listen(masterSocket, SOMAXCONN) < 0) {
        raiseSocketExceptionEvent("Failed to start listening.");
        return false;
    }

    if ((epollFd = epoll_create1(0)) < 0) {
        raiseSocketExceptionEvent("Failed to create epoll instance");
        return false;
    }

    // Configure listening socket for all epoll operations
    ev.data.fd = masterSocket;
    ev.events = EPOLLIN | EPOLLPRI | EPOLLERR | EPOLLHUP | EPOLLET;
    if (epoll_ctl(epollFd, EPOLL_CTL_ADD, masterSocket, &ev) < 0) {
        raiseSocketExceptionEvent("Failed to configure epoll file descriptor");
        return false;
    }

    while (running) {

        // Get number of file descriptors that are ready for I/O
        int nfds = epoll_wait(epollFd, events, MAX_EVENTS, timeout);

        if (nfds == -1) {
            raiseSocketExceptionEvent("epoll_wait() failed to wait for events.");
            close(nfds);
            close(masterSocket);
            return false;
        }

        // Iterate all events
        for (int i = 0; i < nfds; ++i) {
            if (events[i].data.fd == masterSocket)
                handleListeningFileDescriptorActivity();
            else
                handleClientFileDescriptorActivity(i);
        }
    }

    return true;
}


// Event methods
void Socket::removeEventListener(ISocketEventListener *listener) {
    for (int i = 0; i < eventListeners.size(); i++) {
        if (eventListeners[i] == listener) {
            eventListeners.erase(eventListeners.begin() + i);
            break;
        }
    }
}

void Socket::addEventListener(ISocketEventListener *listener) {
    eventListeners.push_back(listener);
}


// Socket Events
void Socket::raiseClientDisconnectedEvent(int socket) const {
    for (int i = 0; i < eventListeners.size(); i++) {
        eventListeners[i]->onDisconnect(SocketConnectEventArgs(socket));
    }
}

void Socket::raiseClientConnectedEvent(int socket) const {
    for (int i = 0; i < eventListeners.size(); i++) {
        eventListeners[i]->onConnect(SocketConnectEventArgs(socket));
    }
}

void Socket::raiseDataSendEvent(const std::vector<char> &data, int fd) const {
    for (int i = 0; i < eventListeners.size(); i++) {
        eventListeners[i]->onDataSend(SocketDataEventArgs(data, fd));
    }
}

void Socket::raiseDataReceivedEvent(const std::vector<char> &data, int fd) const {
    for (int i = 0; i < eventListeners.size(); i++) {
        eventListeners[i]->onDataReceive(SocketDataEventArgs(data, fd));
    }
}

void Socket::raiseSocketExceptionEvent(std::string message) const {
    for (int i = 0; i < eventListeners.size(); i++) {
        eventListeners[i]->onSocketException(SocketExceptionEventArgs(message));
    }
}


// Private methods
void Socket::setNonBlocking(int socket) {
    int options;
    if ((options = fcntl(socket, F_GETFL)) < 0) {
        raiseSocketExceptionEvent("F_GETFL failed.");
        return;
    }
    options = options | O_NONBLOCK;
    if (fcntl(socket, F_SETFL, options) < 0) {
        raiseSocketExceptionEvent("F_SETFL failed.");
        return;
    }
}

void Socket::handleListeningFileDescriptorActivity() {

    // accept client connection
    socklen_t addressLen = sizeof(address);
    int fd = accept(masterSocket, (struct sockaddr *) &address, &addressLen);
    if (fd < 0) {
        raiseSocketExceptionEvent("Invalid file descriptor from accepted connection ");
        return;
    }

    // Set fd to be non blocking
    setNonBlocking(fd);

    // Monitor read operations, set edge-triggered
    ev.events = EPOLLIN | EPOLLOUT | EPOLLET;
    ev.data.fd = fd;

    // Add fd to epoll data structure
    if (epoll_ctl(epollFd, EPOLL_CTL_ADD, fd, &ev) < 0) {
        raiseSocketExceptionEvent("Failed to add connected socket fd to epoll");
        return;
    }

    // Notify that a new client connected
    raiseClientConnectedEvent(fd);
}

void Socket::handleClientFileDescriptorActivity(int index) {
    if (events[index].events & EPOLLIN) {

        // Invalid filedescriptor
        if (events[index].data.fd < 0) {
            raiseSocketExceptionEvent("Not a valid filedescriptor.");
            return;
        }

        // Data available for reading
        char buffer[512];
        ssize_t result = recv(events[index].data.fd, buffer, sizeof(buffer), 0);

        if (result < 0) {
            if (errno == ECONNRESET) {
                close(events[index].data.fd);
                raiseSocketExceptionEvent("Failed to receive data");
            }
            return;
        } else if (result == 0) {
            close(events[index].data.fd);
            raiseClientDisconnectedEvent(events[index].data.fd);

        } else {
            ev.events = EPOLLOUT | EPOLLIN | EPOLLET;
            std::vector<char> data(buffer, buffer + result);
            raiseDataReceivedEvent(data , events[index].data.fd);
        }
    }
    if (events[index].events & EPOLLOUT) {
        // ToDo Implement
        std::cout << "EPOLLOUT" << std::endl;
    }
    if (events[index].events & EPOLLRDHUP) {
        // ToDo Implement
        std::cout << "EPOLLRDHUP" << std::endl;
    }
    if (events[index].events & EPOLLHUP) {
        // ToDo Implement
        std::cout << "EPOLLHUP" << std::endl;
    }
    if (events[index].events & EPOLLERR) {
        raiseSocketExceptionEvent("Unexpected exception occured EPOLLERR");
        close(events[index].data.fd);
    }
}

Socket.h

#ifndef TINYSOCKET_SOCKET_H
#define TINYSOCKET_SOCKET_H

#include <sys/socket.h>
#include <netinet/in.h>
#include <sys/epoll.h>
#include <unistd.h>
#include <string>
#include <vector>

#include "ISocketEventListener.h"
#include "SocketExceptionEventArgs.h"
#include "SocketDataEventArgs.h"
#include "SocketConnectEventArgs.h"


class Socket {
public:


    /// Initialize new server socket
    /// \param port The listening port
    Socket(uint16_t port);


    /// Initializes the socket, bind the port to the socket etc
    /// \return True if initialization was successful, otherewise false,
    bool initialize();


    /// Start the server socket and listen for events such as incoming and outgoing
    /// connections and data as well as exceptions that occurred.
    /// \return False if an exception occurred, true if server was stopped on purpose.
    bool start();


    /// Register a socket event listener
    /// \param listener The listener which handles socket events
    void addEventListener(ISocketEventListener *listener);


    /// Removes a socket event listener
    /// \param listener The listener which handles socket events
    void removeEventListener(ISocketEventListener *listener);

private:

    /// Holds all socket event listeners
    std::vector<ISocketEventListener *> eventListeners;


    void raiseDataSendEvent(const std::vector<char> &data, int fd) const;

    void raiseDataReceivedEvent(const std::vector<char> &data, int fd) const;

    void raiseClientConnectedEvent(int socket) const;

    void raiseClientDisconnectedEvent(int socket) const;

    void raiseSocketExceptionEvent(std::string message) const;



    void setNonBlocking(int socket);

    void handleListeningFileDescriptorActivity();

    void handleClientFileDescriptorActivity(int fd);


    const int MAX_EVENTS = 20;

     struct epoll_event ev;
     struct epoll_event events[20];



    const int timeout = -1;

    struct sockaddr_in address;

    /// The port the server will listen on
    uint16_t listeningPort;

    /// The socket of the server
    int masterSocket;

    int socketOptions = 1;

    bool running = true;

    /// The filedescriptor returned from epoll_create1()
    int epollFd;


};


#endif //TINYSOCKET_SOCKET_H

ISocketEventListener.cpp

#include "ISocketEventListener.h"

void ISocketEventListener::onConnect(SocketConnectEventArgs e) {}

void ISocketEventListener::onDisconnect(SocketConnectEventArgs e) {}

void ISocketEventListener::onDataSend(SocketDataEventArgs e) {}

void ISocketEventListener::onDataReceive(SocketDataEventArgs e) {}

void ISocketEventListener::onSocketException(SocketExceptionEventArgs e) {}

ISocketEventListener.h

#ifndef TINYSOCKET_ISOCKETEVENTLISTENER_H
#define TINYSOCKET_ISOCKETEVENTLISTENER_H


#include "SocketConnectEventArgs.h"
#include "SocketDataEventArgs.h"
#include "SocketExceptionEventArgs.h"

class ISocketEventListener {
public:
    virtual void onConnect(SocketConnectEventArgs e);

    virtual void onDisconnect(SocketConnectEventArgs e);

    virtual void onDataSend(SocketDataEventArgs e);

    virtual void onDataReceive(SocketDataEventArgs e);

    virtual void onSocketException(SocketExceptionEventArgs e);

};


#endif //TINYSOCKET_ISOCKETEVENTLISTENER_H

SocketConnectEventArgs.cpp

#include "SocketConnectEventArgs.h"

SocketConnectEventArgs::SocketConnectEventArgs(int _fd) {
    fileDescriptor = _fd;
}

int SocketConnectEventArgs::getFileDescriptor() const {
    return fileDescriptor;
}

SocketConnectEventArgs.h

#ifndef TINYSOCKET_SOCKETCONNECTEVENTARGS_H
#define TINYSOCKET_SOCKETCONNECTEVENTARGS_H


class SocketConnectEventArgs {

public:
    SocketConnectEventArgs(int _fd);

    int getFileDescriptor() const;

private:
    int fileDescriptor;
};


#endif //TINYSOCKET_SOCKETCONNECTEVENTARGS_H

SocketDataEventArgs.cpp

#include "SocketDataEventArgs.h"

SocketDataEventArgs::SocketDataEventArgs(std::vector<char> _data, int _fd) {
    transfer_data = std::move(_data);
    fd = _fd;
}

std::vector<char> SocketDataEventArgs::getData() const {
    return transfer_data;
}

int SocketDataEventArgs::getFd() const {
    return fd;
}

SocketDataEventArgs.h

#ifndef TINYSOCKET_SOCKETDATAEVENTARGS_H
#define TINYSOCKET_SOCKETDATAEVENTARGS_H

#include <string>
#include <vector>

class SocketDataEventArgs {

public:

    SocketDataEventArgs(std::vector<char> _data, int fd);

    std::vector<char> getData() const;

    int getFd() const;

private:
    std::vector<char> transfer_data;
    int fd;
};


#endif //TINYSOCKET_SOCKETDATAEVENTARGS_H

SocketExceptionEventArgs.cpp

#include <iostream>
#include "SocketExceptionEventArgs.h"

SocketExceptionEventArgs::SocketExceptionEventArgs(std::string message) {
    exceptionMessage = message;
}

const std::string &SocketExceptionEventArgs::getExceptionMessage() const {
    return exceptionMessage;
}

SocketExceptionEventArgs.h

#ifndef TINYSOCKET_SOCKETEXCEPTIONEVENTARGS_H
#define TINYSOCKET_SOCKETEXCEPTIONEVENTARGS_H

#include <string>

class SocketExceptionEventArgs {
public:
    explicit SocketExceptionEventArgs(std::string message);

    const std::string &getExceptionMessage() const;

private:
    std::string exceptionMessage;
};


#endif //TINYSOCKET_SOCKETEXCEPTIONEVENTARGS_H

To create a simple server that can handle multiple clients, thee wrapper can be used like this:

main.cpp

#include <iostream>
#include "Socket.h"


class MySocketListener : public ISocketEventListener {
private:
    void onConnect(SocketConnectEventArgs e) override {
        std::cout << "Client connected | SD: " << e.getFileDescriptor() << std::endl;
    }

    void onDisconnect(SocketConnectEventArgs e) override {
        std::cout << "Client disconnected | SD: " << e.getFileDescriptor() << std::endl;
    }

    void onDataSend(SocketDataEventArgs e) override {
        std::cout << "Client sending data." << std::endl;
    }

    void onDataReceive(SocketDataEventArgs e) override {
        std::vector<char> data = e.getData();
        std::cout << "Received from fd " << e.getFd() << ": ";
        for (std::vector<char>::const_iterator i = data.begin(); i != data.end(); i++) {
            std::cout << *i;
        }

        std::cout << std::endl;
    }

    void onSocketException(SocketExceptionEventArgs e) override {
        std::cout << "[ERROR] : " << e.getExceptionMessage() << std::endl;
    }
};

int main() {

    // Declare listener for socket events
    auto eventListener = new MySocketListener();

    // Create socket object
    Socket socket(1337);

    // Add event listeners
    socket.addEventListener(eventListener);

    // Start server
    if (socket.initialize()) {
        socket.start();
    }

    return 0;
}

I appreciate all feedback.

\$\endgroup\$
3
  • \$\begingroup\$ Is there a specific reason for you not using c++11 and later versions ? Should an answer stick to pre-c++11 ? \$\endgroup\$ Commented Apr 26, 2018 at 14:14
  • \$\begingroup\$ @HaraldScheirich I am fine with C++11. However, the post is mainly related to improving the epoll loop/event structure. \$\endgroup\$
    – 766F6964
    Commented Apr 26, 2018 at 14:22
  • \$\begingroup\$ In case my post is unclear please let me know. \$\endgroup\$
    – 766F6964
    Commented Apr 27, 2018 at 9:24

1 Answer 1

2
+25
\$\begingroup\$

Sorry I can't help much with the socket programming, given the choice i'd probably go for boost::asio to handle the lower level parts. With regard to your use of epoll, I looked at the docs and that seems fine.

Single threaded implementation

One of the issues I see with your implementation is that the processing in the listeners is blocking the processing in your poll loop. That means that a if a listener is doing "work" in it's event notification callback, the event polling work stops. You might not need this in your specific application but I'd probably would like to see the polling and reception of data, and the event processing done in two separate threads.

This could be accomplished by the listener thread pushing the events onto an event queue, and a separate thread dealing with the listeners in it's own loop whenever there is work left. C++11 with std::thread and std::mutex would be helpful here.

Listener Interfaces

Your listener interface has different call signatures for each event that is fine, but wrapping the event data in separate structures seems over the top when you already have separate interfaces. E.g. the SocketConnectEventArgs has one member, the file descriptor. There is no reason for having that wrapped in a class. That lister interface might as well look like this

    class ISocketEventListener {
    public:

    virtual void onConnect(int fd);

    virtual void onDisconnect(int fd);

    virtual void onDataSend(int fd, const std::vector<char>& data);

    virtual void onDataReceive(int fd, const std::vector<char>& data);

    virtual void onSocketException(const std::string& exception);

};

Wrapping the arguments in a class would make more sense if you used one listener call signature. This might become necessary if you implement something like the queuing construct mentioned above. But then the class would probably be structured so you could push all events onto one queue. (Mainly so wouldn't have to implement 5 different queues).

If nothing else the event objects should be passed via const & e.g.

virtual void onConnect(const SocketConnectEventArgs& event);

especially for the data events this will prevent copying the data that is being passed.

Sharing raw pointers

Your listeners get registered via raw pointers, this can lead to problems when you don't know who is holding on to these pointers and who is responsible for cleaning them up. This is a case where std::shared_ptr and std::weak_ptr are really helpful, depending on how you envision the ownership of the listeners. If your Socket class holds the listener via std::weak_ptr the could be deleted from the outside and the socket class could check if they are still alive. If you want the Socket class to co-own the listeners use shared:ptr and they would stay alive as long as the socket is up.

Range based for loops

In C++11 and above its much easier to write for loops, and more idiomatic to use range based for loops, your event broadcast loops could look like this

for(auto& listener : eventListeners) {
    listener.onConnect(fd);
}

Destructor

You didn't write what you want to do with this, Socket is a class that doesn't seemed to be designed for ever being shut down. Which might be fine for your use. I'd probably still like to see an orderly shutdown for the connection, if nothing else so you don't have to research socket programming again when you actually do need it.

\$\endgroup\$
2
  • \$\begingroup\$ Thanks for the review. It certainly does include some valid points. I wonder tho if the epoll loop structure can be improved somehow. It looks really messy on my code. \$\endgroup\$
    – 766F6964
    Commented Apr 30, 2018 at 23:01
  • \$\begingroup\$ Which parts do you perceive as messy ? You are doing fairly low level programming here, a lot of times that doesn't necessarily lend it self to looking "pretty". Overall the loop looks fairly reasonable, you seem to handle a good bunch of error cases. The only other prettyfication i saw is using the event rather than an index as input to handleClientFileDescriptorActivity() \$\endgroup\$ Commented May 1, 2018 at 14:27

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.