Re: Loop generates never-ending sockets and threads, need help debugging

From:

Maxim Yegorushkin <maxim.yegorushkin@gmail.com>

Newsgroups:

comp.lang.c++

Date:

Sat, 06 Feb 2010 18:45:56 +0000

Message-ID:

<4b6db8e4$0$9753$6e1ede2f@read.cnntp.org>

On 05/02/10 17:55, Kurt wrote:

Hi all,

I have a client/server project written by a hired contractor, but it's got a fairly big bug when it comes to dealing with thread and sockets. It creates threads but never closes the handle to them, thus causing the server to accumulate hundreds of thousands of open handles in a day or so. The client and server both share similar code, but I'll just post the server-side.

void CTransferServer::startCommandThread() {
    int iCommandPort = _wtoi(configHandler->getTextValue(L"TransferCommandPort"));
    SOCKET socCommand = CSocHandler::getServerSocket(iCommandPort);
    if (socCommand == INVALID_SOCKET) {
        //cout<< "\nCould not create transfer command server socket."<< WSAGetLastError();
        return;
    }

    if (listen(socCommand, SOMAXCONN)) {
        //cout<< "\nCould not listen on transfer command port."<< WSAGetLastError();
        return;
    }
    while (blStarted) {
        SOCKET sa = accept(socCommand, 0, 0); // block for connection request
        if (sa ==INVALID_SOCKET) {
            break;
        }
        void **params = (void **) malloc(sizeof(void*)*2);
        SOCKET *s = new SOCKET;
        *s = sa;
        params[0] = (void*)this;
        params[1] = (void*)s;
        DWORD dwGenericThread;
        //unsigned int iGenericThread;
        HANDLE hWorkerThread = CreateThread(NULL, 0, transferCommandWorkerThread, params, 0,&dwGenericThread);
        //HANDLE hWorkerThread = (HANDLE)_beginthreadex(NULL, 0, transferCommandWorkerThread, params, 0,&iGenericThread);
        //WaitForSingleObject(hWorkerThread, INFINITE);
        //CloseHandle(hWorkerThread);
    }
}

DWORD WINAPI transferCommandWorkerThread(LPVOID param) {
    void **params = (void **)param;
    CTransferServer *transferServer = (CTransferServer*)params[0];
    SOCKET *commandSocket = (SOCKET*)params[1];
    transferServer->handleCommandConnection(*commandSocket);
    delete commandSocket;
    free((void*)params);
    return 0;
}

So, the while-loop creates a socket that accepts a connection and passes it off to the thread. I am not 100% familiar with sockets and threads, but I have a general idea about them. As far as I can tell, the workerThread does close/delete the sockets properly, but the main loop never closes the handle to the thread. I tried adding WaitForSingleObject() and CloseHandle() but it appears to close the thread prematurely. This causes the communication between the server and client to get disconnected or in a deadlock state. I heard that _beginthreadex() is better to use, but when I tried that, I believe it made matters worse.

Any ideas on how to debug this or perhaps a better way of writing this?

If you can you boost, you should. It can take care of all the
boilerplate code for starting a new thread and passing arguments to that
thread. Applying it to your problem:

#include <boost/bind.hpp>
#include <boost/thread.hpp>

struct CTransferServer
{
     void startCommandThread();
     void handleCommandConnection(SOCKET client);

     bool blStarted;
};

void CTransferServer::startCommandThread() {
     // ...
     SOCKET socCommand;
    while (blStarted) {
        SOCKET sa = accept(socCommand, 0, 0);
        if (sa ==INVALID_SOCKET)
            break;
     boost::thread( // start a new thread
         boost::bind( // in a member function of this
               &CTransferServer::handleCommandConnection, this
             , sa // pass this extra argument
             )
         );
    }
}

It is much shorter as the original version and does not leak thread handles.

A gotcha, just in case, CTransferServer destructor will need to make
sure that no threads are running before it returns. Otherwise those
threads will end up accessing an already destroyed object.

--
Max