How to terminate a thread correctly

This is the forum for miscellaneous technical/programming questions.

Moderator: 2ffat

How to terminate a thread correctly

Postby mark_c » Tue Mar 24, 2015 3:00 am

Hello everybody,
as we can observe, the code below create a thread with an infinite loop.
I read that it is dangerous call "TerminateThread" because it causes memory leak and other problems.
Another problem that I've observed is this: when I click on "Stop Button" and the "bAbort"
variable switch to "false", sometime the thread continue running ad infinitum.

Now I wonder what are the correct steps to terminate a thread and free allocated resource without exit
by application, ready for another start.

Thank you

Code: Select all
//---------------------------------------------------------------------------
#include <vcl\vcl.h>

#pragma hdrstop

#include "Unit1.h"

DWORD WINAPI ThreadTest( LPVOID lpParameter );
DWORD ID1;
HANDLE MyThread;
AnsiString buffer;
void SendData(AnsiString msg);
bool bAbort;

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

#pragma link "Grids"
#pragma resource "*.dfm"
TForm1 *Form1;
//---------------------------------------------------------------------------
__fastcall TForm1::TForm1(TComponent* Owner)
   : TForm(Owner)
{
        ServerSocket1->Port = 5000;
        ServerSocket1->Active = True;
 }
//---------------------------------------------------------------------------
void __fastcall TForm1::Button2Click(TObject *Sender)
{
        bAbort = false;
        TerminateThread(MyThread,0);
        Form1->Caption = "Stopped.....";
}
//---------------------------------------------------------------------------

void __fastcall TForm1::ServerSocket1ClientError(TObject *Sender,
      TCustomWinSocket *Socket, TErrorEvent ErrorEvent, int &ErrorCode)
{
        if (ErrorCode == 10053) {
                Socket->Close();
        }

        ErrorCode = 0;
}
//---------------------------------------------------------------------------

void __fastcall TForm1::Button4Click(TObject *Sender)
{
        bAbort = true;

        MyThread = CreateThread(NULL,0,ThreadTest,NULL,0,&ID1);

        buffer="";
        Form1->Caption = "Started.....";
}
//---------------------------------------------------------------------------

DWORD WINAPI ThreadTest( LPVOID lpParameter )
{
        while(bAbort)
        {

                for(int i=0; i<625000; i++) buffer+="ABC,xxx.xxx.xxx.xxx;";

                SendData(buffer);
                buffer="";

                Sleep(1000);
        }

        return 0;
}
//---------------------------------------------------------------------------

void SendData(AnsiString msg)
{
        try{
                for(int actconn = 0; actconn < Form1->ServerSocket1->Socket->ActiveConnections; actconn++)
                        Form1->ServerSocket1->Socket->Connections[actconn]->SendText(msg);

        } catch(...) { }
}
//---------------------------------------------------------------------------

void __fastcall TForm1::FormDestroy(TObject *Sender)
{
        ServerSocket1->Active = false;
}
//---------------------------------------------------------------------------


mark_c
BCBJ Guru
BCBJ Guru
 
Posts: 129
Joined: Thu Jun 21, 2012 1:13 am

Re: How to terminate a thread correctly

Postby rlebeau » Tue Mar 24, 2015 3:24 pm

mark_c wrote:I read that it is dangerous call "TerminateThread" because it causes memory leak and other problems.


That is correct.

mark_c wrote:when I click on "Stop Button" and the "bAbort"
variable switch to "false", sometime the thread continue running ad infinitum.


You are doing a LOT of memory (re)allocations inside your thread. It may just be taking a long time to finish those steps before it has a chance to check the abort flag. Setting the flag is just a signal, but the thread still has to check the signal periodically and exit as soon as possible.

mark_c wrote:Now I wonder what are the correct steps to terminate a thread and free allocated resource without exit by application, ready for another start.


Try something more like this instead:

Code: Select all
//---------------------------------------------------------------------------
#include <vcl\vcl.h>
#pragma hdrstop

#include "Unit1.h"

class TMyThread : public TThread
{
protected:
    virtual void __fastcall Execute();
public:
    __fastcall TMyThread();
};

TMyThread *MyThread = NULL;

void SendData(AnsiString msg);

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

#pragma link "Grids"
#pragma resource "*.dfm"
TForm1 *Form1;
//---------------------------------------------------------------------------
__fastcall TForm1::TForm1(TComponent* Owner)
   : TForm(Owner)
{
    ServerSocket1->Port = 5000;
    ServerSocket1->Active = True;
}
//---------------------------------------------------------------------------
void __fastcall TForm1::Button2Click(TObject *Sender)
{
    if (MyThread)
    {
        MyThread->Terminate();
        MyThread->WaitFor();
        delete MyThread;
        MyThread = NULL;
    }

    Caption = "Stopped.....";
}
//---------------------------------------------------------------------------

void __fastcall TForm1::ServerSocket1ClientError(TObject *Sender,
      TCustomWinSocket *Socket, TErrorEvent ErrorEvent, int &ErrorCode)
{
    Socket->Close();
    ErrorCode = 0;
}
//---------------------------------------------------------------------------

void __fastcall TForm1::Button4Click(TObject *Sender)
{
    if (MyThread)
        Button2->Click();

    MyThread = new TMyThread;

    Caption = "Started.....";
}
//---------------------------------------------------------------------------

__fastcall TMyThread::TMyThread()
    : TThread(false)
{
}

void __fastcall TMyThread::Execute()
{
    while (!Terminated)
    {
        AnsiString buffer;
        buffer.SetLength(625000 * 20);

        char *ptr = buffer.c_str();
        for(int i = 0; i < 625000; ++i)
        {
            if (Terminated) return;
            strcpy(ptr, "ABC,xxx.xxx.xxx.xxx;");
            ptr += 20;
        }

        if (Terminated) return;
        SendData(buffer);

        if (Terminated) return;
        Sleep(1000);
    }
}
//---------------------------------------------------------------------------

void SendData(AnsiString msg)
{
    try
    {
        for(int actconn = 0; actconn < Form1->ServerSocket1->Socket->ActiveConnections; ++actconn)
            Form1->ServerSocket1->Socket->Connections[actconn]->SendText(msg);
    }
    catch (const Exception &)
    {
    }
}
//---------------------------------------------------------------------------

void __fastcall TForm1::FormClose(TObject *Sender, TCloseAction &Action)
{
    Button2->Click();
    ServerSocket1->Active = false;
}
//---------------------------------------------------------------------------


With that said, there is still a problem in this code - SendData() is not thread-safe. Clients could connect/disconnect, thus altering the ActiveConnections list, while SendData() is running, leading to all kinds of problems. I will leave that as an exercise for you to fix that.
Remy Lebeau (TeamB)
Lebeau Software
User avatar
rlebeau
BCBJ Author
BCBJ Author
 
Posts: 1545
Joined: Wed Jun 01, 2005 3:21 am
Location: California, USA

Re: How to terminate a thread correctly

Postby mark_c » Thu Mar 26, 2015 1:47 pm

thanks for your very clear answer. Sorry but I do not understand, however, what types of problems may experience SendData (). I simply thought that if a client disconnects while receiving data at the most lose some data. What escapes me?
mark_c
BCBJ Guru
BCBJ Guru
 
Posts: 129
Joined: Thu Jun 21, 2012 1:13 am

Re: How to terminate a thread correctly

Postby rlebeau » Thu Mar 26, 2015 5:10 pm

mark_c wrote:Sorry but I do not understand, however, what types of problems may experience SendData (). I simply thought that if a client disconnects while receiving data at the most lose some data. What escapes me?


I told you why your current SendData() implementation is not safe:

"Clients could connect/disconnect, thus altering the ActiveConnections list, while SendData() is running"

You are calling SendData() in a worker thread, but TServerSocket is running in the main UI thread. Imagine what happens if SendData() is looping through the Connections list when a client disconnects mid-loop. The main thread removes the client from the list that your worker thread is still looping through.

You might skip a legitimate client if the removal happens after you finish calling SendText() and before your loop counter increments.

You might get a bounds error from Connections[] if the removal happens between the time that you check ActiveConnections and the time you access Connections[].

You might crash with an AccessViolation, or even corrupt memory, from accessing a freed TCustomWinSocket object if the removal happens after you have accessed Connections[] and before you call SendText().

These are the kinds of race conditions and dangerous things that can happen when you access data/objects across thread boundaries without adequate synchronization.
Last edited by rlebeau on Fri Mar 27, 2015 12:38 pm, edited 1 time in total.
Remy Lebeau (TeamB)
Lebeau Software
User avatar
rlebeau
BCBJ Author
BCBJ Author
 
Posts: 1545
Joined: Wed Jun 01, 2005 3:21 am
Location: California, USA

Re: How to terminate a thread correctly

Postby mark_c » Fri Mar 27, 2015 1:12 am

edit
Last edited by mark_c on Fri Mar 27, 2015 1:15 am, edited 1 time in total.
mark_c
BCBJ Guru
BCBJ Guru
 
Posts: 129
Joined: Thu Jun 21, 2012 1:13 am

Re: How to terminate a thread correctly

Postby mark_c » Fri Mar 27, 2015 1:14 am

thanks for the clarification, but, sorry, the problems arise only if I have multiple clients connected to my server is right?
If the client is only one the race-condition does not occur, but is it right?


another question is this: there problems of race condition using visual components such as Label, EditText and so on using only one thread?

http://www.borlandtalk.com/thread-safe- ... 07682.html
mark_c
BCBJ Guru
BCBJ Guru
 
Posts: 129
Joined: Thu Jun 21, 2012 1:13 am

Re: How to terminate a thread correctly

Postby rlebeau » Fri Mar 27, 2015 12:46 pm

mark_c wrote:thanks for the clarification, but, sorry, the problems arise only if I have multiple clients connected to my server is right?


No. Several of the conditions I mentioned can happen when accessing any given client. So having just one client is enough to cause problems.

The fact of the matter is the way you have implemented SendData() *is not thread-safe* and needs to be re-written to avoid any race conditions. In this case, since you are using TServerSocket in its default non-blocking mode, SendData() should delegate its data to the main UI thread and have the main thread loop through the Connection list sending data as needed. Try something like this:

Code: Select all
void __fastcall TForm1::ServerSocket1ClientConnect(TObject *Sender, TCustomWinSocket *Socket)
{
    Socket->Data = new AnsiString; // cache for pending data
}

void __fastcall TForm1::ServerSocket1ClientDisconnect(TObject *Sender, TCustomWinSocket *Socket)
{
    delete static_cast<AnsiString*>(Socket->Data);
}

void __fastcall TForm1::ServerSocket1ClientWrite(TObject *Sender, TCustomWinSocket *Socket)
{
    AnsiString *PendingData = static_cast<AnsiString*>(Socket->Data);
    if (!PendingData->IsEmpty())
    {
        int sent = Socket->SendText(*PendingData);
        if (sent > 0)
            PendingData->Delete(1, sent);
    }
}

static const UINT APP_SEND_DATA = WM_APP + 1;

void SendData(AnsiString msg)
{
    AnsiString *pmsg = new AnsiString(msg);
    if (!PostThreadMessage(MainThreadID, APP_SEND_DATA, 0, pmsg))
        delete pmsg;
}

__fastcall TForm1::TForm1(TComponent *Owner)
    : TForm(Owner)
{
    Application->OnMessage = AppMessage;
}

__fastcall TForm1::~TForm1()
{
    Application->OnMessage = NULL;
}

void __fastcall TForm1::AppMessage(MSG &Msg, bool &Handled)
{
    if (Msg.message != APP_SEND_DATA)
        return;

    Handled = true;
    AnsiString *pmsg = static_cast<AnsiString*>(Msg.lParam);

    for (int actconn = 0; actconn < ServerSocket1->Socket->ActiveConnections; ++actconn)
    {
        TCustomWinSocket *client = ServerSocket1->Socket->Connections[actconn];
        AnsiString *PendingData = static_cast<AnsiString*>(client->Data);

        if (PendingData->IsEmpty())
        {
            int sent = client->SendText(*pmsg);
            if ((sent > 0) && (sent < pmsg->Length()))
                *PendingData = pmsg->SubString(sent+1, MaxInt);
        }
        else
            *PendingData += *pmsg;
    }

    delete pmsg;
}


mark_c wrote:there problems of race condition using visual components such as Label, EditText and so on using only one thread?


Yes, because you don't have one thread accessing them, you have two threads - the main UI thread, and your worker thread. ANY access to the UI from a worker thread must be synchronized with the main UI thread. ONLY the main UI thread should be making direct UI calls.
Last edited by rlebeau on Fri Mar 27, 2015 1:14 pm, edited 2 times in total.
Remy Lebeau (TeamB)
Lebeau Software
User avatar
rlebeau
BCBJ Author
BCBJ Author
 
Posts: 1545
Joined: Wed Jun 01, 2005 3:21 am
Location: California, USA

Re: How to terminate a thread correctly

Postby mark_c » Fri Mar 27, 2015 1:11 pm

Now I understand why sometimes, randomly, are displayed errors and usually when writing data in Labels; in this case the characters in the form change the size or style randomly (bold, normal).

You've explained that it is better to delegate the main thread to send data on the socket, what do you mean?
Does it mean that the function SendData () must be called in the main thread?
Last question: using the socket to blocking mode I would solve the problem of race-condition when sending data to multiple clients?

thank you
mark_c
BCBJ Guru
BCBJ Guru
 
Posts: 129
Joined: Thu Jun 21, 2012 1:13 am

Re: How to terminate a thread correctly

Postby rlebeau » Fri Mar 27, 2015 1:19 pm

mark_c wrote:You've explained that it is better to delegate the main thread to send data on the socket, what do you mean?


I edited my previous message to include an example.

mark_c wrote:Does it mean that the function SendData () must be called in the main thread?


That is one way to do it. Or just have it send its data to the main thread (as my example does).

mark_c wrote:using the socket to blocking mode I would solve the problem of race-condition when sending data to multiple clients?


No. When TServerSocket is run in that mode, each client will be in its own dedicated thread. So you would have to maintain a thread-safe queue of data for each client, push data into that queue when needed, and have each client thread dip into its queue periodically. This way, a send to a given client is performed in the context of that client's thread, not in the main thread, or in the data pushing thread.
Remy Lebeau (TeamB)
Lebeau Software
User avatar
rlebeau
BCBJ Author
BCBJ Author
 
Posts: 1545
Joined: Wed Jun 01, 2005 3:21 am
Location: California, USA

Re: How to terminate a thread correctly

Postby mark_c » Mon Mar 30, 2015 6:13 am

hello,
I've another question: in order to understand the mechanism of race-condition between the main thread and a thread implemented by me, I wrote a short code below that generates an error, but I am not clear at what point in its execution.
Sorry if I used again CreateThread instead of the class made available by the VCL.

thank you

Code: Select all
//---------------------------------------------------------------------------
#include <vcl\vcl.h>

#pragma hdrstop

#include "Unit1.h"

HANDLE MyThread;
DWORD WINAPI ThreadTest( LPVOID lpParameter );
DWORD ID1;

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

#pragma link "Grids"
#pragma resource "*.dfm"
TForm1 *Form1;
//---------------------------------------------------------------------------
__fastcall TForm1::TForm1(TComponent* Owner)
   : TForm(Owner)
{
 }
//---------------------------------------------------------------------------

void __fastcall TForm1::Button1Click(TObject *Sender)
{
        MyThread=CreateThread(NULL,0,ThreadTest,NULL,0,&ID1);
        Form1->Caption = "Started.....";
}
//---------------------------------------------------------------------------

DWORD WINAPI ThreadTest( LPVOID lpParameter )
{
   for(int i=0; i<10000; i++)
                  Form1->Label1->Caption=i;

        Form1->Caption = "Stopped.....";
        Sleep(50);
        TerminateThread(MyThread,0);
}
//---------------------------------------------------------------------------
mark_c
BCBJ Guru
BCBJ Guru
 
Posts: 129
Joined: Thu Jun 21, 2012 1:13 am

Re: How to terminate a thread correctly

Postby rlebeau » Mon Mar 30, 2015 5:04 pm

mark_c wrote:in order to understand the mechanism of race-condition between the main thread and a thread implemented by me, I wrote a short code below that generates an error, but I am not clear at what point in its execution.


Your worker thread is directly accessing the UI (the TLabel::Caption and TForm::Caption properties). YOU CANNOT DO THAT! You MUST delegate the access to the main UI thread. If you don't want to use the TThread class, you will have to implement the delegation manually.

Also, DO NOT use TerminateThread()! And certainly DO NOT make a thread terminate itself like that. All you have to do is exit from your thread callback like any normal function exit.

Try something more like this (this is just one way to delegate, there are other options available as well):

Code: Select all
//---------------------------------------------------------------------------
#include <vcl\vcl.h>

#pragma hdrstop

#include "Unit1.h"

HANDLE MyThread;
DWORD WINAPI ThreadTest( LPVOID lpParameter );
DWORD ID1;
HWND hMsgWnd;

static const UINT APPWM_SET_LABEL_CAPTION = WM_APP + 1;
static const UINT APPWM_SET_FORM_CAPTION = WM_APP + 2;

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

#pragma link "Grids"
#pragma resource "*.dfm"
TForm1 *Form1;
//---------------------------------------------------------------------------
__fastcall TForm1::TForm1(TComponent* Owner)
   : TForm(Owner)
{
    hMsgWnd = AllocateHWnd(MyAppWndProc);
}
//---------------------------------------------------------------------------

__fastcall TForm1::~TForm1()
{
    DeallocateHWnd(hMsgWnd);
}
//---------------------------------------------------------------------------

void __fastcall TForm1::MyAppWndProc(TMessage &Message)
{
    switch (Message.Msg)
    {
        case APPWM_SET_LABEL_CAPTION:
            Label1->Caption = String(reinterpret_cast<PChar>(Message.LParam), Message.WParam);
            break;

        case APPWM_SET_FORM_CAPTION:
            this->Caption = String(reinterpret_cast<PChar>(Message.LParam), Message.WParam);
            break;

        default:
            Message.Result = DefWindowProc(hMsgWnd, Message.Msg, Message.WParam, Message.LParam);
            break;
}
//---------------------------------------------------------------------------

void __fastcall TForm1::Button1Click(TObject *Sender)
{
    MyThread = CreateThread(NULL, 0, ThreadTest, NULL, 0, &ID1);
    this->Caption = "Started.....";
}
//---------------------------------------------------------------------------

void SetLabelCaption(const String &s)
{
    SendMessage(hMsgWnd, APPWM_SET_LABEL_CAPTION, s.Length(), reinterpret_cast<LPARAM>(s.c_str()));
}

void SetFormCaption(const String &s)
{
    SendMessage(hMsgWnd, APPWM_SET_FORM_CAPTION, s.Length(), reinterpret_cast<LPARAM>(s.c_str()));
}

DWORD WINAPI ThreadTest( LPVOID lpParameter )
{
    for(int i = 0; i < 10000; ++i)
        SetLabelCaption(i);

    SetFormCaption("Stopped.....");
    Sleep(50);

    return 0;
}
//---------------------------------------------------------------------------
Last edited by rlebeau on Tue Mar 31, 2015 2:05 pm, edited 1 time in total.
Remy Lebeau (TeamB)
Lebeau Software
User avatar
rlebeau
BCBJ Author
BCBJ Author
 
Posts: 1545
Joined: Wed Jun 01, 2005 3:21 am
Location: California, USA

Re: How to terminate a thread correctly

Postby mark_c » Tue Mar 31, 2015 1:34 am

rlebeau be patient, I'm learning. So, ultimately, if I use the TThread class this is thread safe?
And in thise case do not need to add semaphores or other?

thank you
mark_c
BCBJ Guru
BCBJ Guru
 
Posts: 129
Joined: Thu Jun 21, 2012 1:13 am

Re: How to terminate a thread correctly

Postby rlebeau » Tue Mar 31, 2015 2:37 pm

mark_c wrote:So, ultimately, if I use the TThread class this is thread safe?


I already showed you how to make the code thread-safe using TThread.
Remy Lebeau (TeamB)
Lebeau Software
User avatar
rlebeau
BCBJ Author
BCBJ Author
 
Posts: 1545
Joined: Wed Jun 01, 2005 3:21 am
Location: California, USA

Re: How to terminate a thread correctly

Postby mark_c » Wed Apr 01, 2015 12:16 am

thanks, sorry but I needed only a further confirmation. Last question: is there a manual that explains in detail how they are handled by the VCL thread?

thank you
mark_c
BCBJ Guru
BCBJ Guru
 
Posts: 129
Joined: Thu Jun 21, 2012 1:13 am

Re: How to terminate a thread correctly

Postby rlebeau » Wed Apr 01, 2015 12:41 am

mark_c wrote:is there a manual that explains in detail how they are handled by the VCL thread?


Writing Multithreaded Applications Index

Using the Main VCL Thread **

In a nutshell, TThread::Synchronize() and TThread::Queue() place requests into a thread-safe queue, and then the main thread periodically checks that queue for pending requests and executes them. Synchronize() waits for its request to be processed, whereas Queue() does not.

** Ignore the part about you having to call CheckSynchronize() manually. The VCL already handles that automatically for you. The only time you would have to call CheckSynchronize() manually is if you are blocking the main thread from processing messages, or you need to call TThread::Synchronize()/Queue() inside a DLL.
Remy Lebeau (TeamB)
Lebeau Software
User avatar
rlebeau
BCBJ Author
BCBJ Author
 
Posts: 1545
Joined: Wed Jun 01, 2005 3:21 am
Location: California, USA

Next

Return to Technical

Who is online

Users browsing this forum: Bing [Bot] and 11 guests

cron