Multithreaded Socket code to connect to 100 different machines

Question

This is my socket.cpp

#pragma hdrstop

#include "MySocketClient.h"

MySocketClient::MySocketClient() throw(SocketException){
    WSAData data;
    InitializeCriticalSection(&sect_);
    if(WSAStartup(MAKEWORD(2,2),&data)!=0){
        throw SocketException("Cant load WinSock library");
    }
    connected_=false;
}
//-----------------------------------------------------------------------------

MySocketClient::~MySocketClient(){
    WSACleanup();
}
//-----------------------------------------------------------------------------

void MySocketClient::receive(void){
    char recBuf[1024];
    while(1){
        EnterCriticalSection(&sect_);
        memset(recBuf,0,1024);
        int recLen=recv(sock_,recBuf,1024,0);
        if(recLen==0){
            onDisconnect(sock_);
            connected_=false;
            break;
        }else if(recLen<0){
            connected_=false;
            onConnectionError(sock_);
            break;
        }else{
            onReceive(sock_,recBuf,recLen);
        }
        LeaveCriticalSection(&sect_);
    }
}
//------------------------------------------------------------------------------
void MySocketClient::sendData(char *sendBuf,int sendLen) throw(SocketException){
    onSendData(sendBuf,sendLen);
    if(send(sock_,sendBuf,sendLen,0)<0){
        onConnectionError(sock_);
    }
    Sleep(200);
}
//------------------------------------------------------------------------------
void MySocketClient::Connect(string hostname,unsigned short port) throw(SocketException){
    sock_=socket(AF_INET,SOCK_STREAM,IPPROTO_TCP);
    if(sock_==INVALID_SOCKET){
        throw SocketException("Cant create socket");
    }
    HOSTENT *host=gethostbyname(hostname.c_str());
    if(host==NULL){
        throw SocketException("Cant resolve host");
    }
    addr_.sin_family=AF_INET;
    addr_.sin_port=htons(port);
    addr_.sin_addr.S_un.S_addr=*(unsigned long*)host->h_addr_list[0];
    if(connect(sock_,(sockaddr*)&addr_,sizeof(sockaddr))==SOCKET_ERROR){
        throw SocketException("Connect Error");
    }
    DWORD dwThreadId;
    threadHandle_=CreateThread(NULL,0,startReceive,this,0,&dwThreadId);
    if(threadHandle_==NULL){
        connected_=false;
        shutdown(sock_,SD_BOTH);
        closesocket(sock_);
        throw SocketException("Receive thread create error");
    }
    connected_=true;
    onConnect(sock_);
}
//------------------------------------------------------------------------------



void MySocketClient::Disconnect(void){
    TerminateThread(threadHandle_,0);
    shutdown(sock_,SD_BOTH);
    closesocket(sock_);
}

        //Virtual functions
void MySocketClient::onReceive(SOCKET sock,char *recBuf,int recLen) throw(SocketException){
}

void MySocketClient::onDisconnect(SOCKET sock){
}


void MySocketClient::onConnectionError(SOCKET sock){
}

void MySocketClient::onSendData(char *sendBuf,int sendLen){

}

void MySocketClient::onConnect(SOCKET sock){
}
#pragma package(smart_init)

This is mysocket.h

//---------------------------------------------------------------------------


#ifndef MySocketClientH
#define MySocketClientH
#include <winsock2.h>
#include <string.h>
#include "socketException.h"
#include <process.h>
using namespace std;
class MySocketClient{
    private     :
        sockaddr_in             addr_;
        unsigned short          port_;
        string                  hostname_;
        CRITICAL_SECTION        sect_;
        HANDLE                  threadHandle_;
        bool                    connected_;
    protected   :
        SOCKET          sock_;

        //Virtual functions
        virtual void onConnect(SOCKET sock);
        virtual void onReceive(SOCKET sock,char *recBuf,int recLen) throw(SocketException);
        virtual void onDisconnect(SOCKET sock) ;
        virtual void onConnectionError(SOCKET sock);
        virtual void onSendData(char *sendBuf,int sendLen);
    private :
        void receive(void);
        static DWORD WINAPI startReceive(LPVOID param){
            MySocketClient *_this=(MySocketClient*)param;
            _this->receive();
            return 0;
        }
    public      :
        MySocketClient() throw(SocketException);
        ~MySocketClient();
        void sendData(char *sendBuf,int sendLen) throw(SocketException);
        void Connect(string hostname,unsigned short port) throw(SocketException);
        void Disconnect(void);
        bool isConnected(void){
            return connected_;
        }

};
#endif

The question that i asked is on the followink link https://stackoverflow.com/questions/12471761/creating-multiple-tcp-socket-connections

Answer 1

The code generally looks ok. Does it work as expected?

What is the reason for using a critical section in MySocketClient::receive()? Are you sharing common data between multiple threads? It doesnt appear so, and even if you were, this wouldnt protect anything since each instance has its own critical section variable. In order for multiple threads to execute code mutually exclusively, then they all need to use the same critical section variable. So, to summarize, I think you can remove the critical section variable.

Regarding the design, it would be more cohesive if you separated the socket communication from the thread handling in separate classes. It will work as is, but it would be cleaner to separate them.

Its generally cleaner to initialize the class variables in the constructor with a constructor initializer list as follows:

MySocketClient::MySocketClient() throw(SocketException) : 
    connected_(false), sock_(-1) // etc
{
}

If you plan on leaving the thread creation code in Connect(), then all code should be placed before creating the thread to avoid race conditions, as follows:

void MySocketClient::Connect(string hostname,unsigned short port) throw(SocketException){

    // ...

    // Notice I moved these 2 lines     
    connected_=true;
    onConnect(sock_);

    DWORD dwThreadId;
    threadHandle_=CreateThread(NULL,0,startReceive,this,0,&dwThreadId);
    if(threadHandle_==NULL){
        connected_=false;
        shutdown(sock_,SD_BOTH);
        closesocket(sock_);
        throw SocketException("Receive thread create error");
    }

    // Dont put any code here that accesses instance variables to avoid race conditions
}

One last thing, its usually considered good form to provide a way to exit the infinite while loop. So you should consider changing is as follows:

void MySocketClient::receive(void){
    char recBuf[1024];
    while(run_){
        // ...
    }
}

Then, add a setter/getter to the MySocketClient class to set the field, thus allowing the client to exit gracefully. It should be protected, preferably by a read/write lock.

Answer 2

It could be that at 100 connections you won't notice, but you should be aware that the 1-thread-per-connection model does not scale well and is not in line with the current state of the art. See what is written about the c10k problem.

With a thread-per-connection approach you will have high memory overhead from each thread's stack, and you will spend a lot of time context switching.

What scalable network code does these days is:

• On Unix: Use epoll (Linux), kqueue (*BSD) or equivalents with non-blocking sockets. The model here is that you ask the kernel to block until some file descriptor is ready for I/O. It informs you which fd is ready and you can issue the non-blocking read or write.

• On Windows: Issue your I/O with "overlapped" I/O, and use I/O completion ports or APC to be notified when the I/O is finished.

In both of these models the ultimate goal is to not spend time waiting for synchronous I/O, such as a blocking read or write to the socket. This allows you to process multiple requests without needing multiple threads.