Windows socket class

Question

I need some advice to see if my simple Windows socket class is good enough to be used in a simple chat application.

#include <iostream>
#include <string>
#include <Windows.h>

class Socket 
{
private:
    WSADATA         wsaData;
    SOCKET          hSocket;
    sockaddr_in     service;
    std::string     addr;
    USHORT          port;
    int             exitCode;

    bool initializeWSA();
    bool initializeSocket();
    bool bind();
    bool listen();
    bool connect();

    /// Modified copy constructor for *accepted* connection sockets
    Socket(Socket& socket, SOCKET hSockect) :   hSocket( hSockect ), 
                                            wsaData( socket.wsaData ),
                                            service( socket.service ),
                                            addr( socket.addr ),
                                            port( socket.port ){}
public:
    bool close();
    Socket* acceptConnection();

    /// Default Constructor
    Socket()
    {}

    /// Main Constructor
    Socket(std::string addr, USHORT port, bool actAsServer = false) : addr( addr ), port( port )
    {
        initializeWSA();
        initializeSocket();

        service.sin_family = AF_INET;
        service.sin_addr.s_addr = inet_addr(addr.c_str());
        service.sin_port = htons(port);

        if( actAsServer )
        {
            bind();
            listen();
        }
        else
            connect();
    }

    template<class T>
    int recvData(T& i) {

        try
        {
            int ret = ::recv( hSocket, reinterpret_cast<char*>(&i), 32, 0 );
            if( ret == SOCKET_ERROR )

             throw   WSAGetLastError();

            return ret;
        }
        catch(std::exception& e)
        {
            std::cerr << "Socket//error while receiving; Error#" << WSAGetLastError() << "." << std::endl;
        }
    }
    /// recv specialization for std::string
    int recvData<std::string>(std::string& i)
    {
        try
        {
            int cread;
            // receive string size
            long length = 0;
            cread = ::recv( hSocket, reinterpret_cast<char*>(&length), sizeof( length ), 0 );
            if( cread == SOCKET_ERROR )
            {
                throw WSAGetLastError();
                return cread;
            }
            length = ntohl( length );
            // receive string
            while( 0 < length ) {
                char buffer[1024];
                int cread;
                cread = ::recv( hSocket, buffer, min( sizeof( buffer ), length ), 0 );
                if( cread == SOCKET_ERROR )
                {
                    throw WSAGetLastError();
                    return cread;
                }
                i.append( buffer, cread );
                length -= cread;
            }
            return i.length();
        }
        catch(std::exception& e)
        {
            std::cerr << "Socket//error while receiving; Error#" << WSAGetLastError() << "." << std::endl;
        }
    }


    template<class T>
    int sendData(T& i) {
        try
        {
        int ret = ::send( _hSocket, reinterpret_cast<const char*>(&i), sizeof(i), 0 );
        if( ret == SOCKET_ERROR )

            throw WSAGetLastError();
        return ret;
        }
        catch(std::exception& e)
        {
            std::cerr << "Socket//error while receiving; Error#" << WSAGetLastError() << "." << std::endl;
        }
    }
    /// send specialization for std::string
    int sendData<std::string>(std::string& i)
    {
        try
        {
        int ret;
        // send length of string
        long length = htonl( i.length() );
        ret = ::send( hSocket, reinterpret_cast<const char*>(&length), sizeof(length), 0 );
        if( ret == SOCKET_ERROR )
        {
            throw WSAGetLastError();

            return ret;
        }
        // send string
        ret = ::send( hSocket, i.data(), i.length(), 0 );
        if( ret == SOCKET_ERROR )
        {
            throw WSAGetLastError();

            return ret;
        }
        return ret;
        }
        catch(std::exception& e)
        {
            std::cerr << "Socket//error while receiving; Error#" << WSAGetLastError() << "." << std::endl;
        }
    }
    template<>
    int sendData<char*>(char*& i)
    {
        std::string data;
        data.append( i );
        sendData( data );
    }

    /// Default Deconstructor, cleans up socket handle
    ~Socket() { close(); }
};

Answer 1

Pretty sure this is not portable.

    long length = htonl( i.length() );
    ret = ::send( hSocket, reinterpret_cast<const char*>(&length), sizeof(length), 0 );

You are obviously trying to make it portable (good). But htonl() does not return a long which is platform/compiler dependent. What you want really want is uint32_t (man htonl).

Anything that has Create/Destroy cycle should be wrapped in an RAII object. That way if you create it then something else fails you don't need to remember to destroy it. That is done automatically.

    initializeWSA();
    initializeSocket();

If initializeWSA() which I presume calls the windows socket initialization code also has a counterpart to shut it down. So if initializeSocket() fails the WSAData will be correctly re-claimed.

I think you are trying to cram too much functionality into a single class.

    if( actAsServer )
    {
        bind();
        listen();
    }
    else
        connect();

Why not have a class that is explicitly designed for server end and one designed for the client end. Then can share a lot of common code in a base class. But you don't need to mix them.

This may return data but it may not be the whole message. Or it may be the combination of several messages.

        long length = 0;
        cread = ::recv( hSocket, reinterpret_cast<char*>(&length), sizeof( length ), 0 );

But there is a distinct possibility that it could potentially return 3 bytes (not the 4 you need). If so you need to continue reading until you get the 4 bytes you need (sorry sizeof(length).

So you should wrap your ::recv into a loop and make sure you read as much as you actually need before continuing (or specify MSG_WAITALL) in the configuration.

Also not all errors are fatal. EINTR just means an interrupt was seen (so if a timer goes off or something like that). If you don't care about the interrupt you can just start the loop again.

Conversely ::send() may not always send all the data.

    ret = ::send( hSocket, reinterpret_cast<const char*>(&length), sizeof(length), 0 );

You may want to keep track of how much is sent and add it up until all the data has been sent across the socket. Again not all error are fatal.

Do you really want the copy constructor to copy WSAData?

/// Modified copy constructor for *accepted* connection sockets
Socket(Socket& socket, SOCKET hSockect) :   hSocket( hSockect ), 
                                        wsaData( socket.wsaData ),
                                        service( socket.service ),
                                        addr( socket.addr ),
                                        port( socket.port ){}

Does the destructor not destroy something in wsaData (hard to tell and I am not a windows expert) in which case it may be done twice if you are not careful.

Two points here: 1) Indentation. Make it obvious. 2) I prefer to wrap conditionals in curly braces. {}. On older compilers they implemented throw with a macro and macros are prone to errors if they were not written correctly. My point here is that you can always tells if a what you write is a single statement or multiple statements (hidden in a macro function). If its multiple statments then this type of if will break. Prefer to use '{}' to make it explicit.

        if( ret == SOCKET_ERROR )

         throw   WSAGetLastError();

This try block only contains a C function ::recv so no exceptions are going to be emitted from that. So the only exception here is the one you throw (which does not match the catch).

    try
    {
        int ret = ::recv( hSocket, reinterpret_cast<char*>(&i), 32, 0 );
        if( ret == SOCKET_ERROR )

         throw   WSAGetLastError();

        return ret;
    }
    catch(std::exception& e)
    {
        std::cerr << "Socket//error while receiving; Error#" << WSAGetLastError() << "." << std::endl;
    }

Also exceptins are great for passing errors across interface boundries (ie out of public methods). But internally within your code using error codes is fine (becuase the library is self contained you can make sure you check the error codes from each method call).

see: https://softwareengineering.stackexchange.com/questions/118142/what-is-a-good-number-of-exceptions-to-implement-for-my-library/118187#118187

Answer 2

• I'm not excited about the structure of the code. I think you're putting more in one class than is really a good idea.

Just for example, I'd start with a a tiny class that does nothing but handle the WSAStartup/WSACleanup sequence:

class WSAUser { 
    WSADATA data;
public:
    WSAUser() { /* call WSAStartup() */ }
    ~WSAUser() { /* call WSACleanup() */ }
};

...then any other class that's going to use winsock just includes an instance of that object to ensure the WSAStartup and WSACleanup are called appropriately.

• Some of your comments are utterly useless, to the point that the code would be better off without them.

For one example:

/// Default Constructor
Socket()
{}

Comments should not just repeat what's already obvious from the code itself. They should explain things that aren't obvious. For example, if you're using an unusual or non-obvious algorithm, it's often best to explain what algorithm you're using, and possibly why (especially if there's some other algorithm that's likely to seem like a better choice, but for non-obvious reasons isn't).

• Some of your code looks broken

One obvious example:

template<class T>
int recvData(T& i) {

    try
    {
        int ret = ::recv( hSocket, reinterpret_cast<char*>(&i), 32, 0 );

This is attempting to receive and write 32 bytes of data into the destination object, regardless of that object's size. If that object is smaller than 32 bytes, it'll write past its end--a buffer overrun leading to undefined behavior (not to mention probably security problems).

If you change 32 to sizeof(i), this will at least work for primitive types, but will still produce bad results for most types that contain pointers. Given that it's a template, you're currently promising that it'll work for any possible type. Although it's somewhat more advanced, you might want to do some reading about things like std::enable_if and type-traits that will let you specify the types for which this code is intended to work (and prevent the code from compiling if a type that won't work is passed).

• Misuse of exception handling

At least in my opinion, you're making poor use of exception handling. For example:

try
{
    int ret = ::recv( hSocket, reinterpret_cast<char*>(&i), 32, 0 );
    if( ret == SOCKET_ERROR )
        throw   WSAGetLastError();
    return ret;
}
catch(std::exception& e)
{
   std::cerr << "Socket//error while receiving; Error#" << WSAGetLastError() << "." << std::endl;
}

First of all, WSAGetLastError() returns an int, so that's the type this will potentially throw. Since int isn't derived from std::exception, there's probably no way this exception handler can ever be invoked. Even if, however, we fix that problem, this would be a poor use of exception handling. If we really want to print out a message to cerr in case of a problem, that can be done a lot more directly:

    int ret = ::recv( hSocket, reinterpret_cast<char*>(&i), 32, 0 );
    if( ret == SOCKET_ERROR )
        std::cerr << "Socket//error while receiving; Error#" << WSAGetLastError() << "." << std::endl;

The real win from exception handling stems from the fact that this part of the code doesn't really know how to react to the problem appropriately. Printing to the standard error stream makes sense for some applications, but not at all for others (e.g., in a windowed application, there might not even be a standard error stream).

As such, it may well make sense for this code to throw an exception here--but not catch it or attempt to handle it at all:

int ret = ::recv( hSocket, reinterpret_cast<char*>(&i), 32, 0 );
if( ret == SOCKET_ERROR ) {
    std::stringstream buf;
    buf << "Socket//error while receiving; Error#" << WSAGetLastError();
    throw std::runtime_error(buf.str().c_str());
}

...then some code at the application level will catch the exception and display the result appropriately for the type of application in which this is being used (e.g., a server might log it, a client might display it in a message box).

Answer 3

WSAGetLastError() returns an int. Throwing an int is legal but not quite usual. Your catch(std::exception& e) statements are fairly useless, given that Win32 APIs are unlikely to throw a std::exception object.

Your default constructor is dangerous: you can't tell if a socket instance has been properly-constructed or default-constructed. You should probably not have a default constructor. You should also probably 'delete' the default copy constructor and assignment operator.

You didn't post source code for your bind/listen/accept/connect methods.

You may need two addresses: one for your address (passed to bind), and another for the remote address (passed to connect or returned from accept).