TCP socket base class and Winsock implementation

Question

I'm coding a chat application from scratch in modern C++. I'm currently building an interface for interacting with sockets. I want it to be cross platform. Am I approaching platform independence correctly? Also is my error handling acceptable? Please tell me what is wrong and what should I improve or change.

Socket.h


#include <string_view>
#include <cstdint>
#include <optional>
#include <memory>

bool InitNetworking();

class TcpSocket
{
public:
    TcpSocket() = default;
    virtual ~TcpSocket() = default;

    TcpSocket(const TcpSocket &) = delete;
    TcpSocket(TcpSocket &&) = delete;
    
    void operator=(const TcpSocket &) = delete;
    void operator=(TcpSocket &&) = delete;

    virtual void *GetHandle() = 0;
    virtual bool Connect(std::string_view ip_address, std::uint16_t port) = 0;
    virtual std::shared_ptr<TcpSocket> Accept(std::string_view ip_address, std::uint16_t port) = 0;
    virtual int Send(TcpSocket &sock, std::string_view message) = 0;
    virtual std::string Receive() = 0;

    static std::unique_ptr<TcpSocket> Create(std::string_view name, void *socket_handle = nullptr);
private:
};

Socket.cpp

#include "Socket.h"
#include "Win32TcpSocket.h"

#ifdef _WIN32

static WSADATA wsa;
bool InitNetworking()
{
    if (!WSAStartup(MAKEWORD(2, 2), &wsa))
        return true;
    return false;
}

std::unique_ptr<TcpSocket> TcpSocket::Create(std::string_view name, void *socket_handle)
{
    auto ptr = Win32TcpSocket::Create(name, (SOCKET)socket_handle);
    return ptr;
}

#endif

Win32TcpSocket.h


#include <string_view>
#include <cstdint>
#include <memory>

#include "Socket.h"
#include <WinSock2.h>
#include <WS2tcpip.h>

class Win32TcpSocket : public TcpSocket
{
public:
    Win32TcpSocket() = default;
    virtual ~Win32TcpSocket();


    virtual void *GetHandle() override;
    virtual bool Connect(std::string_view ip_address, std::uint16_t port) override;
    virtual std::shared_ptr<TcpSocket> Accept(std::string_view ip_address, std::uint16_t port) override;
    virtual int Send(TcpSocket &sock, std::string_view message) override;
    virtual std::string Receive() override;

    static std::unique_ptr<Win32TcpSocket> Create(std::string_view name, SOCKET socket_handle = 0);
private:
    SOCKET m_socket = INVALID_SOCKET;
    std::string_view m_name = "";
};

Win32TcpSocket.cpp

#include <iostream>
#include <exception>
#include <stdexcept>

std::unique_ptr<Win32TcpSocket> Win32TcpSocket::Create(std::string_view name, SOCKET socket_handle)
{
    auto ptr = std::make_unique<Win32TcpSocket>();
    if (!socket_handle)
    {
        ptr->m_socket = socket(AF_INET, SOCK_STREAM, 0);
        if (ptr->m_socket == INVALID_SOCKET)
            throw std::runtime_error("Invalid socket!");
    }
    else
    {
        ptr->m_socket = socket_handle;
    }
    
    ptr->m_name = name;
    return ptr;
}

void *Win32TcpSocket::GetHandle()
{
    return (void*)m_socket;
}

Win32TcpSocket::~Win32TcpSocket()
{
    if(m_socket != INVALID_SOCKET)
        closesocket(m_socket);

    m_socket = INVALID_SOCKET;
}

bool Win32TcpSocket::Connect(std::string_view ip_address, std::uint16_t port)
{
    sockaddr_in hint = {};
    hint.sin_family = AF_INET;
    hint.sin_port = htons(port);
    inet_pton(AF_INET, ip_address.data(), &hint.sin_addr);
    int connect_result = connect(m_socket, (sockaddr *)&hint, sizeof(hint));

    if (connect_result == SOCKET_ERROR)
    {
        return false;
    }
    return true;
}

std::shared_ptr<TcpSocket> Win32TcpSocket::Accept(std::string_view ip_address, std::uint16_t port)
{
    sockaddr_in server_address{};
    server_address.sin_family = AF_INET;
    inet_pton(AF_INET, ip_address.data(), &server_address.sin_addr);
    server_address.sin_port = htons(port);

    int err = bind(m_socket, (struct sockaddr *)&server_address, sizeof(server_address));

    if (err == SOCKET_ERROR)
    {
        return nullptr;
    }

    err = WSAGetLastError();

    if (listen(m_socket, SOMAXCONN) == SOCKET_ERROR)
    {
        return nullptr;
    }

    sockaddr_in client_address = {};
    int sz = sizeof(client_address);

    SOCKET client = accept(m_socket, (struct sockaddr *)&client_address, &sz);
    if (client == INVALID_SOCKET)
    {
        return nullptr;
    }
    
    sockaddr_in hints{};
    int hint_size = sizeof(hints);

    err = getpeername(client, (sockaddr *)&hints, &hint_size);
    if (err == INVALID_SOCKET)
    {
        return nullptr;
    }

    char buff[256]{};
    inet_ntop(AF_INET, &(hints.sin_addr), buff, 256);

    auto ptr = Win32TcpSocket::Create(buff, client);
    return ptr;
}

int Win32TcpSocket::Send(TcpSocket &sock, std::string_view message)
{
    SOCKET sockfd = (SOCKET)sock.GetHandle();

    if (sockfd != INVALID_SOCKET)
    {
        return send(sockfd, message.data(), (int)message.length(), 0);
    }
    
    return 0;
}

std::string Win32TcpSocket::Receive()
{
    char buff[256]{};
    int bytes = recv(m_socket, buff, 256, 0);
    return buff;
}

Thank you everyone for the suggestions. I know I'm reinventing the wheel, but as you said it's for learning purposes. I'll refactor some of the code and try to keep your advises in mind. — Cherestes Radu
– Cherestes Radu, Commented Mar 13, 2024 at 20:50

uli · Accepted Answer · 2024-03-12 15:13:03Z

Firstly, there is no right or wrong about different approaches. The difference is rather how smooth it will be working with this, how easy it is to get it to work and how hard it is to break it. Now, looking at your code, it could be improved, but it is clear to read and understand. No problem there, so let's look at the architecture...

Socket Baseclass Is Overloaded

The socket baseclass does two things: Input/Output and accepting new connections. These two things are fundamentally different and they are typically used in totally different parts of the program as well. Mixing them into the baseclass will give you headaches.

Access To The Handle

You define the type of the handle in the baseclass already (void*, the least specific type of all) and force each implementation to provide a getter. There are two things wrong with this:

Not every implementation uses void*. So, in order to use the handle, you need to cast it to some other type first. This is always error-prone.
You don't document what can or should be done with this handle. Am I now responsible for calling close() on the handle? Under MS Windows, it also needs to be closesocket(), I believe, which doesn't make it any easier to use.

In summary, don't put this into the baseclass. Instead, only provide this handle in the derived class and use the correct type. Further, distinguish between just lending a copy (no ownership) and transfer of ownership. You can support both, which is what you already do in the constructor taking a socket handle.

Using Polymorphism

You have one base class (actually, just an interface) and one derived class for each OS. Why would you do that at all, when you always only have one of them "active"? Under MS Windows, you only use the Win32TcpSocket, under Solaris, you would perhaps only use the SolarisTcpSocket, so it's always the same anyway. Use one class declaration (header) and different implementations (.cpp files) instead, that would be much clearer. Also, you could get rid of any virtual, which reduces the footprint a tiny bit.

Documentation

Generally, concerning your use of polymorphism here, the base class is clearly an interface, which is good. However, there is no documentation what each function does, what is required of the parameters and what guarantees the methods offer. I could guess and would probably be close, but not spot on. For example, the Send() method, which @reinderien already mentioned, is very counter-intuitive. This also affects correct use of this class. Lastly and most importantly, writing things down forces you to think about it and possibly find weird or counterintuitive stuff yourself.

OS-Specific Split On A Higher Level

Your socket class is still very low level. Also, if you want to extend this, you may find yourself looking at e.g. select() (common), WaitForMultipleObjects() (Windows), epoll (Linux) or other OS-specific things that are similar but have very different low-level APIs. Also, consider e.g. a blocking and a non-blocking call to accept().

For that reason, I'd make a split on a higher level. For example, I'd create one class that manages establishing connections and only that. Internally, it could well use a thread that uses a blocking accept() or perhaps some OS-specific API. It would actively emit the new connections using a callback function. Then, I would use a different class for something that only does IO. Again, this could be using buffers and a thread internally for ease of use. Neither of the two would expose raw handles.

Miscellaneous Stuff

Why is the MS Windows-specific code in socket.cpp? It should be in the OS-specific file. The TcpSocket base class shouldn't need to know about the Win32TcpSocket derived class.
If you return true or false, don't use an if clause to switch between them. Instead, just return the condition you would use in the if clause anyway. Example return connect_result != SOCKET_ERROR;.
The name "win32 API" is obsolete, it's called "Windows API" today, so I'd drop that from the name of the class.
Don't use a shared_ptr if you actually transfer full ownership. Also, make your type moveable, then you wouldn't need to clarify that anyway. Since you then can't return nullptr, you'll have to signal errors with exceptions. There, you can also provide context like e.g. the failed call and what the errno/GetLastError()/WSAGetLastError() value was.
IPv6 support? DNS names support? Just a suggestion.
I assume you know or at least suspect that you're reinventing the wheel, which is fine for learning. However, one skill to learn is also to evaluate and integrate other libraries. I would suggest ZeroMQ and Boost, both have goodies for TCP networking.

Reinderien · Accepted Answer · 2024-03-12 12:41:49Z

It doesn't seem like a great idea to force inheritors of TcpSocket to return a void *GetHandle, for two reasons - it breaks encapsulation, and doesn't actually conform to typical handle types. For details read Socket Data Type. It also forces some of your later code to perform acrobatics, casting out of void* back to HANDLE. If you really, really need to expose the raw handle, templates can do this in a type-safe manner (HANDLE for winsock, int for Unix-like sockets).

If you needed to keep (SOCKET)sock.GetHandle() (which you shouldn't), don't use C-style casts; use the appropriate C++ templated cast instead.

You're using a mix of error signalling schemes - exceptions, boolean return, and mixed zero-or-size. Choose one signalling scheme for consistency. If this were to be a successful abstraction, I would expect Send to return a size_t rather than an int since it's describing a byte count.

In Receive, use sizeof(buff) rather than rewriting 256.

Good use of string_view.

Send is bizarre - it's a member method, but also accepts a reference to its own class type. Why? Drop the sock parameter and use m_socket directly.

If you were to lose exceptions, then your pattern for Create would make a lot of sense - a static pseudoconstructor that can either return a new instance pointer on success or a nullptr on failure. However, you do have exceptions, so this pattern is not necessary. Just write a normal constructor and throw from it on failure. This will allow you to make both m_socket and m_name immutable const, assigned in initializer lists.

Replace the string in std::runtime_error("Invalid socket!"); with WSAGetLastError() / FormatMessage() . A custom exception class capturing both the error number and string would help.

SOCKET and int are not type-safe. They're not pointers though, so they provide runtime safety against incorrect (possibly NULL) pointers being dereferenced. — G. Sliepen
– G. Sliepen, Commented Mar 12, 2024 at 14:40

Stack Exchange Network

TCP socket base class and Winsock implementation

2 Answers 2

Socket Baseclass Is Overloaded

Access To The Handle

Using Polymorphism

Documentation

OS-Specific Split On A Higher Level

Miscellaneous Stuff

You must log in to answer this question.

Hot Network Questions

TCP socket base class and Winsock implementation

2 Answers 2

Socket Baseclass Is Overloaded

Access To The Handle

Using Polymorphism

Documentation

OS-Specific Split On A Higher Level

Miscellaneous Stuff

You must log in to answer this question.

Related

Hot Network Questions