TCP socket server for UNIX

Question

I'm very new to C++, but I really want to write good code and increase my development skill, so I ask you for some review.

Scheme of socket server

This is the scheme of how my socket server works. It creates a socket, binds it, sets to listen state, creates a new thread (in order not to block the execution of the program), which accepts new connections, creates new threads for each client, reads data from them and dispatches data to observers (server is a child of Observable class, which allows to dispatch events to Observers.

#include <iostream>
#include <string.h>
#include <stdio.h>
#include <vector>
#include <arpa/inet.h>
#include <sys/socket.h>
#include <pthread.h>
#include "Observer.h"

using namespace std;

class SocketServer : public Observable
{
    private:
        string                          serverIp;
        int                             serverPort;
        int                             masterSocket;

        void                            SocketCreate ( );
        void                            SocketBind ( );
        void                            SocketListen ( );
        static void*                    SocketAccept ( void* );
        static void*                    SocketRead ( void* );

        void                            RequestDispatch ( int , string );

    public:
                                        SocketServer ( string , int );

        static void                     Send ( int , string );
        static void                     Log ( int , string );
};

struct ClientRequest
{
        int                             socketFD;
        string                          request;
};

struct ServerAndSocket
{
        SocketServer*                   socketServer;
        int                             socketFD;
};


        SocketServer::SocketServer ( string serverIp , int serverPort )
{
    this->serverIp = serverIp;
    this->serverPort = serverPort;

    this->SocketCreate();
    this->SocketBind();
    this->SocketListen();

    pthread_create ( new pthread_t() , NULL , SocketServer::SocketAccept, this );
};

void    SocketServer::SocketCreate ( )
{
    this->masterSocket = socket ( AF_INET , SOCK_STREAM , 0 );
    if ( this->masterSocket < 0 )
    {
        perror ( "socket" );
        _exit(0);
    }
    else
    {
        int opt = 1;
        setsockopt (this->masterSocket, SOL_SOCKET, SO_REUSEADDR, &opt, sizeof (opt));
        cout << "Socket created successfully with file descriptor " << this->masterSocket << "\n";
    };
};

void    SocketServer::SocketBind ( )
{
    struct                      sockaddr_in serverAddress;


    serverAddress.sin_family = AF_INET;
    serverAddress.sin_port = htons ( this->serverPort );
    serverAddress.sin_addr.s_addr = inet_addr ( this->serverIp.data());

    if ( bind ( this->masterSocket , (sockaddr*)&serverAddress , sizeof ( serverAddress ) ) < 0 )
    {
        perror ( "bind" );
        _exit(0);
    }
    else
    {
        cout << "Socket bound successfully to the " << this->serverIp << ":" << this->serverPort << " address\n";
    };
};

void    SocketServer::SocketListen ( )
{
    listen ( this->masterSocket , 5 );
    cout << "Socket is beeing listened now\n";
};

void*   SocketServer::SocketAccept ( void* serverPointer )
{
    int                                     socketFD;
    pthread_t*                              clientThread;
    ClientRequest                           clientRequest;
    SocketServer*                           this_;
    ServerAndSocket                         serverAndSocketPtr;

    this_ = (SocketServer*)serverPointer;

    while ( 1 )
    {
        socketFD = accept ( this_->masterSocket , NULL , NULL );
        if ( socketFD < 0 )
        {
            perror ( "accept" );
        }
        else
        {
            this_->RequestDispatch ( socketFD , "CLIENT_CONNECTED" );

            serverAndSocketPtr.socketServer = this_;
            serverAndSocketPtr.socketFD = socketFD;

            pthread_create ( new pthread_t() , NULL , SocketServer::SocketRead, &serverAndSocketPtr );
        };
    };
};

void*   SocketServer::SocketRead ( void* serverAndSocketPointer )
{
    char                        input[256];
    int                         inputLength;
    SocketServer*               socketServerPtr;
    int                         socketFD;
    ServerAndSocket*            serverAndSocketPtr;

    serverAndSocketPtr = (ServerAndSocket*)serverAndSocketPointer;
    socketServerPtr = serverAndSocketPtr->socketServer;
    socketFD = serverAndSocketPtr->socketFD;

    while ( 1 )
    {
        memset ( (void*)&input , '\0' , sizeof ( input ) );
        inputLength = read ( socketFD , (void*)&input , 255 );
        if ( inputLength < 0 )
        {
            perror ( "read" );
        }
        else if ( inputLength == 0 || input[0] == '\0' )
        {
            socketServerPtr->RequestDispatch ( socketFD , "CLIENT_DISCONNECTED" );
            pthread_exit( NULL );
        }
        else
        {
            socketServerPtr->RequestDispatch ( socketFD , input );
        };
    };
};



void    SocketServer::RequestDispatch ( int socketFD , string request  )
{
    struct ClientRequest                clientRequest;

    SocketServer::Log ( socketFD ,  "---> " + request );
    clientRequest.socketFD = socketFD;
    clientRequest.request = request;
    this->DispatchEvent (  (void*)&clientRequest );
};



void    SocketServer::Send ( int socketFD , string message )
{
    int                                 bytesWritten;

    bytesWritten = write ( socketFD , message.c_str() , message.size() + 1 );

    if ( bytesWritten < 0 )
    {
        perror ( "write" );
    }
    else
    {
        SocketServer::Log ( socketFD , "<--- " + message );
    };
};

void    SocketServer::Log ( int socketFD , string message )
{
    sockaddr                            address;
    socklen_t                           addressLength;
    sockaddr_in*                        addressInternet;
    string                              ip;
    int                                 port;

    getpeername ( socketFD , &address , &addressLength );
    addressInternet = (struct sockaddr_in*)&address;

    ip = inet_ntoa ( addressInternet->sin_addr );
    port = addressInternet->sin_port;

    cout << ip << ":" << port << " " <<  message << "\n";
};

A year after this code grew to a Object-oriented Linux networking library

Answer 1

#include <iostream>
#include <string.h>
#include <stdio.h>
#include <vector>
#include <arpa/inet.h>
#include <sys/socket.h>
#include <pthread.h>
#include "Observer.h"

using namespace std;

This is fine for trivial demo code and such, but more often a mistake for anything larger or more serious.

class SocketServer : public Observable {

Personally, I think I'd prefer this as something like:

namespace Socket {
    class Server {
        // ...
    };
}

---

    void                            SocketCreate();
    void                            SocketBind();
    void                            SocketListen();
    static void*                    SocketAccept(void*);
    static void*                    SocketRead(void*);

...then these would become just : Create, Bind, Listen, Accept, and Read (but the full names would be, for example, Socket::Server::Create, remaining quite self-explanatory).

SocketServer::SocketServer(string serverIp, int serverPort)
{
    this->serverIp = serverIp;
    this->serverPort = serverPort;

A member initializer list is generally preferable:

Server::Server(string serverIp, int serverPort) : serverIp(ServerIp), serverPort(serverPort) {

---

    this->SocketCreate();
    this->SocketBind();
    this->SocketListen();

this-> is ugly noise. Avoid it.

    pthread_create(new pthread_t(), NULL, SocketServer::SocketAccept, this);

If at all possible, you almost certainly want to use C++11's built-in thread support instead of using pthreads directly. The result is not only much more portable, but nearly always quite a bit cleaner as well.

void    SocketServer::SocketCreate()
{
    this->masterSocket = socket(AF_INET, SOCK_STREAM, 0);
    if (this->masterSocket < 0)

One more time: this-> is ugly noise that should be avoided whenever possible (and at least in C++, that's nearly always).

    {
        perror("socket");
        _exit(0);
    }

In my opinion, this is a terrible idea. Code at this level shouldn't set policy about how errors are handled this way. It should (probably) retrieve the error message string using strerror, then throw an exception.

    else
    {
        int opt = 1;
        setsockopt(this->masterSocket, SOL_SOCKET, SO_REUSEADDR, &opt, sizeof (opt));
        cout << "Socket created successfully with file descriptor " << this->masterSocket << "\n";

Again, using cout for something on the order of logging seems a rather poor idea.

    if (bind(this->masterSocket, (sockaddr*) &serverAddress, sizeof (serverAddress)) < 0)
    {
        perror("bind");
        _exit(0);
    }
    else
    {
        cout << "Socket bound successfully to the " << this->serverIp << ":" << this->serverPort << " address\n";
    };

I'll note what a poor idea this is one last time, and leave it at that.

    char                        input[256];

This size should probably be set with a named constant instead of a magic number.

memset((void*) &input, '\0', sizeof (input));

This seems to be pointless.You gain nothing from setting this memory to 0's immediately before read puts its data into the

        inputLength = read(socketFD, (void*) &input, 255);

This should almost certainly use something like sizeof(input)-1 instead of 255.

Answer 2

Something I hadn't seen anyone else pick up on which I think deserves mention is in regards to your Send method implementation not handling incomplete writes. According to a manual page for the write system call (and experience) you are not guaranteed that bytesWritten will be all of the bytes that you've requested: "It is not an error if this number is smaller than the number of bytes requested".

This can be remedied by adding code in your Send method to loop over the writing of the data. Something in C++11 or newer that might look like:

void SocketServer::Send(int socketFD, string message)
{
    auto bytesToSend = message.length();
    auto messagePtr = message.c_str();
    while (bytesToSend > 0)
    {
        const auto bytesWritten = write(socketFD, messagePtr, bytesToSend);
        if (bytesWritten == -1)
        {
            // do your error handling and possibly return depending on errno.
        }
        else
        {
            bytesToSend -= bytesWritten;
            messagePtr += bytesWritten;
        }
    }
}

Answer 3

I think you should probably add a few destructors (think RAII) so both the open socket and the listening sockets always get properly closed.

Oh, and while Jerry's comments are pretty good, ignore the one about using

this->

Its not 'noise', its actually a good practice according to several coding guidelines I've worked with. Your compiler doesn't need it, but it does make your code more readable to other project members.