TCP client library using Winsock WSAEventSelect in C++

Question

One way to write a client which uses a specific protocol, eg http like in this case, is to create a base class with the basic socket handling functionality with virtual functions which can be overridden with an inheriting derived class.

But it is more flexible I think to be able to get the socket handling class to call a function specified by the client. Hence, the tcpclient constructor takes three arguments which are calling client callback functions to handle the asynchronous connect, data received and exception events.

But the callback functions are straight functions. It would probably be more flexible to allow member functions to be called somehow. I wasn't sure how to do that.

Anyway, I would appreciate any comments on the code.

tcpclient.hpp :

#ifndef TCPCLIENT_HPP_
#define TCPCLIENT_HPP_

#include <thread>

typedef void(*connect_callback)();
typedef void(*receive_callback)(const unsigned char* data, unsigned length);
typedef void(*exception_callback)(const int errorcode, const char* error_message);

enum SOCKET_ERROR_CODE { ERR_FATAL, ERR_INFORMATIONAL };

class tcpclient {
public:
    tcpclient(connect_callback connectcb, receive_callback receivecb, exception_callback exceptioncb, const int buffersize = 4096);
    ~tcpclient();

    bool connect(const char* host, unsigned short port);
    unsigned send(const unsigned char* data, unsigned length);
    unsigned send(const char* data);
    int close();
    bool is_connected() const;

    tcpclient(const tcpclient&) = delete;
    tcpclient& operator=(const tcpclient&) = delete;

private:
    int closesocket();
    int shutdown();
    connect_callback connectfunc_;
    receive_callback receivefunc_;
    exception_callback exceptionfunc_;
    std::thread reader_thread;
    void start_listening();
    bool shutting_down_;
    bool connected_;
    const int buffersize_;
};

#endif // TCPCLIENT_HPP_

tcpclient.cpp :

#include "tcpclient.hpp"

#ifdef WIN32

#ifndef WIN32_LEAN_AND_MEAN
#define WIN32_LEAN_AND_MEAN  // required for winsock2.h
#endif

#include <winsock2.h>
#include <ws2tcpip.h>  // getaddrinfo
#pragma comment(lib,"ws2_32.lib")  //winsock2 lib

#include <string>

// below vars static in cpp file so not to header pollute
// header. Could have used pimpl idiom - but easier way
static SOCKET socket_ = INVALID_SOCKET;
static WSAEVENT        comms_event = 0;

tcpclient::tcpclient(connect_callback connectcb, receive_callback receivecb, exception_callback exceptioncb, const int buffersize)
    : connectfunc_(connectcb), receivefunc_(receivecb), exceptionfunc_(exceptioncb), 
      buffersize_(buffersize), shutting_down_(false), connected_(false) {

    WSADATA w = { 0 };
    int error = WSAStartup(0x0202, &w);

    if (error || w.wVersion != 0x0202)
    { // there was an error
        throw "Could not initialise Winsock2";
    }

    socket_ = ::socket(AF_INET, SOCK_STREAM, IPPROTO_TCP); // Create socket

    // launch listener thread to handle any received data
    reader_thread = std::thread(&tcpclient::start_listening, this);

    // yield main thread to give reader_thread time to get going
    std::this_thread::yield();
}

tcpclient::~tcpclient() {
    shutting_down_ = true;

    if (reader_thread.joinable())
        reader_thread.join();  // prevents crash - due to terminate being called on running thread still 'alive'

    if (socket_ != INVALID_SOCKET) {
        shutdown();
        closesocket();
    }

    WSACleanup();
}

bool tcpclient::connect(const char* host, unsigned short port) {

    if (connected_) {
        if (exceptionfunc_) {
            exceptionfunc_(ERR_INFORMATIONAL, "attempt to connect failed - socket client already connected");
        }
        return false;  // already connected, client must disconnect first
    }

    struct addrinfo* result = NULL;
    struct addrinfo hints;

    struct sockaddr_in target = { 0 };
    memset(&hints, 0, sizeof(hints));
    hints.ai_family = AF_UNSPEC;
    hints.ai_socktype = SOCK_STREAM;
    hints.ai_protocol = IPPROTO_TCP;

    // inet_pton() returns 1 on success. It returns -1 if there was an error (errno is set), or 0 if the input isn't a valid IP address.
    int ret = inet_pton(AF_INET, host, &(target.sin_addr));
    if (ret != 1) {
        // ok so we assume not a proper ip address, so try getaddrinfo - might be a hostname/domain name
        ret = getaddrinfo(host, NULL, &hints, &result);
        if (ret != 0) {
            if (exceptionfunc_) {
                std::string s = "getaddrinfo failed with error: " + std::to_string(ret) + '\n';
                exceptionfunc_(ERR_INFORMATIONAL, s.c_str());
            }
            return false;
        }
        memcpy(&target, result->ai_addr, sizeof(sockaddr_in));
    }
    target.sin_family = AF_INET;  // IPv4
    target.sin_port = htons(port);

    ret = ::connect(socket_, reinterpret_cast<sockaddr *>(&target), sizeof(sockaddr));

    if (ret != 0) {
        ret = WSAGetLastError();
        if (ret == WSAEWOULDBLOCK) {
            // normal asynchronous connection
            ret = 0; 
        }
        else {
            char* s = NULL;
            FormatMessageA(FORMAT_MESSAGE_ALLOCATE_BUFFER | FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_IGNORE_INSERTS,
                NULL, ret,
                MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT),
                s, 0, NULL);

            if (exceptionfunc_) {
                exceptionfunc_(ERR_INFORMATIONAL, s);
            }
        }
    }

    return ret == 0;
}

bool tcpclient::is_connected() const {
    return connected_;
}

unsigned tcpclient::send(const unsigned char* data, unsigned length) {
    return ::send(socket_, reinterpret_cast<const char*>(data), length, 0);
}

unsigned tcpclient::send(const char* data) {
    return ::send(socket_, data, strlen(data), 0);
}

int tcpclient::close() {
    int ret = INVALID_SOCKET;
    if (socket_ != INVALID_SOCKET) {
        shutdown();
        ret = closesocket();
    }
    return ret;
}

int tcpclient::closesocket() {
    int ret = INVALID_SOCKET;
    if (socket_ != INVALID_SOCKET) {
        ret = ::closesocket(socket_);
        socket_ = INVALID_SOCKET;
    }
    return ret;
}

int tcpclient::shutdown() {
    int ret = INVALID_SOCKET;
    if (socket_ != INVALID_SOCKET) {
        // SD_SEND says to server, we have no more data to send
        // server may respond with response data
        ret = ::shutdown(socket_, SD_SEND);
    }
    return ret;
}

void tcpclient::start_listening() {

    // Create an event object to be used with this socket
    comms_event = WSACreateEvent();
    if (comms_event == WSA_INVALID_EVENT)
    {
        if (exceptionfunc_) {
            exceptionfunc_(ERR_FATAL, "Error creating winsock WSACreateEvent object");
        }
    }

    // setup winsock event handling 
    //The return value is zero if the application's specification of the network events and the associated event object was successful. 
    //Otherwise, the value SOCKET_ERROR is returned, and a specific error number can be retrieved by calling WSAGetLastError.
    int ret = WSAEventSelect(socket_, comms_event, FD_READ | FD_CONNECT | FD_CLOSE);
    if (ret != 0) {
        int lasterror = WSAGetLastError();
        std::string s = "Winsock communication error - unable to create event object winsock error " + std::to_string(lasterror);
        if (exceptionfunc_) {
            exceptionfunc_(ERR_FATAL, s.c_str());
        }

        return;
    }

    // Handle async network events
    WSANETWORKEVENTS events;
    char* buffer = new char[buffersize_]();

    while (!shutting_down_) {
        // Wait for a socket event
        DWORD dwRet;
        dwRet = WSAWaitForMultipleEvents(1, &comms_event, FALSE, WSA_INFINITE, FALSE);
        if (dwRet == WSA_WAIT_TIMEOUT)
        {
            // will never get to here as using WSA_INFINITE
            break;
        }

        // Type of event that occurred
        int nRet = WSAEnumNetworkEvents(socket_, comms_event, &events);
        if (nRet == SOCKET_ERROR)
        {
            if (exceptionfunc_) {
                exceptionfunc_(ERR_FATAL, "Winsock WaitForCommsEvent socket error");
            }

            break;
        }

        // Handle events
        // Connect event
        if (events.lNetworkEvents & FD_CONNECT)
        {
            if (events.iErrorCode[FD_CONNECT_BIT] != 0)
            {
                if (exceptionfunc_) {
                    exceptionfunc_(ERR_FATAL, "Winsock events error code for FD_CONNECT_BIT");
                }
                break;
            }

            connected_ = true;
            if (connectfunc_) {
                connectfunc_();
            }
        }

        // Read event
        if (events.lNetworkEvents & FD_READ)
        {
            // No need to keep reading, will get a new event for any data not read
            int bytes = recv(socket_, buffer, buffersize_, 0);
            if (bytes == SOCKET_ERROR)
            {
                if (exceptionfunc_) {
                    exceptionfunc_(ERR_FATAL, "Winsock socket error reading received data");
                }
                break;
            }
            if (bytes > 0)
            {
                if (receivefunc_) {
                    receivefunc_(reinterpret_cast<const unsigned char*>(buffer), bytes);
                }
            }
        }

        // Close event
        if (events.lNetworkEvents & FD_CLOSE)
        {
            connected_ = false;
            if (exceptionfunc_) {
                exceptionfunc_(ERR_INFORMATIONAL, "Socket closed");
            }

            shutdown();

            closesocket();

            break;
        }
    }

    delete [] buffer;
}


#elif
#error Only Windows platform supported
#endif

exercising program code :

#include "tcpclient.hpp"

#include <stdio.h>
#include <string.h>

#include <chrono>
#include <iostream>


static void printhex(const unsigned char* data, const unsigned len)
{
    for (size_t i = 0; i < len; ++i) {
        printf("%02x ", data[i]);
        if ((i + 1) % 16 == 0)
            putchar('\n');
    }
    putchar('\n');
}

class http_client
{
public:
    http_client(const char* host, const unsigned short port) : host_(host), port_(port), client(NULL) {
        http_ptr = this;
        start();
    }
    ~http_client() { delete client; }

    static http_client* get() { return http_ptr; }

    void do_connect() {
        printf("do_connect called\n");
        std::string url = "GET / HTTP/1.0\r\nHost: " + std::string(host_) + "\r\nConnection: close\r\nUser-Agent: test socket program/1.0\r\nAccept-Encoding: gzip/\r\nAccept-Charset: ISO-8859-1,UTF-8;q=0.7,*;q=0.7\r\nCache-Control: no - cache/\r\nAccept-Language: de, en; q = 0.7, en - us; q = 0.3/\r\n\r\n";
        client->send(url.c_str());
    }

    void do_data(const unsigned char* data, unsigned length) {
        printhex(data, length);
    }

    static void connect_handler() {
        printf("connect_handler called\n");
        http_client* cl = get();
        cl->do_connect();
    }

    static void data_handler(const unsigned char* data, unsigned length) {
        http_client* cl = get();
        cl->do_data(data, length);
    }
    static void exception_handler(const int errorcode, const char* error_message) {
        printf("Error %d, %s\n", errorcode, error_message);
    }

private:
    const char* host_;
    const unsigned short port_;
    tcpclient* client;

    static http_client* http_ptr;

    http_client(const http_client&) = delete;
    http_client& operator=(const http_client&) = delete;
    void start() {
        client = new tcpclient(connect_handler, data_handler, exception_handler);
        client->connect(host_, port_);
    }
};

http_client* http_client::http_ptr = NULL;

int main(int argc, char* argv[]) {

    if (argc != 2) {
        printf("Usage: testprog.exe <host>\n");
        exit(0);
    }

    http_client client(argv[1], 80);

    // stop program finishing before we have downloaded all web content
    for (int i = 0; i < 5; ++i) {
        putchar('.');
        std::this_thread::sleep_for(std::chrono::seconds(1));
    }

}

Late additional comment

After reading this article:

http://www.cprogramming.com/c++11/c++11-lambda-closures.html

I found that I can change the tcpclient constructor to look like this:

tcpclient(std::function<void()> connectcb, std::function<void(const unsigned char*, unsigned)> receivecb, std::function<void(const int, const char*)> exceptioncb, const int buffersize = 4096);

And use these members:

std::function<void()> connectfunc_;
std::function<void(const unsigned char*, unsigned)> receivefunc_;
std::function<void(const int, const char*)> exceptionfunc_;

Then I can construct like this in start function in main :

client = new tcpclient([this]() { do_connect(); }, [this](const unsigned char* data, unsigned length) { do_data(data, length); }, [this](const int errorcode, const char* error_message) { exception_handler(errorcode, error_message); });

Is that the standard way to do function pointers to member functions in C++? Any comment on that would be interesting. I guess it could look a bit daunting to someone not familiar to std::function.

I won't change the code in the review at this late stage.

Answer 1

start_listening

There are a couple of issues with the naming of this method. Listening has certain expectations in TCP. Namely that you're listening for incoming connections. This isn't what your 'listening' for in this method. 'start' suggests that the method is going to 'start' something, where as it actually seems to contain the entire thread function for reading events from the socket. Both of these seem misleading, so I'd reconsider the naming.

Thread life cycle

You start your listening thread when the tcpclient is created and then cleanup after it in the destructor, however it is possible for the thread to terminate earlier than object destruction. Just looking at the public interface, I would assume that I could reuse the same instance of tcpclient to connect to different endpoints, send/receive information, close the socket then do it again for a different endpoint. However, once the socket is closed the first time, the thread method will end and there is noway to restart it without creating a new instance of tcpclient.

static to hide implementation

You're declaring comms_event and socket_ as static in the implementation file. The main problem I have with this is that it is effectively turning your tcpclient into a singleton, but without being explicit about it. It would be very easy to try to create to instances of the tcpclient and connect them to two endpoints, without realising that they were sharing the same underlying socket handle. There are better ways to hide the underlying implementation.

callbacks

I'm not familiar with the std::function you've mentioned at the end of your post, however there are two alternate patterns that I've used with callbacks. The first is to pass an extra parameter (usually a (void*)) to calls to callback functions, which is registered along with the callback. This allows the callback to interpret the received information, based on the the supplied value. The parameter value could be a high level connection identifier, or some classes this pointer, the tcpclient doesn't need to know it just saves and passes on the value.

The alternative I've used is to register an event receiver interface, rather than callback methods. So you would define an abstract base class:

class tcpclient_event_receiver {
public:
    virtual void connected() = 0;
    virtual void received(const unsigned char* data, unsigned length) = 0;
    virtual void exception(const int errorcode, const char* error_message) = 0;
};

And tcpclient's constructor is adapted as so:

tcpclient(shared_ptr<tcpclient_event_receiver> eventreceiver, const int buffersize = 4096);

repeated code

You do this a lot:

if (exceptionfunc_) {
    exceptionfunc_(ERR_INFORMATIONAL, s);
}

I'd be tempted to wrap it in a method, so that the exceptionfunc_ check isn't missed