Windows socket class

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

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

Attribution
Source : Link , Question Author : NinjaDeveloper , Answer Author : Community

Leave a Comment