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

From:

Kurt <kurt.knudsen@gmail.com>

Newsgroups:

comp.lang.c++

Date:

Mon, 8 Feb 2010 07:09:36 -0800 (PST)

Message-ID:

<950ffaf5-5222-4bb7-8e20-6dceca5da373@a5g2000yqi.googlegroups.com>

On Feb 6, 1:45 pm, Maxim Yegorushkin <maxim.yegorush...@gmail.com>
wrote:

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 c=
reates 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 cl=
ient and server both share similar code, but I'll just post the server-side=
..

void CTransferServer::startCommandThread() {
int iCommandPort = _wtoi(configHandler->getTextValue(L"Transfe=

rCommandPort"));

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 comm=

and 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, t=

ransferCommandWorkerThread, params, 0,&dwGenericThread);

//HANDLE hWorkerThread = (HANDLE)_beginthreade=

x(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 passe=

s it off to the thread. I am not 100% familiar with sockets and threads, bu=
t 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 communica=
tion between the server and client to get disconnected or in a deadlock sta=
te. 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 handl=

es.

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

Hi Max,

Thanks for the brief introduction. I'll create a branch project and
implement boost into it and see how it goes. It's so hard for me to
debug this code since I didn't originally code it and the contractor
is essentially saying 'You made changes, you're SOL.' I came in today
and things seem to be going all right on the server. I need to check
the clients to see how they are running, though. I had a few bugs I
inserted into the code, which the contractor saw and I assume which is
why he's not willing to offer much help, that I removed.

I appreciate everyone's feedback and it seems I'm heading in the right
direction. It seems that people love Boost and I'm sure it's a good
resume builder.

Thanks,

Kurt