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

Discussion in 'C++' started by Maxim Yegorushkin, Feb 6, 2010.

  1. 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
    Maxim Yegorushkin, Feb 6, 2010
    #1
    1. Advertising

  2. Maxim Yegorushkin

    Kurt Guest

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

    On Feb 6, 1:45 pm, Maxim Yegorushkin <>
    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 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


    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
    Kurt, Feb 8, 2010
    #2
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. never ending progress bar

    , May 5, 2005, in forum: ASP .Net
    Replies:
    1
    Views:
    530
    Lucas Tam
    May 5, 2005
  2. Craven Birds

    Newbie! Never ending Webpage!

    Craven Birds, Aug 1, 2004, in forum: HTML
    Replies:
    42
    Views:
    1,424
    Uncle Pirate
    Aug 5, 2004
  3. Replies:
    1
    Views:
    439
    Jim Moe
    Mar 16, 2006
  4. Branimir Maksimovic
    Replies:
    5
    Views:
    296
    James Kanze
    Feb 7, 2010
  5. Isaac Won
    Replies:
    9
    Views:
    353
    Ulrich Eckhardt
    Mar 4, 2013
Loading...

Share This Page