First socket class

Question

I want my socket class to be reviewed, including recommendations and optimizations.

Sokect.hpp

    class Socket {
    protected:


        Socket(SOCKET s);
        Socket();
        SOCKET s_;
        sockaddr_in sa_;
        int* count_;

    private:
        static void StartSocket();
        static void EndSocket();
        static int  totalNumberSockets_;



    public:

        virtual ~Socket();
        Socket(const Socket&);


        std::string RecvData();
        void SendData(std::string);

        void Close();

    };

using namespace std;




//start sockect 
void Socket::StartSocket() {
    if (!totalNumberSockets_) {
        WSADATA info;
        if (WSAStartup(MAKEWORD(2, 0), &info)) {
            throw "Could not start WSA";
        }
    }
    ++totalNumberSockets_;
}


//end end the socket
void Socket::EndSocket() {
    WSACleanup();
}

Socket::Socket() : s_(0) {
    StartSocket();
    s_ = socket(AF_INET, SOCK_STREAM, 0);

    if (s_ == INVALID_SOCKET) {
        cerr << "Error at Socket(): " << WSAGetLastError() << endl;
        throw "INVALID_SOCKET";
    }

    count_ = new int(1);
}

Socket::Socket(SOCKET s) : s_(s) {
    StartSocket();
    count_ = new int(1);
};

Socket::~Socket() {
    if (!--(*count_)) {
        Close();
        delete count_;
    }
    --totalNumberSockets_;
    if (!totalNumberSockets_)
        EndSocket();
}

Socket::Socket(const Socket& o) {
    count_ = o.count_;
    (*count_)++;
    s_ = o.s_;


    totalNumberSockets_++;
}



void Socket::Close() {
    closesocket(s_);
}



std::string Socket::RecvData() {
    std::string strBuffer;
    do{
        char buffer;
        int recvInt = recv(s_, &buffer, 1, 0);
        if (recvInt == INVALID_SOCKET)
        {
            return "";
        }
        if (recvInt == SOCKET_ERROR)
        {
            if (errno == EAGAIN) {
                return strBuffer;
            }
            else {
                // not connected anymore
                return "";
            }
        }

        strBuffer += buffer;
        if (buffer == '\n')  return strBuffer;
    } while (true);
}
void Socket::SendData(std::string s) {
    s += "\n";
    send(s_, s.c_str(), s.length(), 0);
}

Answer 1

I think you have too many different responsibilities bound up into one class. That ends up not only making that class more complex, but adding extra complexity overall as well. As a starting point, I'd have a really trivial class that does nothing but stack-based management of the WSAStartup/WSACleanup:

struct socket_user {
    socket_user() {
        WSADATA info;
        if (WSAStartup(MAKEWORD(2, 0), &info)) 
            throw "Could not start WSA";       
    }
    ~socket_user() { 
        WSACleanup();
    }
};

Then you can just embed an instance of that into your socket class:

class Socket {
    socket_user s;
    // ...
};

Windows keeps a counter internally, so you're allowed to call WSAStartup repeatedly with matching calls to WSACleanup. No need for extra work on your own part to do that counting over again. So, that trivial class replaces about half the code in the question.

That leaves (mostly) your RecvData and SendData. I think RecvData is open to some improvement as well. Right now, it makes a separate call to recv for each byte of data. That's likely to add a fair amount of overhead. I'd consider passing a somewhat larger buffer to recv so you can receive more data at a time, and spend proportionally less time just jump to/returning from recv. Even increasing the buffer size to 16 bytes is likely to reduce overhead substantially.

I don't really like how you're handling errors either. In particular, if you get a SOCKET_ERROR, you throw away data you've already received and read. If you've already received some data, I'd prefer to see that returned, and let the application decide whether it's really worth keeping or not.

   if (recvInt == INVALID_SOCKET)
    {
        return strBuffer;
    }

That brings us to another point: I'd rather see more descriptive names than recvInt and strBuffer. I'd consider something like socket_error and just buffer respectively (odd how removing part of the latter name actually makes the result more meaningful, but there it is).

I'd also like to see the code put into relevant name spaces. For example, those parts that relate to IP in general (e.g., IP addresses) could be an an IP namespace. Those parts specific to TCP or UDP would go in that namespace (which would itself be inside the IP namespace, since UDP and TCP are both IP protocols).

Code might look something like this:

#ifndef SOCK2_INCLUDED_
#define SOCK2_INCLUDED_

#include <string>
#include <iostream>
#include <winsock2.h>
#include <algorithm>

#pragma comment(lib, "ws2_32.lib")

namespace IP {
struct socket_user { 
    WSADATA data;
    socket_user() {
        WSAStartup(MAKEWORD(2, 2), &data);
    }
    ~socket_user() { 
        WSACleanup();
    }
};

class address {
    socket_user u;
    struct sockaddr_in dest;
    struct in_addr addr;

    hostent *lookup(std::string const &hostname) { 
        hostent *host;

        if (isdigit(hostname[0])) {
            addr.s_addr = inet_addr(hostname.c_str());
            host = gethostbyaddr((char const *)&addr, sizeof(addr), AF_INET);
        }
        else 
            host = gethostbyname(hostname.c_str());
        return host;
    }

public:

    address(std::string const &hostname, short port=80) {
        hostent *host = lookup(hostname);

        dest.sin_family = AF_INET;
        dest.sin_port = htons(port);

        addr.S_un.S_un_b.s_b1 = host->h_addr_list[0][0];
        addr.S_un.S_un_b.s_b2 = host->h_addr_list[0][1];
        addr.S_un.S_un_b.s_b3 = host->h_addr_list[0][2];
        addr.S_un.S_un_b.s_b4 = host->h_addr_list[0][3];
        dest.sin_addr = addr;
    }

    address(unsigned char b0, unsigned char b1, unsigned char b2, unsigned char b3, short port)
    {
        addr.S_un.S_un_b.s_b1 = b0;
        addr.S_un.S_un_b.s_b2 = b1;
        addr.S_un.S_un_b.s_b3 = b2;
        addr.S_un.S_un_b.s_b4 = b3;
        dest.sin_family = AF_INET;
        dest.sin_port = htons(port);
        dest.sin_addr = addr;
    }

    operator SOCKADDR *() const { return (SOCKADDR *)&dest; }

    size_t size() const { return sizeof(dest); }

    friend std::ostream &operator<<(std::ostream &os, address const &a) {
        return os << (short)a.addr.S_un.S_un_b.s_b1
            << "." << (short)a.addr.S_un.S_un_b.s_b2
            << "." << (short)a.addr.S_un.S_un_b.s_b3
            << "." << (short)a.addr.S_un.S_un_b.s_b4;
    }
};

namespace UDP { 
class socket { 
    SOCKET s;
    socket_user u;
public:
    socket() : s(::socket(AF_INET, SOCK_DGRAM, IPPROTO_UDP)) {}

    int setopt(int option, char const *val=NULL, int len = 0) {
        return setsockopt(s, SOL_SOCKET, option, val, len);
    }

    template <class T>
    int setopt(int option, T val) {
        return setsockopt(s, SOL_SOCKET, option, (char *) &val, sizeof(val));
    }

    template <class T>
    int send(T const &t, address const &a) { 
        return sendto(s, (char *)&t, sizeof(t), 0, a, a.size());
    }

    template <class T>
    void read(address const &a, T &buffer) {
        connect(s, a, a.size());
        recv(s, (char *)&buffer, sizeof(buffer), 0);
    }

    ~socket() { closesocket(s); }
};
}

namespace TCP {
class socket { 
    socket_user u;
    SOCKET s;
    address a;
    bool connected;

    void connect() { 
        if (connected) 
            return;
        ::connect(s, a, a.size()); 
        connected = true; 
    }

    void disconnect() {
        closesocket(s);
        connected = false;
    }

public:
    socket(address const &a_) 
        : s(::socket(AF_INET, SOCK_STREAM, IPPROTO_TCP)), a(a_), connected(false) 
    {
        connect();
    }

    int send(std::string const &str) {
        return ::send(s, str.data(), str.size(), 0);
    }

    int read(void *buffer, size_t size) { 
        return ::recv(s, (char *)buffer, size, 0);
    }

    template <class T>
    int read(T &b) {
        return ::recv(s, (char *) &b, sizeof(b), 0);
    }

    ~socket() { closesocket(s); }
};
}

}

#endif

For the moment, I've only included IP and UDP. TCP would be similar, but would (obviously enough) create a connection-oriented TCP socket. To get an idea of what code using this would look like, let's consider a small utility to retrieve and display the time from a time server. To show both TCP and UDP sockets, I'll include two versions: one using the RFC 868 time protocol (which uses TCP), the other using the RFC 4330 SNTP protocol, which uses UDP. Note that for real use, the latter is strongly preferred (UDP imposes a great deal less overhead, and the SNTP packet format supports much more accurate results as well).

#include "sock2.h"
#include <time.h>

#if RFC_868
// TIME (RFC 868) version
int main(){
    IP::address a("time-c.nist.gov", 37);

    IP::TCP::socket s(a);
    DWORD t;
    s.read(t);
    time_t now = htonl(t) - 2208988800L;
    struct tm *n = localtime(&now);
    std::cout << n->tm_year + 1900 << "/" << n->tm_mon << "/" << n->tm_mday
        << "\t" << n->tm_hour << ":" << n->tm_min << ":" << n->tm_sec << "\n";
}

#else
// SNTP (RFC 4330) version
struct ntp_packet {
    // conversion from NTP epoch to Unix/Windows epoch (midnight jan 1, 1900 to midnight jan 1, 1970).
    static const int epoch = (86400U * (365U * 70U + 17U));
    static const int port = 123;
    static const int timeout = 6000;

    unsigned char mode_vn_li;
    unsigned char stratum;
    char poll;
    char precision;

    unsigned long root_delay;
    unsigned long root_dispersion;
    unsigned long reference_identifier;
    unsigned long reference_timestamp_secs;
    unsigned long reference_timestamp_fraq;
    unsigned long originate_timestamp_secs;
    unsigned long originate_timestamp_fraq;
    unsigned long receive_timestamp_seqs;
    unsigned long receive_timestamp_fraq;
    unsigned long transmit_timestamp_secs;
    unsigned long transmit_timestamp_fraq;

    ntp_packet() {
        memset(this, 0, sizeof(*this));
        mode_vn_li = (4 << 3) | 3;
        originate_timestamp_secs = htonl(time(0) + epoch);
    }

    operator time_t() { return ntohl(transmit_timestamp_secs) - epoch; }
};

int main() {
    IP::address a("time-c.nist.gov", ntp_packet::port);
    IP::UDP::socket s;

    int timeout = ntp_packet::timeout;
    s.setopt(SO_RCVTIMEO, timeout);

    ntp_packet packet;
    s.send(packet, a);
    s.read(a, packet);
    time_t now = (time_t) packet;
    struct tm *n = localtime(&now);
    std::cout << n->tm_year + 1900 << "/" << n->tm_mon+1 << "/" << n->tm_mday
        << "\t" << n->tm_hour << ":" << n->tm_min << ":" << n->tm_sec << "\n";
}

#endif

In both cases, the overhead imposed by the socket interface is fairly minimal. In fact, the majority of the space is simply for the declaration of the fields in the SNTP packet. I suppose that could be reduced, but it hardly seems worthwhile. Note that although we ignore most of these in SNTP, they're actually used in NTP, so the server expects the packet you send to include all the fields, even though we just set most of them to zeros.

A few notes: the SNTP code probably has at least some degree of fragility. In theory the compiler could insert arbitrary padding between the fields, so what we see comes out misaligned. In fact, doing so would be quite unusual--the packet format is designed so all the 32-bit fields are aligned to 32-bit boundaries, so the compiler won't normally insert extra padding between the fields.

These also assume that the time_t for the computer is based on an epoch of midnight, 1 Jan 1970. That's pretty common (used by both Windows and Linux), but not strictly required by standard C++ (though if memory serves, it is required by Posix).

This also depends on the ntp_packet being a standard layout class--i.e., since it doesn't contain any virtual functions or anything like that, it can be written to like a POD (but since it has a ctor, it's not technically a POD). Standard layout is new with C++11, so theoretically it could break on older compilers. Again, likelihood of actual breakage is quite low in practice (actually, I don't know of any compiler it will break with, though there are obviously quite a few with which I haven't tested).

Note, however, that those both apply only to the SNTP implementation, not to the actual sockets code. Also note the fairly minimal degree of overhead imposed by using sockets this way. We basically define an address object, open a socket, and read/write our data.

Also note the use of the template member functions for reading and writing data, as well as setting socket options (I've set the timeout on the UDP socket to demonstrate the latter). These simplify client code (no need to pass buffer sizes) as well as improving safety (no chance of accidentally passing the wrong size and overwriting a buffer).

So, some classes for sockets, and functioning RFC 868 and 4330 clients, with the longer of the two clients still only using 7 lines of code devoted to sockets. Of course, these are pretty simple protocols. Implementing things like WebSockets would add quite a be more code and complexity.

As a final note: although it's not nearly as elaborate in many ways, some might note that this code bears some resemblance to Boost ASIO, especially in terms of naming. That wasn't exactly intentional, but was a result of (I'd guess) similar thinking processes. For servers or clients that aren't as simple as the ones I've shown here, I'd give serious consideration to using ASIO instead of hand-rolling something like this. This is all right for simple tasks like I've shown here; if you need much more than that, ASIO is almost certainly a better choice.