Re: Threads, Message Pump and Sync
On Dec 7, 11:08 am, MariusVE <pris...@gmail.com> wrote:
This is a MFC dialog-based application with only one dialog and two
buttons: "Start" and "Exit".
When the user presses the start button, the application goes through a
for loop of allocating CString's and posts them to the UI thread.
When pressing EXIT, the application shuts down.
I have two threads: 1 is responsable w/ allocating the CStrings and 1
is responsable w/ freeing them (to simulate the 'delayed'
deallocation).
This is the code:
The thread that allocates the CStrings is created in OnInitDialog as
follows:
m_pWorkThread = AfxBeginThread((AFX_THREADPROC)WorkerThread, this,
THREAD_PRIORIT=
Y_NORMAL, 0, CREATE_SUSPENDED);
m_pWorkThread->m_bAutoDelete = FALSE;
m_pWorkThread->ResumeThread();
Here is the delete thread creation:
m_pDelWorkThread = AfxBeginThread((AFX_THREADPROC)DelWorkerThread,
this,
THREAD_PR=
IORITY_NORMAL, 0, CREATE_SUSPENDED);
m_pDelWorkThread->m_bAutoDelete = FALSE;
m_pDelWorkThread->ResumeThread();
You have copy-paste code there (thread creation is same, bar thread
func).
The worker thread is implemented as follows:
DWORD CTestTDlg::WorkerThread(LPVOID lpVoid)
{
CTestTDlg* pThis = (CTestTDlg*)lpVoid;
pThis->DoWork();
return 0;
}
void CTestTDlg::DoWork()
{
HANDLE objects[2];
objects[0] = m_hQuitEvent;
objects[1] = m_hProcessEvent;
m_bWorkThreadRunning = TRUE;
while (m_bWorkThreadRunning)
{
switch (WaitForMultipleObjects(2, objects=
, FALSE, INFINITE))
{
case WAIT_OBJECT_0:
m_bWorkThreadRunning = =
FALSE;
break;
case WAIT_OBJECT_0+1:
Process();
break;
}
}
}
You don't need m_bWorkThreadRunning. You should use m_hQuitEvent for
that. When you set it in the main thread, WaitFor... will get out with
WAIT_OBJECT_0, at which point you know that you should stop.
Process is implemented as this:
void CTestTDlg::Process()
{
for (int i = 0; i < 15000 && m_bWorkThreadRunning; ++i)
{
Sleep(0); // I know it is not a good idea=
to use sleep in multi-
threaded code.
// However, "Slee=
p(0)" should "yield" to
other threads since all the thread priorities are the same.
// Is this correc=
t ?
Yes, but you don't achieve anything concrete with this sleep.
If you put it after PostMessage, you could have __hoped__ that the
thread being awaken is you UI thread, but that's all.
CString* pString = new CString;
PostMessage(UserMsg_Add, (WPARAM)pString,=
0);
}
}
Here is the handler of the UserMsg_Add:
LRESULT CTestTDlg::OnAdd(WPARAM wParam, LPARAM lParam)
{
CString* pStr = (CString*)(wParam);
int iUpperBound = m_Array.GetUpperBound();
m_Array.SetAtGrow(++iUpperBound, pStr); // m_Array is def=
ined as:
CArray<CString*, CString*> m_Array;
return 0;
}
Use Add instead of SetAtGrow. SetAtGrow is used when you want to add
long after current container end, and fill the remainder with default
values for the type being held by the container. also, do this for
exception safety:
std::auto_ptr<CString> P((CString*)(wParam));
array.Add(P.get());
P.release();
When the user presses the "Start" button, the following code is
called:
SetEvent(m_hProcessEvent);
m_hProcessEvent is an auto-reset event created in the constructor of
the dialog this way:
m_hProcessEvent = CreateEvent(NULL, FALSE, FALSE, NULL);
Therefore, at this point, the for loop in the allocating worker thread
is running.
Now, if the user presses the Exit button this is what happens:
m_bQuit = TRUE; // This is a little bit suspect. You will see how I
use this later.
m_bWorkThreadRunning = FALSE;
SetEvent(m_hDelProcessEvent); // This is an auto-reset event that
tells the deallocation thread to perform the deallocations:
This is very bad. Your work (producer) thread is still running, and
you fired off deletion (consumer) thread. So your producer might post
messages to your UI, UI might try to who will add your strings to the
array, and at the same time, your consumer might try to delete strings
from the array. This is a disaster waiting to happen, and you either
must insure that producer stops before firing off the consumer
(better), or synchronize access to your array (doable).
... and nothing else. No exit point here; just a flag set (m_bQuit) to
inform everyone that we want to eventually exit.
That's OK.
Here is the implementation of the de-allocation thread:
DWORD CTestTDlg::DelWorkerThread(LPVOID lpVoid)
{
CTestTDlg* pThis = (CTestTDlg*)lpVoid;
pThis->DelDoWork();
return 0;
}
void CTestTDlg::DelDoWork()
{
HANDLE objects[2];
objects[0] = m_hDelQuitEvent;
objects[1] = m_hDelProcessEvent;
m_bDelWorkThreadRunning = TRUE;
while (m_bDelWorkThreadRunning)
{
switch (WaitForMultipleObjects(2, objects=
, FALSE, INFINITE))
{
case WAIT_OBJECT_0:
m_bDelWorkThreadRunning =
= FALSE;
break;
case WAIT_OBJECT_0+1:
DelProcess();
break;
}
}
}
You don't need m_bDelWorkThreadRunning.
And here is the DelProcess function and the most important part of the
program:
void CTestTDlg::DelProcess()
{
int iUpperBound = m_Array.GetUpperBound();
for (int i = 0; i <= iUpperBound; ++i)
{
Sleep(0); // Sleep(0) -- yield; w/o it, w=
ouldn't this thread take
100% CPU while deleting ?
delete m_Array.GetAt(i); // deallocate al=
l the CStrings
}
m_Array.RemoveAll();
if (m_bQuit) // As you noted earlier, this was set by the=
"Exit"
button handler.
{
SetEvent(m_hQuitEvent); // This tells the=
allocating thread to Exit.
DWORD dwRet = WaitForSingleObject(m_pWo=
rkThread->m_hThread,
INFINITE); // WaitFor returns WAIT_OBJECT_0
if (dwRet == WAIT_OBJECT_0) delete m_=
pWorkThread; // statement
executed as dwRet is WAIT_OBJECT_0. confirmed w/ debug.
This makes no sense at all. You should Wait... and delete your work
thread in the UI. You could do it together with your delete thread (in
code below). By the time you reach that point, your worker should be
long gone. If it's coded so that this isn't the case, you should make
it post "I'm done" message just like you do for your "delete" thread
just below and act on that.
SetEvent(m_hDelQuitEvent); // I tell *thi=
s* thread to exit.
This makes no sense either. At this point, nobody waits on this event,
so there's no need to set it. Your UI thread should call this and you
should only wait on it.
PostMessage(UserMsg_Exit, 0, 0); // This =
is the last message; should
tell the dialog to be dismissed.
}
}
Here is the message handler of UserMsg_Exit:
LRESULT CTestTDlg::OnExit(WPARAM wParam, LPARAM lParam)
{
// I know, I shouldn't use WaitFor in the UI thread. H=
owever,
w/o it, wouldn't deleting the thread directly be involved in a race-
condition ?
DWORD dwRet = WaitForSingleObject(m_pDelWorkThread->=
m_hThread, iTimeout); // dwRet is set to WAIT_OBJECT_0 because the
thread exists normally.
Timeout is bad here. You will not reach it, and if you do, you have
bigger problems. Use INFINITE. Why? Because, at this point, you know
that your thread is exiting (it told you so through PostMessage), so
you know that you'll wait shortly. If, however, that doesn't happen,
IMO it's best to hang than cover problems up.
// I guess the proper solution would be to hav=
e this
thread as an autodelete thread ? This way, I don't have to
// wait for it and would be deallocated automa=
tiicaly.
No, m_bAutoDelete is bad almost always. The only situation where you
can use it is when you code your thread so that you somehow disconnect
it from the rest of your code (so that it can't touch program state
inadvertently any more), and then leave it to die out. But once you do
that, thread has no reason to run anyhow, so you might just as well
wait for it ensure it doesn't do harm.
m_bAutoDelete=TRUE is a MASSIVE design error. In fact, pretty much all
m_bAutoDelete flags in (exceptions, views, frames...) in MFC are
either plain and simple bugs, either could have been avoided with
minimal work.
// ---- This way, I would avoid a WaitFor in t=
he UI ----
You are mistaken. WaitForSingleObject is not a problem. Problem is
that you can't use it e.g. when you want to process messages from the
background, or when background will take a long time to finish while
you wait. None of that is the case here. WaitFor is, in fact, the only
correct design in situations like yours.
At any point, point to take away is: you normally need to post "I'm
done" message from the thread. When you receive it, use
WaitForSingleObject. If you are afraid that PostMessage will fail on
"I'm done", use a timer and GetExitCodeThread (but then, don't return
STILL_ACTIVE from thread proc, obviously).
Goran.