C++ Linux TCP client/server socket wrapper

Question

I am relatively new to socket programming, so I would like to ask if there is anything wrong with my code :)

For the server socket I included the great new C++ lambdas, so you can start an initialized server using:

server.run([&](serverSock * serv) {...}

Moreover most things run in a new thread to be able to handle multiple clients if necessary. The readAll function is just for my project, because I tell the receiver that the message ended with the simple string "END".

Is there any bad practice, any possible bug in my code? I'm thankful for every tip! :)

clientSock.h

#ifndef CLIENTSOCK_H
#define CLIENTSOCK_H
#include <iostream>
#include <future>
#include <functional>

#include <errno.h>
#include <string.h>

#include <strings.h>
#include <sys/socket.h>
#include <netinet/in.h>
#include <netinet/tcp.h>
#include <arpa/inet.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <netdb.h>
#include <unistd.h>
#include <sys/signal.h>


using namespace std;

class clientSock {
  public:
    clientSock(string host, unsigned int port);
    clientSock();
    clientSock(int sock);
    ~clientSock();

    bool hasError();
    int connect(string host, unsigned int port);
    int disconnect();

    int write(string mesg);
    string read();
    string readAll();

    string host;
    unsigned int port;
    bool connected;

  protected:

  private:
    int enable_keepalive(int sock);

    static const unsigned int buffSize = 1000;
    int sockfd;//establish connection to ID distribution server
    struct sockaddr_in servaddr;
    char recv[buffSize];
    struct hostent* server;
};

#endif // CLIENTSOCK_H

clientSock.cpp

#include "clientSock.h"

clientSock::clientSock(string host, unsigned int port) {
    connect(host, port);
}

clientSock::clientSock() {
    connected = false;
}

clientSock::clientSock(int sock) {
    sockfd = sock;
    connected = true;
}

clientSock::~clientSock() {
    //disconnect();
}

int clientSock::connect(string host, unsigned int port) {
    clientSock::host = host;
    clientSock::port = port;

    bzero(&servaddr, sizeof(servaddr));
    servaddr.sin_family = AF_INET;
    server = gethostbyname(host.data());
    bcopy((char*) server->h_addr, (char*) &servaddr.sin_addr.s_addr, server->h_length);
    servaddr.sin_port = htons(port);

    if(connected)
        disconnect();

    sockfd = socket(AF_INET, SOCK_STREAM, 0);

    /*struct timeval tv;
    tv.tv_sec = 5;//5 Secs Timeout
    tv.tv_usec = 0;//Not init'ing this can cause strange errors
    setsockopt(sockfd, SOL_SOCKET, SO_RCVTIMEO, (char*)&tv, sizeof(struct timeval));*/

    enable_keepalive(sockfd);

    for(size_t i = 0; i < 3; i++) { //try to connect 3 times
        if(::connect(sockfd, (struct sockaddr*) &servaddr, sizeof(servaddr)) < 0)
            cerr << "Error on connecting: " << errno << "  " << strerror(errno) << endl;
        else {
            connected = true;
            return 0;
        }
    }

    connected = false;
    return 1;
}

bool clientSock::hasError() {
    if(sockfd == -1)
        return true;

    int error = 0;
    socklen_t len = sizeof(error);
    int retval = getsockopt(sockfd, SOL_SOCKET, SO_ERROR, &error, &len);

    if(retval != 0 || error != 0)
        return true;
    else
        return false;
}

int clientSock::enable_keepalive(int sock) {
    int yes = 1;

    if(setsockopt(sock, SOL_SOCKET, SO_KEEPALIVE, &yes, sizeof(int)) == -1)
        return -1;

    int idle = 1;

    if(setsockopt(sock, IPPROTO_TCP, TCP_KEEPIDLE, &idle, sizeof(int)) == -1)
        return -1;

    int interval = 1;

    if(setsockopt(sock, IPPROTO_TCP, TCP_KEEPINTVL, &interval, sizeof(int)) == -1)
        return -1;

    int maxpkt = 10;

    if(setsockopt(sock, IPPROTO_TCP, TCP_KEEPCNT, &maxpkt, sizeof(int)) == -1)
        return -1;

    return 0;
}

int clientSock::disconnect() {
    if(!connected)
        return -1;

    close(sockfd);
    connected = false;

    return 0;
}

int clientSock::write(string mesg) {
    if(!connected)
        return 1;

    struct timeval tv;
    tv.tv_sec = 10;
    tv.tv_usec = 0;
    fd_set writefds;
    FD_ZERO(&writefds);
    FD_SET(sockfd, &writefds);

    //cout << "w: " << mesg << endl;

    int sentBytes = 0;

    for(size_t i = 0; i < mesg.length(); i += sentBytes) {
        FD_ZERO(&writefds);
        FD_SET(sockfd, &writefds);
        int rv = select(sockfd + 1, NULL, &writefds, NULL, &tv);

        if(rv == -1)
            cerr << errno << "  " << strerror(errno) << endl;
        else if(rv == 0)
            sentBytes = 0;
        else if(rv > 0 && FD_ISSET(sockfd, &writefds)) {
            sentBytes = ::write(sockfd, mesg.substr(i, mesg.length() - i).c_str(), mesg.length() - i);

            if(sentBytes == -1) {
                cerr << "Error sending IDs: " << errno << "  " << strerror(errno) << endl;
                return 1;
            }
        }
    }

    return 0;
}

string clientSock::read() {
    if(!connected)
        return "";

    struct timeval tv;
    tv.tv_sec = 10;
    tv.tv_usec = 0;
    fd_set readfds;
    FD_ZERO(&readfds);
    FD_SET(sockfd, &readfds);

    string resp = "";
    unsigned int n = 0;


    do {
        FD_ZERO(&readfds);
        FD_SET(sockfd, &readfds);
        int rv = select(sockfd + 1, &readfds, NULL, NULL, &tv);

        if(rv <= -1)
            cerr << errno << "  " << strerror(errno) << endl;
        else if(rv == 0)
            break;
        else if(rv > 0 && FD_ISSET(sockfd, &readfds)) {

            int tn = ::read(sockfd, recv, buffSize - 1);//avoid signcompare warning

            if(tn > 0) {
                n = tn;
                recv[n] = '\0';
                string tResp(recv, n);
                resp += tResp;
            }
            else if(tn == -1) {
                if(errno == 11) { //get the good part of the received stuff also if the connection closed during receive -> happens sometimes with short messages
                    string tResp(recv);

                    if(tResp.find_first_not_of("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ01234567890") == std::string::npos) //but only allow valid chars
                        resp += tResp;
                }
                else
                    cerr << errno << "  " << strerror(errno) << endl;

                break;
            }
            else
                break;

        }
        else
            cerr << "ERROR: rv: " << rv << endl;

    }
    while(n >= buffSize - 1);

    //if(resp != "")
    //cout << "r: " << resp << endl;

    return resp;
}

string clientSock::readAll() {
    string full = read();

    while(full.find("END") == string::npos)
        full += read();

    full = full.substr(0, full.find("END"));

    return full;
}

serverSock.h

#ifndef SERVERSOCK_H
#define SERVERSOCK_H
#include <iostream>
#include <future>
#include <functional>

#include <clientSock.h>

#include <errno.h>
#include <string.h>

#include <strings.h>
#include <sys/socket.h>
#include <netinet/in.h>
#include <arpa/inet.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <netdb.h>
#include <unistd.h>

using namespace std;


class serverSock {
  public:
    serverSock(unsigned int port);
    serverSock();
    ~serverSock();

    clientSock accept();

    bool inited;

    int init(unsigned int port);//!<Necessary only if not already done by using constructor with port
    int start();
    int run(std::function<void(serverSock* sock)> fn);
    int runBlocking(std::function<void(serverSock* sock)> fn);
    int stop();

  protected:
    future<void> thr;

  private:
    int enable_keepalive(int sock);

    int sockfd;//start server to listen for clients to send them ids
    socklen_t clilen;
    struct sockaddr_in servaddr, cliaddr;
    static const unsigned int buffSize = 1000;
    char mesg[buffSize];

    unsigned int listeningPort;
    bool started;
    bool stopSock;

};

#endif // SERVERSOCK_H

serverSock.cpp

#include "serverSock.h"

serverSock::serverSock(unsigned int port) : stopSock(false) {
    inited = false;
    init(port);
    inited = true;
}

serverSock::serverSock() {
    listeningPort = 0;
}

serverSock::~serverSock() {
    stop();
}

int serverSock::init(unsigned int port) {
    listeningPort = port;
    sockfd = socket(AF_INET, SOCK_STREAM, 0);
    int yes = 1;

    if(setsockopt(sockfd, SOL_SOCKET, SO_REUSEADDR, &yes, sizeof(yes)) == -1)
        cerr << errno << "  " << strerror(errno) << endl;

    enable_keepalive(sockfd);

    bzero(&servaddr, sizeof(servaddr));
    servaddr.sin_family = AF_INET;
    servaddr.sin_addr.s_addr = htonl(INADDR_ANY);
    servaddr.sin_port = htons(port);
    started = false;
    return 0;
}

int serverSock::enable_keepalive(int sock) {
    int yes = 1;

    if(setsockopt(sock, SOL_SOCKET, SO_KEEPALIVE, &yes, sizeof(int)) == -1) {
        cerr << errno << "  " << strerror(errno) << endl;
        return -1;
    }

    int idle = 1;

    if(setsockopt(sock, IPPROTO_TCP, TCP_KEEPIDLE, &idle, sizeof(int)) == -1) {
        cerr << errno << "  " << strerror(errno) << endl;
        return -1;
    }

    int interval = 1;

    if(setsockopt(sock, IPPROTO_TCP, TCP_KEEPINTVL, &interval, sizeof(int)) == -1) {
        cerr << errno << "  " << strerror(errno) << endl;
        return -1;
    }

    int maxpkt = 10;

    if(setsockopt(sock, IPPROTO_TCP, TCP_KEEPCNT, &maxpkt, sizeof(int)) == -1) {
        cerr << errno << "  " << strerror(errno) << endl;
        return -1;
    }

    return 0;
}

clientSock serverSock::accept() {
    if(started) {
        int newsockfd = ::accept(sockfd, (struct sockaddr*) &cliaddr, &clilen);

        if(newsockfd == -1)
            cerr << errno << "  " << strerror(errno) << endl;

        return clientSock(newsockfd);
    }
    else return clientSock();
}

int serverSock::start() {
    if(listeningPort == 0) {
        cerr << "ERROR No port defined to listen to" << endl;
        return 1;
    }

    if(bind(sockfd, (struct sockaddr*) &servaddr, sizeof(servaddr)) < 0) {
        cerr << errno << "  " << strerror(errno) << endl;
        return 1;
    }

    listen(sockfd, 5);
    clilen = sizeof(cliaddr);

    started = true;
    stopSock = false;

    return 0;
}

int serverSock::run(std::function<void(serverSock* sock)> fn) {
    thr = std::async([this, fn]() {
        while(!stopSock)
            fn(this);
    });
    return 0;
}

int serverSock::runBlocking(std::function<void(serverSock* sock)> fn) {
    while(!stopSock)
        fn(this);

    return 0;
}

int serverSock::stop() {
    stopSock = true;
    int res = close(sockfd);

    if(res < 0)
        cerr << errno << "  " << strerror(errno) << endl;

    return 0;
}

Answer 1

Design

This is a very thin C like wrapper around sockets. In my opinion there are a lot of style changes that are need to make this good C++ or usable by modern C++ library.

Main issues:

• Two phase construction
• Error Codes escape public interface (use exception)
• You member variables have state not related to the object after construction.
• Your use of select() is incorrect for this context.

I did not review the serverSock but most comments that apply to the clientSock also apply to the server socket.

Self Plug / Further reading

I started writing a series of blogs about Socket programming; Still not complete. But as part of that series you will find a simple socket wrapper class.

• Socket Programming in C
• Socket Read/Write
• C++ Wrapper for Socket
• Socket Protocols

The code is:

• reviewed here - C++ Socket Part-2
• source here

Code Review

Don't use using namespace

using namespace std;

This is particularly bad here because it is in a header file. You should not use it in a source file either (bad habit).

Doing so makes your code much more susceptible to bugs, read: Why is “using namespace std” considered bad practice?

Putting it in a header file is worse because it can pollute other people's code (any code that includes your header file). If I include your header file in my code it could potentially break my code in subtle ways. Things like that get you band from projects.

Its a bad habit and you should break it as soon as possible.

The reason the "Standard" namespace is called std:: is so that it is short and therefore not "much" of a burden to use as a prefix in-front of Types/Objects.

Public Member Variables are always a bad idea:

Anybody can come along and change the state of your object without you being able to tell.

class clientSock {
  public:
    .....
    string host;
    unsigned int port;
    bool connected;

Member objects should always be private with access controlled via public/protected member methods.

Never Use Two Phase Construction

An object should be created correctly or not created at all. Never use the constructor to try and initialize an object that fails; then force the user to check to the state with an additional call. This will result in your users code having bugs as users of your code will forget to check the state.

This is your second phase check.

    bool hasError();

If the constructor fails then throw an exception.

Error Codes

Error codes are great inside your object to indicate error states internally. But error codes should never escape your public interface. Prefer to throw an exception rather than force the user of your object to check an error code to verify correctness.

These return error codes.

    int connect(string host, unsigned int port);
    int disconnect();

A user of your code will eventually forget to check an error code thus resulting in bug.

If something goes wrong that you can't fix locally then throw an exception. If the user of the code does not catch the exception then it will correctly terminate the application (any other action is a bug without knowing the context). You can then find this in testing and add the appropriate code to fix the problem (or ignore it as appropriate).

Semantics and Rule of Three

The compiler generates the copy constructor and copy assignment operator for you by default. So the following are not your only constructors.

class clientSock {
  public:
    clientSock(string host, unsigned int port);
    clientSock();
    clientSock(int sock);
    ~clientSock();

Compiler generated:

    clientSock(clientSock const&);
    clientSock& operator=(clientSock const&);

This allows:

{
    clientSock  sock1("google.com", 80);
    clientSock  sock2(sock1);
}
// Destructor calls close on sock2
// Destructor calls close on sock1 (same as sock1)
// Probably not devastating but definitely untidy.

Personally I would disable the copy semantics of the object.

Conversely I would enable the move semantics of a socket object. So that it can be passed into functions etc.

    clientSock(clientSock&&);             // Notice the double &&
    clientSock& operator=(clientSock&&);

Interface design

    string readAll();

Read everything? You have not defined a protocol at this level. So how do you know what everything is?

Member Vs Local variable

These two are not really part of the state of the object.

    struct sockaddr_in servaddr;
    struct hostent* server;

They are used to set up the connection. But are not used after that point. So not really much point in keeping them as members.

Initializer List

clientSock::clientSock(string host, unsigned int port) {
    connect(host, port);
}

OK I am not going to say its totally wrong. But I think you should set up member variables in the constructor before calling member functions. Currently the state of the object is random (for POD values) and member functions assume (especially public facing ones) assume that the state of the object is well defined.

Now using member function to set the state is OK (if you have a couple of constructors and some large piece of common code). But I don't think this falls into the same category. So I would initialize the members then call connect().

clientSock::clientSock(string host, unsigned int port)
    : host("")
    , port(-1)
    , connected(false)   // this is the important one as you check it in connect.
    , sock(-1)
{
    connect(host, port);
}

Good practice to initialize all members so they have defined values. In the next one port will have an indeterminate value.

clientSock::clientSock() {
    connected = false;
}

This one shows that host and port are not really part of the state of the object. They should be removed from the object. As soon as you have a socket them they are no longer relevant.

clientSock::clientSock(int sock) {
    sockfd = sock;
    connected = true;
}

Why is the disconnect commented out?

clientSock::~clientSock() {
    //disconnect();
}

Maybe you need to check if it is connected before disconnecting.

Member accesses

    clientSock::host = host;
    clientSock::port = port;

This is a funny way of accessing member variables. Normally I would use this-> to specify the exact member (rather than className:: as that implies static members).

But better yet is not never to shadow member variables. Shadowed variables will eventually cause a problem as you will forget to disambiguify them from the shadow and your compile will not warn you when you go wrong.

A nice trick is to turn on your compiler warnings to tell you about shadowed variables and to treat them as errors.

Comments

I hate bad comments. But this would be a nice place for a comment.

    setsockopt(sockfd, SOL_SOCKET, SO_RCVTIMEO, (char*)&tv, sizeof(struct timeval));*/

    enable_keepalive(sockfd);

Magic Numbers

Put magic numbers (like 3) into constants.

    for(size_t i = 0; i < 3; i++) { //try to connect 3 times
        if(::connect(sockfd, (struct sockaddr*) &servaddr, sizeof(servaddr)) < 0)
            cerr << "Error on connecting: " << errno << "  " << strerror(errno) << endl;
        else {
            connected = true;
            return 0;
        }
    }

But why try and connect three times? Is there some reason you think things will change? If they do change what does that mean about the error that happened the first two times? This code is too low level to make decisions like this. If there is an error here you should be getting a bit of code that has much more context to make the decision to try and reconnect again.

Boolean results with if

    if(retval != 0 || error != 0)
        return true;
    else
        return false;

This can be much more logically represented as:

    return (retval != 0 || error != 0);

Is it really an error to try and disconnect when not connected?

int clientSock::disconnect() {
    if(!connected)
        return -1;        // This is an error state?

    close(sockfd);
    connected = false;

    return 0;
}

Pass parameters by const reference (when they are non-mutable objects).

int clientSock::write(string mesg) {

You just made a copy of mesg. If this is a large string then that is a waste of time since you will just be reading it.

int clientSock::write(string const& mesg) {

Select

You are using select() incorrectly. It's not going to break anything as it is used here. But this is not what it is designed for.

        FD_ZERO(&writefds);
        FD_SET(sockfd, &writefds);
        int rv = select(sockfd + 1, NULL, &writefds, NULL, &tv);

The select() blocks waiting for one of the file descriptors to become available so that a using it will not block when it is used.

But you are blocking on the select() so you are not gaining any advantage to using select here. The point of select() is when you have lots of sockets with lots of data being read/written. Then select() allows you to continuously read from sockets that are ready and not block waiting on sockets that have nothing yet.

Remove select() from this context.

Printing error messages

This code is way too low level to be printing error messages.

        if(rv == -1)
            cerr << errno << "  " << strerror(errno) << endl;

Error messages should be sent to the user by a piece of code that understands the context under which the code is being used. If this code is being used as a server nobody will ever see this message. If this code is being used as a windows application nobody will ever see this message.

Throw an exception if this is unrecoverable (with the message). Some higher level piece of code will get the message and put it in the appropriate place (log file std::cerr etc).

Not all write errors are unrecoverable

            if(sentBytes == -1) {
                cerr << "Error sending IDs: " << errno << "  " << strerror(errno) << endl;
                return 1;
            }

There are a couple of error messages from ::write that are not unrecoverable.

https://linux.die.net/man/2/write

EAGAIN or EWOULDBLOCK or EINTR (EINTR being relatively common).

A whole bunch of those error codes should never happen (assuming your socket code works). There are error messages that should never happen here (caused by bugs in the code (bad file descriptor)) and there are error messages that are transient (network cable was unplugged). You should distinguish between the two types in your errors.

Success failure as 0/1?

C++ has a bool type. If you want to pass success/failure state use a bool rather than a number (unless that number has other meaning). And don't let an error code escape a public interface.

Read All

This implies a protocol so you know where the end is.

string clientSock::readAll() {
    string full = read();

    while(full.find("END") == string::npos)
        full += read();

    full = full.substr(0, full.find("END"));

    return full;
}

Which this code seems to do. But this is supposed to be low level socket code. It should not be handling protocols; this code should be in a derived class for the specific protocol that it understands.

Answer 2

Looks nice (didn't test it, yet). Just a tiny remark... as with other classes pertaining to files, handles, port addresses or instance-external things in general, you should disable the copy constructor like this:

clientSock(const clientSock&) = delete;

A copy of clientSock would refer to the same hostent * and sockfd handle and might appear to work at first... but as each instance uses its own buffer char recv[], this is bound to produce strange and hard-to-find errors. (And imagine what a non-empty destructor call could do to instance copies.)

If you disable the copy ctor, you will in this cases get a nice compile-time error (e.g. when you write a function accepting a socket as parameter, and write by accidentf(clientSock S) - without reference operator &).