Simple TCP Listener class

Question

This is my first winsock TCP listener ever written. I can use it for a small framework.

tcp_listener.h:

#pragma once
#include <WinSock2.h>
#include <thread>
class tcp_listener
{
public:
 tcp_listener(std::function< void(SOCKET) > callback);
 void start(unsigned short port);
private:
 SOCKET server;
 std::thread accept_thread;
 std::function< void(SOCKET) > callback;
};

tcp_listener.cpp:

#pragma comment(lib, "Ws2_32.lib")
#include "tcp_listener.h"
#include <iostream>
tcp_listener::tcp_listener(std::function< void(SOCKET) > callback)
{
 this->callback = callback;
 WSADATA wsa;
 if (WSAStartup(MAKEWORD(2, 2), &wsa) == SOCKET_ERROR)
 {
 std::cerr << "Cannot startup WSA: " << WSAGetLastError() << std::endl;
 }
 else
 {
 server = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
 }
}
void tcp_listener::start(unsigned short port)
{
 if (server != SOCKET_ERROR)
 {
 SOCKADDR_IN sock_addr;
 sock_addr.sin_port = htons(port);
 sock_addr.sin_family = AF_INET;
 sock_addr.sin_addr.s_addr = htonl(INADDR_ANY);
 if (bind(server, (SOCKADDR*)&sock_addr, sizeof(sock_addr)) == SOCKET_ERROR)
 {
 std::cerr << "Cannot bind socket on port " << port << std::endl;
 return;
 }
 if (listen(server, 10) == SOCKET_ERROR)
 {
 std::cerr << "Cannot listen to socket." << std::endl;
 return;
 }
 accept_thread = std::thread([&]() {
 SOCKET client = SOCKET_ERROR;
 while (client == SOCKET_ERROR)
 {
 client = accept(server, NULL, NULL);
 if (client != SOCKET_ERROR)
 {
 callback(client);
 client = SOCKET_ERROR;
 }
 }
 });
 accept_thread.detach();
 }
 else
 {
 std::cerr << "Socket not set." << std::endl;
 }
}

How to use it:

#include <iostream>
#include "tcp_listener.h"
void on_accept(SOCKET client)
{
 std::cout << "blabla new connection" << std::endl;
}
int main()
{
 tcp_listener* listener = new tcp_listener(on_accept);
 listener->start(1232);
 getchar();
 return 1;
}

I still have to make a new class where the SOCKET is stored and the IP etc. like system.net.sockets.Socket and use that as callback.

Also I'd like to get a review on my naming convention.

Answer 1

Naming

I think you're naming convention is OK, it seems consistent which is the most important thing. Some of your names could be a bit more specific:

std::function< void(SOCKET) > callback

callback doesn't tell me what the callback is for. new_connection_callback might be more appropriate.

Shutdown

There's no obvious way to shutdown your server. You just allow the object to be destroyed and hope for the best. I'd usually add a shutdown to a class like this so that the listen thread can be shutdown cleanly. The accept thread itself has a loop while (client == SOCKET_ERROR) which is a condition that is always true. You may as well be saying while(1).

Startup

Since the class has no concept of state it doesn't know whether or not it's running. This means you can't check if it's running when startup is called, so you get a strange error 'can't bind to socket' if you try to start the server on two different ports. Usually you'd encounter this error if the port was already in use. However, since the server port is an instance variable you get it because the SOCKET itself is already in use.

Constructor errors

This is a critical error:

if (WSAStartup(MAKEWORD(2, 2), &wsa) == SOCKET_ERROR)

If you can't get a socket, your application is in trouble. Don't just write to the log, throw an exception so that the caller is aware that there has been a problem and they can stop processing.

Callbacks

It's often useful to pass an additional piece of data into a callback method (for example a pointer to an instance of a collection to add the new socket to). With that in mind, I'd consider adding an extra parameter (usually a void*) to the callback. You also might want to consider typedefing the callback to make it easier to update in one place (if you decide to in the future).

Something like:

typedef std::function<void(SOCKET)> accept_callback;

makes the function signatures more descriptive and concise:

tcp_listener(accept_callback callback);

• Joshua Bakker: Actually, my plan was to allow command line input and make it so when the command destroy is read from the input it will destroy the server (delete all necessary pointers, stop the sockets etc.) besides that, sockets should never be destroyed / restarted while running the server. I kind of don't understand what you mean with the 'Startup' part. Thanks for the tips anyways.
• forsvarir: @JoshuaBakker change the main to try and start twice: listener->start(4040);listener->start(4041);
• Joshua Bakker: Oh yeah I see... I haven't tried C++ sockets in a while actually and I have done lots of C# in the meantime. Does the SOCKET have a error message? It's so much easier in C# to do sockets so I'm still getting used to C++ sockets.
• Joshua Bakker: Also, you say something about void*, you mean something like cprogramming.com/tutorial/function-pointers.html?
• forsvarir: @JoshuaBakker I mean something like pastebin.com/meGkLnPs . However, it's worth noting that I learnt C++ before things like shared_ptr and dynamic_cast were standardised so some would say I've got a somewhat old fashioned approach. You can do a similar thing using typed function pointers which can be more intuitive depending on where you're coming from: stackoverflow.com/q/2402579/592182