mark_c wrote:I ask you if you think there may be collisions in this version, I don't see any
There are some issues with your code.
You have a race condition on your global 'msg' variable. Both threads are modifying 'msg' without synchronizing with each other, and without adequately synchronizing with the main UI thread. For instance, after thread 1 modifies 'msg' and then blocks waiting for the main UI thread to use 'msg', thread 2 is still free to modify 'msg' at will, even while the loop inside of SendMyMessage() is actively using it. So you need to put a full synchronization lock around 'msg', such as a
critical section or
mutex.
You also have a race condition on your global 'a' and 'b' variables, too. For those, you can simply use the
Interlocked API instead of using a full locking mechanism.
Also, in your OnDestroy event (and FYI, DO NOT use the OnDestroy event in C++! Use the Form's destructor instead), DO NOT use TerminateThread() at all! Signal your threads to abort, and then WAIT for them to actually finish their work.
With all of that said, try something more like the following instead (which BTW, really isn't the best way to handle what you are doing, but this is just to show how to fix the code you provided):
- Code: Select all
//---------------------------------------------------------------------------
#include <vcl.h>
#pragma hdrstop
#include "Unit1.h"
#include <stdlib.h>
#include <stdio.h>
bool bAbort = false;
void SendMyMessage(char *msg);
DWORD WINAPI Thread1( LPVOID lpParameter );
DWORD WINAPI Thread2( LPVOID lpParameter );
HANDLE Trd1 = NULL, Trd2 = NULL;
LONG a, b;
System::String msg;
CRITICAL_SECTION msglock;
//---------------------------------------------------------------------------
#pragma package(smart_init)
#pragma resource "*.dfm"
TForm1 *Form1;
//---------------------------------------------------------------------------
__fastcall TForm1::TForm1(TComponent* Owner)
: TForm(Owner)
{
InitializeCriticalSection(&msglock);
ServerSocket1->Port = 5000;
ServerSocket1->Active = True;
}
//---------------------------------------------------------------------------
__fastcall TForm1::~TForm1()
{
StopThreads();
ServerSocket1->Active = False;
DeleteCriticalSection(&msglock);
}
//---------------------------------------------------------------------------
void __fastcall TForm1::Button1Click(TObject *Sender)
{
StopThreads();
bAbort = false;
a = b = 0;
Trd1 = CreateThread(NULL, 0, Thread1, NULL, 0, NULL);
Trd2 = CreateThread(NULL, 0, Thread2, NULL, 0, NULL);
Button1->Enabled = false;
Button2->Enabled = true;
Caption = "Started.....";
}
//---------------------------------------------------------------------------
void __fastcall TForm1::Button2Click(TObject *Sender)
{
StopThreads();
Button1->Enabled = true;
Button2->Enabled = false;
Caption = "Stopped.....";
}
//---------------------------------------------------------------------------
void __fastcall TForm1::StopThreads()
{
if ((!Trd1) && (!Trd2))
return;
bAbort = true;
HANDLE arr[3];
DWORD num = 0;
arr[num++] = reinterpret_cast<HANDLE>(System::Classes::SyncEvent);
if (Trd1) arr[num++] = Trd1;
if (Trd2) arr[num++] = Trd2;
do
{
DWORD WaitResult = MsgWaitForMultipleObjects(num, arr, FALSE, INFINITE, QS_SENDMESSAGE);
if (WaitResult == WAIT_FAILED)
{
RaiseLastOSError();
break;
}
if ((WaitResult >= WAIT_OBJECT_0) && (WaitResult < (WAIT_OBJECT_0 + num)))
{
DWORD idx = WaitResult - WAIT_OBJECT_0;
if (idx == 0)
System::Classes::CheckSynchronize();
else
{
HANDLE h = arr[idx];
CloseHandle(h);
if (h == Trd1) Trd1 = NULL;
else if (h == Trd2) Trd2 = NULL;
num = 1;
if (Trd1) arr[num++] = Trd1;
if (Trd2) arr[num++] = Trd2;
}
}
else if (WaitResult == (WAIT_OBJECT_0 + num))
{
MSG msg;
PeekMessage(&Msg, 0, 0, 0, PM_NOREMOVE);
}
}
while (num > 1);
}
//---------------------------------------------------------------------------
void __fastcall TForm1::DoUpdateLabel()
{
Label1->Caption = InterlockedExchangeAdd(&a, 0);
Label2->Caption = InterlockedExchangeAdd(&b, 0);
}
//---------------------------------------------------------------------------
DWORD WINAPI Thread1( LPVOID lpParameter )
{
while (!bAbort)
{
EnterCriticalSection(&msglock);
msg = " msg1 ";
LeaveCriticalSection(&msglock);
TThread::Synchronize(NULL, Form1->SendMyMessage);
InterlockedIncrement(&a);
TThread::Synchronize(NULL, Form1->DoUpdateLabel);
Sleep(1);
}
return 0;
}
//---------------------------------------------------------------------------
DWORD WINAPI Thread2( LPVOID lpParameter )
{
while (!bAbort)
{
EnterCriticalSection(&msglock);
msg=" msg2 ";
LeaveCriticalSection(&msglock);
TThread::Synchronize(NULL, Form1->SendMyMessage);
InterlockedIncrement(&b);
TThread::Synchronize(NULL, Form1->DoUpdateLabel);
Sleep(1);
}
return 0;
}
//---------------------------------------------------------------------------
//void SendMyMessage(char *msg)
void __fastcall TForm1::SendMyMessage()
{
System::String local_msg;
try
{
EnterCriticalSection(&msglock);
local_msg = msg;
LeaveCriticalSection(&msglock);
local_msg.Unique();
for(int actconn = 0; actconn < ServerSocket1->Socket->ActiveConnections; ++actconn)
ServerSocket1->Socket->Connections[actconn]->SendText(local_msg);
}
catch(...)
{
}
}
//---------------------------------------------------------------------------
void __fastcall TForm1::ServerSocket1ClientError(TObject *Sender, TCustomWinSocket *Socket,
TErrorEvent ErrorEvent, int &ErrorCode)
{
Socket->Close();
ErrorCode = 0;
}
//---------------------------------------------------------------------------