MFC and Worker Thread issue
DewPrism09
if user says cancel I want to wait till worker thread has completed one round
of job and that thread has exited. But It appears like once user says cancel,
OnDestroy is called followed by the destructor and the thread never seem to
have got a chance.
Here is the pseudo code, can you please help me point out the problem ?
Thanks a lot in advance.
CMyDlg * p_Dlg;
Class CMyDlg : public CDialog
{
public:
CMyDlg(CWnd* pParent = NULL); // standard constructor
~CMyDlg();
CWinThread* m_pMyWorkerThread;
HANDLE m_hEventStopMyWorkerThread;
HANDLE m_hEventMyWorkerThreadKilled;
BOOLEAN m_isDialogClosing;
protected:
static UINT WorkerFunction(LPVOID pParam);
virtual BOOL OnInitDialog();
afx_msg void OnDestroy();
void OnWorkButton();
}
CMyDlg::CMyDlg(CWnd* pParent /*=NULL*/)
: CDialog(CMyDlg::IDD, pParent)
{
// Use the following events for thread notification.
m_hEventStopMyWorkerThread = CreateEvent(NULL, FALSE, FALSE, NULL); //
auto reset, initially reset
m_hEventMyWorkerThreadKilled = CreateEvent(NULL, FALSE, FALSE, NULL); //
auto reset, initially reset
m_isDialogClosing = FALSE;
m_pMyWorkerThread = NULL;
p_Dlg = this;
}
CMyDlg::~CMyDlg()
{
DWORD dwExitCode;
if (m_pMyWorkerThread != NULL &&
GetExitCodeThread(m_pMyWorkerThread->m_hThread, &dwExitCode) &&
dwExitCode == STILL_ACTIVE)
{
// Set flag to indicate the dialog is closing. This is
// used to distinguish between stopping the worker thread due to
// closing the dialog versus worker thread actually completing.
m_isDialogClosing = TRUE;
SetEvent(m_hEventStopMyWorkerThread);
WaitForSingleObject(m_hEventMyWorkerThreadKilled, 3000); //waiting
for the worker thread to be killed.
}
CloseHandle(m_hEventStopMyWorkerThread);
CloseHandle(m_hEventMyWorkerThreadKilled);
}
void CMyDlg::OnDestroy()
{
CDialog::OnDestroy();
// wind down any worker thread in progress if user says cancel
SetEvent(m_hEventStopMyWorkerThread);
m_isDialogClosing = TRUE;
WaitForSingleObject(m_hEventMyWorkerThreadKilled, 3000);
}
void CMyDlg::OnWorkButton()
{
// Start Worker thread.
ResetEvent(m_hEventStopMyWorkerThread);
m_pMyWorkerThread = AfxBeginThread(CMyDlg::WorkerFunction, this);
}
UINT CMyDlg::WorkerFunction(LPVOID pParam)
{
CMyDlg* p_Dlg = static_cast<CMyDlg*>(pParam);
if (p_Dlg->m_isDialogClosing)
{
SetEvent(p_Dlg->m_hEventMyWorkerThreadKilled);
return 0;
}
p_Dlg->DoSomeHeavyJob();
WaitForSingleObject(p_Dlg->m_hEventStopMyWorkerThread, 30000);
if (! p_Dlg->m_isDialogClosing)
p_Dlg->DoSomeHeavyJob();
else
SetEvent(p_Dlg->m_hEventMyWorkerThreadKilled);
p_Dlg->m_pMyWorkerThread = NULL;
return(0);
}
DewPrism09
that DoSomeHeavyJob() is a blocking call and is taking much more time than
WaitForSingleObject() timeouts of 3000 as set in OnDestroy and destructor
added together.
Now the problem is simply how to avoid that much time. Is it possible to let
the MFC dialog off from UI or make it invisible while I terminate the worker
thread gracefully ?
Joseph M. Newcomer
do you mean "I closed the dialog"? Or "It is a modal dialog and I hit the cancel button"?
What?
The correct solution is to add code of the following
CMyDlg::CMyDlg()
{
threadrunning = FALSE;
stoptype = 0;
stopthread = FALSE;
}
void CMyDlg::OnCancel()
{
if(threadrunning)
{
stopthread = TRUE;
stoptype = IDCANCEL;
return;
}
CDlg::OnCancel();
}
void CMyDlg::OnOK()
{
if(threadrunning)
{
stopthread = TRUE;
stoptype = IDOK;
return;
}
CDlg::OnOK();
}
/* static */ UINT CMyDlg::threadfunc(LPVOID p)
{
...deal with p, which must contain or have reference to a CMyDlg* object
CMyDlg * dlg = ...extract it in some way from p
while(!dlg->stopthread)
{
...do thread thing
}
dlg->PostMessage(UWM_THREAD_STOPPED);
return 0;
}
UWM_THREAD_STOPPED is a user-defined message
Add
ON_MESSAGE(UWM_THREAD_STOPPED, &CMyDlg::OnThreadStopped)
or
ON_REGISTERED_MESSAGE(...same thing...);
LRESULT CMyDlg::OnThreadStopped(WPARAM, LPARAM)
{
threadrunning = FALSE;
switch(stoptype)
{
case IDOK:
OnOK();
break;
case IDCANCEL:
OnCancel();
break;
default:
break;
}
No events, no timeouts, no waiting, no problem.
joe
On Fri, 4 Sep 2009 04:01:01 -0700, DewPrism09 <DewPr...@discussions.microsoft.com>
wrote:
>I have a worker thread to do some heavy background job. From the frontend GUI
>if user says cancel I want to wait till worker thread has completed one round
>of job and that thread has exited. But It appears like once user says cancel,
>OnDestroy is called followed by the destructor and the thread never seem to
>have got a chance.
>Here is the pseudo code, can you please help me point out the problem ?
>Thanks a lot in advance.
>
>
>CMyDlg * p_Dlg;
****
Is this really a global variable? That is almost certainly an error. It doesn't need to
be, and shouldn't be
****
>
>Class CMyDlg : public CDialog
>{
>
>public:
> CMyDlg(CWnd* pParent = NULL); // standard constructor
> ~CMyDlg();
>
> CWinThread* m_pMyWorkerThread;
> HANDLE m_hEventStopMyWorkerThread;
> HANDLE m_hEventMyWorkerThreadKilled;
> BOOLEAN m_isDialogClosing;
>
>protected:
> static UINT WorkerFunction(LPVOID pParam);
> virtual BOOL OnInitDialog();
> afx_msg void OnDestroy();
> void OnWorkButton();
>}
>
>
>CMyDlg::CMyDlg(CWnd* pParent /*=NULL*/)
> : CDialog(CMyDlg::IDD, pParent)
>{
> // Use the following events for thread notification.
> m_hEventStopMyWorkerThread = CreateEvent(NULL, FALSE, FALSE, NULL); //
>auto reset, initially reset
> m_hEventMyWorkerThreadKilled = CreateEvent(NULL, FALSE, FALSE, NULL); //
>auto reset, initially reset
****
Get rid of both of these events. They are not needed and are erroneous design.
****
> m_isDialogClosing = FALSE;
> m_pMyWorkerThread = NULL;
> p_Dlg = this;
>}
>
>CMyDlg::~CMyDlg()
>{
****
By the time you get here it is FAR too late to stop the thread. Get rid of all this code.
It CANNOT be in the destructor. Completely utterly wrong.
****
> DWORD dwExitCode;
>
> if (m_pMyWorkerThread != NULL &&
> GetExitCodeThread(m_pMyWorkerThread->m_hThread, &dwExitCode) &&
> dwExitCode == STILL_ACTIVE)
> {
> // Set flag to indicate the dialog is closing. This is
> // used to distinguish between stopping the worker thread due to
> // closing the dialog versus worker thread actually completing.
>
> m_isDialogClosing = TRUE;
> SetEvent(m_hEventStopMyWorkerThread);
> WaitForSingleObject(m_hEventMyWorkerThreadKilled, 3000); //waiting
>for the worker thread to be killed.
> }
>
> CloseHandle(m_hEventStopMyWorkerThread);
> CloseHandle(m_hEventMyWorkerThreadKilled);
>}
>
>
>
>void CMyDlg::OnDestroy()
>{
> CDialog::OnDestroy();
>
> // wind down any worker thread in progress if user says cancel
> SetEvent(m_hEventStopMyWorkerThread);
> m_isDialogClosing = TRUE;
> WaitForSingleObject(m_hEventMyWorkerThreadKilled, 3000);
****
All this code is wrong. By the time you get here, it is FAR too late to kill the thread.
Get rid of all this code.
****
>}
>
>
>void CMyDlg::OnWorkButton()
>{
> // Start Worker thread.
> ResetEvent(m_hEventStopMyWorkerThread);
> m_pMyWorkerThread = AfxBeginThread(CMyDlg::WorkerFunction, this);
>}
>
>
>
>UINT CMyDlg::WorkerFunction(LPVOID pParam)
>{
> CMyDlg* p_Dlg = static_cast<CMyDlg*>(pParam);
>
> if (p_Dlg->m_isDialogClosing)
> {
> SetEvent(p_Dlg->m_hEventMyWorkerThreadKilled);
> return 0;
> }
****
Completely wrong. Do not set an event here. All of this is code that deals with events
is just flat-out wrong. The timeouts are a poor choice.
****
>
> p_Dlg->DoSomeHeavyJob();
>
> WaitForSingleObject(p_Dlg->m_hEventStopMyWorkerThread, 30000);
>
> if (! p_Dlg->m_isDialogClosing)
> p_Dlg->DoSomeHeavyJob();
> else
> SetEvent(p_Dlg->m_hEventMyWorkerThreadKilled);
>
> p_Dlg->m_pMyWorkerThread = NULL;
> return(0);
>}
Joseph M. Newcomer [MVP]
email: newc...@flounder.com
Web: http://www.flounder.com
MVP Tips: http://www.flounder.com/mvp_tips.htm
David Ching
news:E3E8D185-5ACF-4AA3...@microsoft.com...
You could hide the window when Cancel is pressed, wait for your heavy job to
finish, then destroy the dialog. Outlook does something similar, but it has
the disadvantage that if you immediately run it again while it is hidden
(but not yet destroyed), it wedges itself and goes into a weird state. On
my machine, the new instance doesn't show any new downloaded messages. I
have to go into Task Manager and terminate the first instance.
So I don't much like programs that are still running even though the UI has
gone away when the user has selected Exit. But it could work like this.
I would just do something simple like immediately disable the Cancel button
after it is clicked, but leave the dialog still showing until the worker
thread has finished.
-- David
Goran
0. read Doug H. essay on how it's best to use AfxBeginThread
(CREATE_SUSPENDED and m_bAutoDelete=false are essential); recent
Sutter's series on concurrent programming in DDJ ain't bad, either
(dare I say, everything he says is pure gold for proper C++ work?)
1. Lack of error-return checking (not relevant for a demo, but
essential for correct operation in production)
2. you don't need any events. You should simply wait on a thread
handle: signal to thread that it should finish through a boolean flag.
It's best to use proper test-and-set functions for said flag
(InterlockedCompareExchange etc), but is not essential, since your
flag only changes once and so CPU cache of your thread will eventually
pick it up.
3. You don't need p_Dlg, either. LPVOID pParam is enough.
4. In general, you cannot wait (WaitForSingleObject) arbitrary limited
time and expect things to work. That requires that you know up front
how long will your thread run between two reads of your "please stop"
flag. Instead, design so that you can use INFINITE.
5. you have some logic problems in your code, too, but since it's
pretty bad anyhow, let's not dwell on that much ;-). One example: if
you enter
if (! p_Dlg->m_isDialogClosing)
p_Dlg->DoSomeHeavyJob();
you will never set "killed" event, and that's what you wait on in
OnDestroy.
Here's a correct, albeit simplified to the maximum, solution:
Class CMyDlg : public CDialog
{
public:
CMyDlg(CWnd* pParent = NULL); // standard constructor
~CMyDlg();
CWinThread* m_pMyWorkerThread;
BOOELAN m_isDialogClosing; // BTW, why not C++ bool?
static UINT WorkerFunction(LPVOID pParam);
}
CMyDlg::CMyDlg(CWnd* pParent /*=NULL*/)
: CDialog(CMyDlg::IDD, pParent)
{
m_isDialogClosing = false;
// Add error checking (AfxBeginThread may return NULL).
// It may also throw - braindead MFC!
m_pMyWorkerThread = AfxBeginThread(CMyDlg::WorkerFunction, this,
THREAD_PRIORITY_NORMAL,
0,
CREATE_SUSPENDED);
m_pMyWorkerThread->m_bAutoDelete = false;
VERIFY(m_pMyWorkerThread->ResumeThread() == 1); // 1, not true.
See doc ;-)
}
CMyDlg::~CMyDlg()
{
m_isDialogClosing = true;
// Add error checking. Yes, you can WFSO with a thread handle
// Note: *m_pMyWorkerThread gives out thread handle.
// m_pMyWorkerThread->m_hThread may be cleaner.
WaitForSingleObject(*m_pMyWorkerThread, INFINITE);
delete m_pWorkerThread;
}
UINT CMyDlg::WorkerFunction(LPVOID pParam)
{
reinterpret_cast_cast<CMyDlg*>(pParam)->DoSomeHeavyJob();
// BTW, can one compile static_cast up here?
return 0;
}
Now, with correct error checking, this works, but has poor user
experience. (Your solution suffers from the same problem, except it
wrongly tries to alleviate that through 3 sec wait). If thread takes a
lot of time to finish, your UI is blocked for a long time. To improve
on this, dialog could set m_isDialogClosing, set some UI element to
the likes of "terminating, please wait", disallow closing itself, and
then wait on a thread. It should not use WFSO, but instead, thread
could inform the dialog when exiting. This is IMO best done with a
PostMessage in WM_APP range. For double protection (e.g. what if for
some crazy reason PostMessage from the thread fails?), dialog could
also set a timer and use GetExitCodeThread to detect thread
termination.
Other remarks: dialog's and thread's lifetime are too tightly bound.
Care must be taken so that the dialog instance does not get destructed
while thread is still running. That's easy to do, but again, perhaps
not the best user interface: user can't dismiss the dialog while
thread is running. So it could be that using dialog pointer in the
thread routine does not work.
What could be done to improve that is to pass data that is going to be
private to a thread and either not used concurrently, or be properly
synchronized. Thread must operate on that data only, so that the
dialog can go away and abandon the thread. But that requires more
work. "Data" must be defined. In it, there will probably be a pointer
to a dialog, so that e.g. the thread can inform on progress, or detect
that he himself should cancel if needed (good trick: ^^^^^^). Also, in
the current design, dialog is the sole owner of the thread. That must
change, so that some other facility in the code can take the thread
over and eventually clean it up^^^. Clearly, such design opens up to a
better flexibility, but also adds more complexity. If you are happy
with the dialog-thread pair, go for it, no problem.
And finally, one architectural nitpick: use of dialog class (CMyDlg)
in the thread proc is suboptimal design. If there is a candidate for
interface segregation principle (http://www.objectmentor.com/resources/
articles/isp.pdf), it's this situation here. Extract what thread needs
to know about it's "environment" and let it see only that ;-). Runtime
price to pay is very small (e.g. virtual call or checking for NULL
HWND instead of relying on OnDestroy and DoWork button), but coupling
is less tight, as well as compile-time dependencies, my favorite hate-
subject ;-).
HTH,
Goran.
^^^Don't ever fall in the trap of leaving CWinThread::m_bAutoDelete to
true. Ghosts of dead MFC programmers will haunt you forever ;-)
^^^^^^shared_ptr for said data in main thread, weak_ptr and short-
lasting use of weak_ptr::lock() or shared_ptr(const weak_ptr&) for
data in the thread, FTW!
Alexander Grigoriev
invalid.
For clean exit, you should reset m_bAutoDelete flag the AfxThread (start the
thread suspended, reset the flag, resume the thread). on't use
m_hEventMyWorkerThreadKilled. After you told the thread to stop, wait on
m_pMyWorkerThread->hThreadHandle by WaitForSingleObject. Remember, that your
worker thread should NEVER call SendMessage.
"DewPrism09" <DewPr...@discussions.microsoft.com> wrote in message
news:627026C9-0B3C-49BF...@microsoft.com...
DewPrism09
logic needs some tweak for practical problems, but nevethless an out-of-box
solution to boot.
DewPrism09
pseudo-code where I omitted more than neccesary. Sorry about that. Thing
remains, your points are helpful to write better programs, always.
Joseph M. Newcomer
environments. The key idea here is that you don't "wait" for a thread to finish, and you
don't block the UI (although you may disable controls on it). You always wait for
asynchronous notifications. We evolved this idea out of discussions on multithreading in
the 1970s, and I've pretty much used it everywhere since. A friend of mine built an
entire TCP/IP networking package out of this architecture, and is probably the person most
responsible for the core ideas. The generalization of this is the DFSM, the Distributed
Finite State Machine, where the state transitions might be done by any of several threads,
but only one thread at a time owns the "state" and is allowed to manipulate it. If you
stand back from the code I showed, about 10', and squint, you realize this is a simple
reduction of the general DFSM model. You can then add to it as you need to deal with more
complex problems with more complex state.
It is based on some articles on threading on my Web site, but this code is
dialog-specific.
joe
On Fri, 4 Sep 2009 09:08:13 -0700, DewPrism09 <DewPr...@discussions.microsoft.com>
wrote:
>Thank You Joe for taking time and posting such an elegant fix. Of course the
>logic needs some tweak for practical problems, but nevethless an out-of-box
>solution to boot.
Joseph M. Newcomer
>Some problems I see:
>
>0. read Doug H. essay on how it's best to use AfxBeginThread
>(CREATE_SUSPENDED and m_bAutoDelete=false are essential); recent
>Sutter's series on concurrent programming in DDJ ain't bad, either
>(dare I say, everything he says is pure gold for proper C++ work?)
>
>1. Lack of error-return checking (not relevant for a demo, but
>essential for correct operation in production)
****
I usually explicitly say in examples "error detection and response are left as an Exercise
For The Reader", or if I'm posting a question, I will say "simplified by eliminating
obvious code for error response" or writing
BOOL b = SomeAPI(...);
if(!b)
...error recovery
...but although SomeAPI worked, the values are wrong here...
****
>
>2. you don't need any events. You should simply wait on a thread
>handle: signal to thread that it should finish through a boolean flag.
>It's best to use proper test-and-set functions for said flag
>(InterlockedCompareExchange etc), but is not essential, since your
>flag only changes once and so CPU cache of your thread will eventually
>pick it up.
****
Actually, you don't need interlocked operations as you describe if your value is
"monotonic", that is, if your "Let the thread run" boolean can ONLY go from TRUE to FALSE
(or your thread-should-stop boolean can only go from FALSE to TRUE) then no complex
interlocked operations are required at all. They are *essential* for non-monotonic
changes, but unnecessary overkill for monotonic changes. The hardware will handle
monotonic changes correctly. Note that there is no concept of the CPU cache "eventually"
picking it up; it will be seen IMMEDIATELY after the value changes (go read about the
cache consistency algorithms implemented in caching hardware to make sure multiprocessor
caches remain coherent images of data; simplistically, bus snooping and global
cache-abort-memory-cycle lines are sufficient. I could [and in one of my courses, do]
devote a 30-minute, multipage discussion to all of the aspects of this, but be assured: if
CPUn changes the value, CPUm for m!=n will get the correct value EVEN IF IT READS THE
VALUE IN THE VERY NEXT MEMORY CYCLE, said cycle having been delayed because both CPUn and
CPUm were trying to change the value at the same time! As soon as the L2 cache sees the
value, cache consistency is guaranteed, and L1 is write-through. And also, there is no
difference between an error of 10ns and an error of 100ms in the race to stop a thread, so
if it missed the test on the current loop iteration because the looping thread in CPUm got
to read it before CPUn could set it, both trying for the same time, the error is harmless.
But it is *not* a caching issue).
The real reason you don't want events is because you don't want to block the GUI thread
****
>
>3. You don't need p_Dlg, either. LPVOID pParam is enough.
****
Absolutely, I agree completely. There is a common failure in threading that people think
that they have to use global variables because the thread function is a global function.
It can be a static class method, which is actually the best way, and you never need global
variables. That's what that LPVOID parameter is all about!
****
>
>4. In general, you cannot wait (WaitForSingleObject) arbitrary limited
>time and expect things to work. That requires that you know up front
>how long will your thread run between two reads of your "please stop"
>flag. Instead, design so that you can use INFINITE.
*****
What I tell my students is "If you use a timeout, you have to have a plan. And the plan
has to say what to do when the timeout happens, and your program has to remain CORRECT if
the timeout occurs. Now if you look at what you did here..." and go on to show how,
under certain timeout conditions, their program will roll over, kick up its heels, and die
horribly, usually with an access fault, but often with other more subtle and sometimes
more serious errors (how can an error be "more serious" than an access fault? When in
means the program does not die, but continues to execute but produces incorrect results!)
****
>
>5. you have some logic problems in your code, too, but since it's
>pretty bad anyhow, let's not dwell on that much ;-). One example: if
>you enter
>
> if (! p_Dlg->m_isDialogClosing)
> p_Dlg->DoSomeHeavyJob();
****
I ignored this glaring error because pretty much everything else was wrong, also. We used
to title this code "Day at the races" and post it on our "Day at the races" Wall Of Shame
in the computer room...
****
>
>you will never set "killed" event, and that's what you wait on in
>OnDestroy.
>
>Here's a correct, albeit simplified to the maximum, solution:
>
>Class CMyDlg : public CDialog
>{
>public:
> CMyDlg(CWnd* pParent = NULL); // standard constructor
> ~CMyDlg();
>
> CWinThread* m_pMyWorkerThread;
> BOOELAN m_isDialogClosing; // BTW, why not C++ bool?
****
What's a BOOLEAN? BOOL, yes, and I often say "there is a difference, and if you get it
wrong you can get into trouble; the simplest way to avoid these problems when dealing with
MFC and the API is stick with the quaint but robust-in-context BOOL"
****
> static UINT WorkerFunction(LPVOID pParam);
>}
>
>CMyDlg::CMyDlg(CWnd* pParent /*=NULL*/)
> : CDialog(CMyDlg::IDD, pParent)
>{
> m_isDialogClosing = false;
> // Add error checking (AfxBeginThread may return NULL).
> // It may also throw - braindead MFC!
****
You really can't start threads in the constructor! Why? Because at the point of the
constructor, THERE IS NO HWND ASSOCIATED WITH THE CWnd! So if the thread starts tossing
PostMessage messages at the window, There Is No There There. The messages will be lost,
and you have potential storage leaks. Any threading must be started in OnInitDialog, when
you know you have an actual HWND wrapped by the CWnd. Doing it in the constructor *might*
work, but it is very, very fragile and the first time the programmer adds a PostMessage,
the code becomes seriously broken.
****
> m_pMyWorkerThread = AfxBeginThread(CMyDlg::WorkerFunction, this,
> THREAD_PRIORITY_NORMAL,
> 0,
> CREATE_SUSPENDED);
>
> m_pMyWorkerThread->m_bAutoDelete = false;
> VERIFY(m_pMyWorkerThread->ResumeThread() == 1); // 1, not true.
****
Note that even if you create the suspended thread in the constructor, you must not allow
it to run until OnInitDialog. So I don't see any advantage to just moving the thread
creation and resumption to OnInitDialog
****
>See doc ;-)
****
It is surprising how much code I find that thinks this is a BOOL value!
****
>}
>
>CMyDlg::~CMyDlg()
>{
> m_isDialogClosing = true;
>
> // Add error checking. Yes, you can WFSO with a thread handle
> // Note: *m_pMyWorkerThread gives out thread handle.
> // m_pMyWorkerThread->m_hThread may be cleaner.
> WaitForSingleObject(*m_pMyWorkerThread, INFINITE);
> delete m_pWorkerThread;
*****
Just like starting the thread, this is FAR TOO LATE to kill the thread. The thread has
been running. It might be doing PostMessage calls targeting the dialog window. The
dialog window is LONG GONE before this code is executed, so there has been a period of
time in which the thread might think it has a valid target, but in fact it does not.
Consider
void CMySomething::CreateWorkerDialog()
{
CMyDlg dlg;
// At this point the thread is running!
...stuff is being set up
// note that the dialog values may not yet be right
// and there is no HWND!
if(dlg.DoModal() != IDOK)
return;
// thread is STILL RUNNING but there is no HWND!
// attempts to read values from dialog may get incorrect results
// because values may be changing in ways that are
// inconsistent
} // could hang for a long time if the thread shutdown doesn't work
// Note that if we hang at the close, other messages, including timer messages
// and WM_PAINT are not being processed, nor are any of the messages
// the thread may have been posting (assuming they weren't being posted
// to the dialog, but, for example, to the dialog's parent)
There are just too many things wrong here.
Once the thread has finished, and you receive the PostMessage that it has finished, you
know that there is practically nothing else it can do except return via the MFC library
code, so at that point:
LRESULT CMyDlg::OnThreadFinished(WPARAM, LPARAM)
{
switch(::WaitForSingleObject(thread->m_hThread, INFINITE))
{
case WAIT_OBJECT_0:
delete thread;
break;
default:
ASSERT(FALSE);
break;
}
...deal with other aspects of thread termination
return 0;
}
>}
>
>UINT CMyDlg::WorkerFunction(LPVOID pParam)
>{
> reinterpret_cast_cast<CMyDlg*>(pParam)->DoSomeHeavyJob();
> // BTW, can one compile static_cast up here?
> return 0;
>}
>
>Now, with correct error checking, this works, but has poor user
>experience. (Your solution suffers from the same problem, except it
>wrongly tries to alleviate that through 3 sec wait). If thread takes a
>lot of time to finish, your UI is blocked for a long time. To improve
>on this, dialog could set m_isDialogClosing, set some UI element to
>the likes of "terminating, please wait", disallow closing itself, and
>then wait on a thread. It should not use WFSO, but instead, thread
>could inform the dialog when exiting. This is IMO best done with a
>PostMessage in WM_APP range. For double protection (e.g. what if for
>some crazy reason PostMessage from the thread fails?), dialog could
>also set a timer and use GetExitCodeThread to detect thread
>termination.
****
Be cautions of WM_APP messages. If I use WM_APP + 52 to inform my client user of
something, and YOU use WM_APP + 52 to inform your client of something else, and the client
wants to use both of our subroutine packages, the client is well-and-truly royally
screwed. Yes, I've seen it happen. This is why I never use WM_APP messages for
ANYTHING, but only Registered Window Messages, and the registered name has a GUID (from
GUIDGEN) as part of the name; for example
UINT UWM_THREAD_FINISHED = ::RegisterWindowMessage(_T("UWM_THREAD_FINISHED") _T("GUIDGEN
GUID here"));
then
ON_REGISTERED_MESSAGE(UWM_THREAD_FINISHED, &CMyDlg::OnThreadFinished)
it avoids all these potential problems. I've not only seen this fail in multiprogrammer
projects, I've seen it fail in single-programmer projects where the programmer either lost
track of which WM_APP values had been used, or took some old, existing, robust code and
included it, not noticing the conflict of WM_APP messages.
****
>
>Other remarks: dialog's and thread's lifetime are too tightly bound.
>Care must be taken so that the dialog instance does not get destructed
>while thread is still running. That's easy to do, but again, perhaps
>not the best user interface: user can't dismiss the dialog while
>thread is running. So it could be that using dialog pointer in the
>thread routine does not work.
****
See my above comments. Even thinking that this might work leads the programmer down a
truly deadly path, one prone to catastrophic failure when the code is maintained by
Unskilled Labor [what I tell my students: "That means the New Hire. Or yourself, six
months later"]
****
>
>What could be done to improve that is to pass data that is going to be
>private to a thread and either not used concurrently, or be properly
>synchronized.
****
Synchronization Is Bad. By that, I mean that synchronization is the point of friction
where two threads rub, and like friction in a physical system, generates heat and wastes
energy. I've seen two threads that work in lockstep, no concurrency at all, because of
the need to synchronize everything. I just rewrote a massive library that had
synchronization EVERYWHERE and eliminated EVERY synchronization primitive; there are no
CRITICAL_SECTIONs anywhere in the new code. I did this by creating a new architecture in
which it is NEVER the case that two threads EVER, UNDER ANY CIRCUMSTANCES, touch the same
data concurrently. Data is owned either by the main thread or the worker thread, and
neither one can touch data the other owns.
You can't get away with this in all cases; there are some cases in which synchronization
absolutely *must* be done because the concurrent access is *required*. But the trick is
to create architectures in which the concurrent access is *not* required, and therefore
the synchronization becomes unnecessary. I have very few examples where synchronization
is mandatory (one of my lab exercises cannot be done without using a mutex, for example,
and is carefully constructed to prevent any other solution from working, but it is highly
artificial in how it constrains the programmer to this solution). Generally, you can
rework the architecture, using asynchronous interthread messaging, to remove any
dependency on the concept of locking-for-concurrent-access.
*****
>Thread must operate on that data only, so that the
>dialog can go away and abandon the thread. But that requires more
>work. "Data" must be defined. In it, there will probably be a pointer
>to a dialog, so that e.g. the thread can inform on progress, or detect
>that he himself should cancel if needed (good trick: ^^^^^^). Also, in
>the current design, dialog is the sole owner of the thread. That must
>change, so that some other facility in the code can take the thread
>over and eventually clean it up^^^.
****
Actually, this is unnecesary. Classic example: a modal progress dialog showing what the
thread is doing, with a "cancel" button on it. In that case, the dialog owns the thread
and is solely responsible for its cleanup. And the dialog simply cannot go away until the
thread is cleanly terminated.
*****
>Clearly, such design opens up to a
>better flexibility, but also adds more complexity. If you are happy
>with the dialog-thread pair, go for it, no problem.
>
>And finally, one architectural nitpick: use of dialog class (CMyDlg)
>in the thread proc is suboptimal design. If there is a candidate for
>interface segregation principle (http://www.objectmentor.com/resources/
>articles/isp.pdf), it's this situation here. Extract what thread needs
>to know about it's "environment" and let it see only that ;-). Runtime
>price to pay is very small (e.g. virtual call or checking for NULL
>HWND instead of relying on OnDestroy and DoWork button), but coupling
>is less tight, as well as compile-time dependencies, my favorite hate-
>subject ;-).
****
Usually, I don't use a CMyDlg* in a thread, but a generic CWnd*, since all I need is
PostMessage. So the PostMessage can post to any window, giving a nice separate between
the client (this week a CMyDlg, next week, in a different project, a CMyView, or in one
case I did recently, a CMyDocument, which uses a CMyMessageSink invisible window to
provide a message sink for the running thread, since I can't rely on the integrity of any
view while the thread is running. OnCloseDocument handles the clean shutdown issue.
joe
*****
>
>HTH,
>Goran.
>
>^^^Don't ever fall in the trap of leaving CWinThread::m_bAutoDelete to
>true. Ghosts of dead MFC programmers will haunt you forever ;-)
>
>^^^^^^shared_ptr for said data in main thread, weak_ptr and short-
>lasting use of weak_ptr::lock() or shared_ptr(const weak_ptr&) for
>data in the thread, FTW!
Goran
>You really can't start threads in the constructor! Why? Because at the point of the
>constructor, THERE IS NO HWND ASSOCIATED WITH THE CWnd!
Absolutely!
However, note that code does not show any use of HWND, in the original
code, either ;-) I honestly left OnInitDialog and OnWorkButton out
deliberately. BTW, there's more issues in my code, e.g. upon thread
creation failure, ctor must either throw or something convoluted must
be invented to enable caller to see failure.
Note also that later I speak about HWND use (when I spoke about
employing ISP). Here's what I think: code running in the thread should
not know about HWND at all. It's an implementation detail. It should
have a facility to inform UI about the progress (and termination). Let
that facility deal with HWND. Yes, this might be more complicated^,
but in a more abstract sense, I find the "independent" design better.
^e.g. code must set "thread done" flag if thread really finishes
before OnInitDialog. There, dialog inspects the flag and EndDialog
accordingly.
Same goes for the destructor versus OnDestroy, really.
But I'll tell you where I come from really: I have a facility that
spawns a thread and waits for it in the GUI, similar to what this post
wants. Since it's used for operations of varying duration, I wanted
not to even show the dialog if background is quick. So what I did was
to start the thread before the dialog instance even exists. I continue
with the modal dialog, but don't show it immediately. I check
"terminated" in OnInitDialog, too. I have aforementioned "interface"
that thread uses, which does never see PostMessage or HWND. Instead,
there are ::Progress and ::ThreadDone "callbacks". I wrapped all this
in one function, where I pass an "executable" object. From there I
built stuff up to passing a class instance and a member function to
execute in the background.
Of course, this is all because I built half a dozen dialog/thread
pairs and eventually got fed up ;-).
Goran.
P.S. I don't understand what you mean about WM_APP issues. I think
that works if I control the window I'm sending messages to? (That's my
usage).
Joseph M. Newcomer
On Sat, 5 Sep 2009 03:58:45 -0700 (PDT), Goran <goran...@gmail.com> wrote:
>On Sep 5, 3:28�am, Joseph M. Newcomer <newco...@flounder.com> wrote:
>>You really can't start threads in the constructor! Why? Because at the point of the
>>constructor, THERE IS NO HWND ASSOCIATED WITH THE CWnd!
>
>Absolutely!
>
>However, note that code does not show any use of HWND, in the original
>code, either ;-) I honestly left OnInitDialog and OnWorkButton out
>deliberately. BTW, there's more issues in my code, e.g. upon thread
>creation failure, ctor must either throw or something convoluted must
>be invented to enable caller to see failure.
****
Yes, but the thread shown by the OP simply said "do some heavy computation" with no
indication as to what that was or what it did; therefore, it is not safe to assume that
the absence of the window is a neutral condition
*****
>
>Note also that later I speak about HWND use (when I spoke about
>employing ISP). Here's what I think: code running in the thread should
>not know about HWND at all. It's an implementation detail. It should
>have a facility to inform UI about the progress (and termination). Let
>that facility deal with HWND. Yes, this might be more complicated^,
>but in a more abstract sense, I find the "independent" design better.
****
Why shouldn't a thread know about an HWND? It doesn't have to know about a specific C++
subclass of CWnd, but part of the abstract interface spec of the thread can safely be
stated as "from time to time the thread will notify a selected window, via PostMessage, of
events of interest, such as..." And that facility, which is running in the thread, must
be able to assume that the CWnd* actually has a valid HWND; otherwise, you get into
situations such as the thread has terminated before the window was created but has no way
to notify the "owner" that it has terminated. Requiring an HWND in a CWnd * is a valid
specification! Requiring a specific *type* of CWnd* is not. Yes, this means you can't
run the thread in a console app without some extra effort, but this is not a killer
limitation. The complexity of handling thread notifications if the window does not exist
is far more fragile, difficult to test, and likely to fail. Simple limitation: There is a
CWnd* which is the message sink and it is created before the thread (with its HWND value)
and will not be destroyed until after the thread finishes. I see nothing wrong with
building that requirement in as part of the thread specification.
****
>
>^e.g. code must set "thread done" flag if thread really finishes
>before OnInitDialog. There, dialog inspects the flag and EndDialog
>accordingly.
****
This adds gratuitous complexity to what should be a simple task
****
>
>Same goes for the destructor versus OnDestroy, really.
****
There are three interesting points of closing a window
OnClose/OnOK/OnCancel: the closing can be aborted or delayed if necessary
OnDestroy: The HWND exists but will not upon completion, and the operation cannot be
delayed without blocking the thread.
Destructor: Who knows what is valid? Nothing can be delayed without blocking the thread.
The destructor is the most fragile point, because it can introduce a surprising hang if
the thread has not finished. Furthermore, there is no capability of notifying the user
that the thread is not terminating, because the delay has to require blocking the main GUI
thread *invisibly* (that is, no explicit code on the part of the programmer) and therefore
no notifications can be posted.
Creating the thread in OnInitDialog guarantees that the HWND exists; delaying the
OnOK/OnCancel by simply deferring the operation until thread shutdown notification
guarnatees that the HWND exists beyond the lifetime of the thread. Because the thread
does a PostMessage that it has finished, other pending messages to the dialog can be
handled in FIFO order; the thread-finished message is guaranteed to be the last message
sent and therefore it is now permissible to utilize and/or destroy any thread-related
state (such as the CWinThread object). This maintains easily the invariant that the HWND
exists for the life of the thread.
****
>
>But I'll tell you where I come from really: I have a facility that
>spawns a thread and waits for it in the GUI, similar to what this post
>wants.
****
Spawning a thread and waiting for it is just a clumsy implementation of a subroutine call.
****
>Since it's used for operations of varying duration, I wanted
>not to even show the dialog if background is quick. So what I did was
>to start the thread before the dialog instance even exists. I continue
>with the modal dialog, but don't show it immediately. I check
>"terminated" in OnInitDialog, too. I have aforementioned "interface"
>that thread uses, which does never see PostMessage or HWND. Instead,
>there are ::Progress and ::ThreadDone "callbacks". I wrapped all this
>in one function, where I pass an "executable" object. From there I
>built stuff up to passing a class instance and a member function to
>execute in the background.
*****
Callbacks add their own problems; I try to avoid them if at all possible. I just finished
a project that uses callbacks, but that is because the entire library is written in pure
C. Otherwise, I would have done virtual methods. I like positive-acknowledgement
protocols where there are no possible "race" conditions. They are very insensitive to
accidents of timing.
joe
*****
>
>Of course, this is all because I built half a dozen dialog/thread
>pairs and eventually got fed up ;-).
>
>Goran.
>
>P.S. I don't understand what you mean about WM_APP issues. I think
>that works if I control the window I'm sending messages to? (That's my
>usage).
![Doug Harrison [MVP]'s profile photo](http://lh3.googleusercontent.com/a-/ALV-UjVvzhOQGbT9K0Re6Qg-OU4VGuHq0tAdTu01YlmIjUGm6t_o8g=s40-c)
Doug Harrison [MVP]
wrote:
>P.S. I don't understand what you mean about WM_APP issues. I think
>that works if I control the window I'm sending messages to? (That's my
>usage).
As Raymond Chen put, WM_USER can be used by whoever called RegisterClass,
and WM_APP can be used by whoever called CreateWindow. But you still need
to document custom message so that people who subclass your windows can use
WM_APP; MS kinda blew it with the listview control, which uses an
undocumented WM_APP+42 or thereabouts to help with in-place editing. It is
a bit more foolproof to use registered messages for everything.
--
Doug Harrison
Visual C++ MVP
Joseph M. Newcomer
same WM_APP-based message. I gave the example where two people each write a component,
and each assumes that WM_APP+n will be used to notify a target window. You are the target
window. How do you know which component sent the notification?
You don't have this problem with Registered Window Messages if you include a GUID in the
name.
joe
Doug Harrison [MVP]
<newc...@flounder.com> wrote:
>The problem with WM_APP deals with the conflicts that can arise if two components use the
>same WM_APP-based message. I gave the example where two people each write a component,
>and each assumes that WM_APP+n will be used to notify a target window. You are the target
>window. How do you know which component sent the notification?
But that directly violates the rule I gave about who gets to define WM_APP
messages, so you wouldn't use it that way. Doesn't mean it's useless,
though.
>You don't have this problem with Registered Window Messages if you include a GUID in the
>name.
Isn't WM_NOTIFY the generalization of the scenario you described?
Joseph M. Newcomer
On Sun, 06 Sep 2009 14:33:04 -0500, "Doug Harrison [MVP]" <d...@mvps.org> wrote:
>On Sat, 05 Sep 2009 21:30:20 -0400, Joseph M. Newcomer
><newc...@flounder.com> wrote:
>
>>The problem with WM_APP deals with the conflicts that can arise if two components use the
>>same WM_APP-based message. I gave the example where two people each write a component,
>>and each assumes that WM_APP+n will be used to notify a target window. You are the target
>>window. How do you know which component sent the notification?
>
>But that directly violates the rule I gave about who gets to define WM_APP
>messages, so you wouldn't use it that way. Doesn't mean it's useless,
>though.
****
Surprisingly almost everyone ignores this rule. Component writers define WM_APP messages
inside the DLLs. I've been done in even when my WM_APP messages were entirely within my
app, defined by my app, for my app's purposes (back before I realized that WM_APP is a
Really Bad Idea) by people doing broadcasts of WM_APP messages and nailing my app; and it
is typical that I see this in other people's apps. So I gave up using anything but
Registered Window Messages years ago. Never had a problem since.
****
>
>>You don't have this problem with Registered Window Messages if you include a GUID in the
>>name.
>
>Isn't WM_NOTIFY the generalization of the scenario you described?
****
Sort of. But this only really makes sense if the component is a child window of some
other window; it does not really work well when we are talking about a subroutine library,
a DLL, or some other component that is a non-window component.
joe
****
Goran
> Why shouldn't a thread know about an HWND? It doesn't have to know about a specific C++
> subclass of CWnd, but part of the abstract interface spec of the thread can safely be
> stated as "from time to time the thread will notify a selected window, via PostMessage, of
> events of interest, such as..." And that facility, which is running in the thread, must
> be able to assume that the CWnd* actually has a valid HWND; otherwise, you get into
> situations such as the thread has terminated before the window was created but has no way
> to notify the "owner" that it has terminated. Requiring an HWND in a CWnd * is a valid
> specification! Requiring a specific *type* of CWnd* is not. Yes, this means you can't
> run the thread in a console app without some extra effort, but this is not a killer
> limitation. The complexity of handling thread notifications if the window does not exist
> is far more fragile, difficult to test, and likely to fail. Simple limitation: There is a
> CWnd* which is the message sink and it is created before the thread (with its HWND value)
> and will not be destroyed until after the thread finishes. I see nothing wrong with
> building that requirement in as part of the thread specification.
> ****
Sure, and that's how I was doing it for a long time. It has, however,
always irked me that I have to pass some sort of the "thread context"
to dialog class and to OnInitDialog, so that there I can create and
start the thread. But OnInitDialog is quite far away from my starting
point. I "know" that it will be called, but... how do I know that? And
__why__ do I have to go that far just to start the thread? On top of
that, as I said up there, I might better avoid flicker of a dialog
being quickly shown if I let the thread run first. So I said, oh screw
that, just start the thread right away and make it work in the dialog.
So dialog only knows that there's a thread to wait on, and that it can
be called with "progress" and "terminated" functions from another
thread (and so it does PostMessage(WM_APP+X, ...) to itself).
Consequence of that inversion is that thread lifetime spans that of
the dialog, and is in the hands of a function that started the thread
(so dialog dtor/OnDestroy don't WFSO on the thread, but it's just the
same as they did).
You are of course right that not limiting thread lifetime to
OnInitDialog/Ondestroy is, ahem... gratuitous complexity. And I kinda
regret to have written my code up here with ctor/dtor. (I see that
now), giving impression to original poster of ctor/dtor being a
solution is a bad idea, as he seems poorly versed in the matter.
> >^e.g. code must set "thread done" flag...
>
> ****
> This adds gratuitous complexity to what should be a simple task
> ****
Yes (on second thought, there's no flag there, but rather a
GetExitCodeThread call).
>
> >Same goes for the destructor versus OnDestroy, really.
>
> ****
> There are three interesting points of closing a window
>
> OnClose/OnOK/OnCancel: the closing can be aborted or delayed if necessary
>
> OnDestroy: The HWND exists but will not upon completion, and the operation cannot be
> delayed without blocking the thread.
>
> Destructor: Who knows what is valid? Nothing can be delayed without blocking the thread.
>
> The destructor is the most fragile point, because it can introduce a surprising hang if
> the thread has not finished. Furthermore, there is no capability of notifying the user
> that the thread is not terminating, because the delay has to require blocking the main GUI
> thread *invisibly* (that is, no explicit code on the part of the programmer) and therefore
> no notifications can be posted.
True. To the defense of what I actually did: I do not finish the
dialog before thread exits. I disable closing it and wait for
"terminated" callback from the thread. That way, user sees that he's
waiting for "cleanup". Sadly, that has to be done either way if thread
and dialog lifetime are tied together. I really wish I could overcome
that, and one day I'll god damn put up some code to do it!
> Spawning a thread and waiting for it is just a clumsy implementation of a subroutine call.
Yes, of course, but you seem to have forgotten that the thread is
there because we want to keep the UI alive and inform user about the
progress ;-)
> ****>Since it's used for operations of varying duration, I wanted
> >not to even show the dialog if background is quick. So what I did was
> >to start the thread before the dialog instance even exists. I continue
> >with the modal dialog, but don't show it immediately. I check
> >"terminated" in OnInitDialog, too. I have aforementioned "interface"
> >that thread uses, which does never see PostMessage or HWND. Instead,
> >there are ::Progress and ::ThreadDone "callbacks". I wrapped all this
> >in one function, where I pass an "executable" object. From there I
> >built stuff up to passing a class instance and a member function to
> >execute in the background.
>
> *****
> Callbacks add their own problems; I try to avoid them if at all possible. I just finished
> a project that uses callbacks, but that is because the entire library is written in pure
> C. Otherwise, I would have done virtual methods. I like positive-acknowledgement
> protocols where there are no possible "race" conditions. They are very insensitive to
> accidents of timing.
Yes, callbacks are like that. I have two of them: "terminated" and
"progress". Again, to my defense, if HWND is alive, callbacks go to
PostMessage. If not, "progress" stores received params that
OnInitDialog picks up^^^. "terminated" is just lost (but
GetExitCodeThread picks thread termination up).
^^^"progress" has to store params because otherwise they have to go on
the heap; once there, they may never get delivered through
PostMessage, and leaked (regardless of HWND being alive for the
lifetime of the thread or not).
And here and now, I just spotted a flaw in what I did: non-stable
m_hWnd is accessed from another thread without proper synchronization.
The way it is done, thread code sees NULL at first, NON-NULL later and
doesn't get to see NULL again. It does not matter if I miss any
message, but what if my code sees bad non-NULL HWND? E.g. perhaps it
can see only 32 bits of it in a 64-bit build (not that we have 64)?
No, it's better that HWND is stable for the lifetime of the thread.
Crap... Seems like "gratuitous complexity", plus this issue, is reason
enough to go back to the old AfxBeginThread in OnInitDialog that I
dislike so much ;-).
Goran.
Joseph M. Newcomer
On Mon, 7 Sep 2009 01:20:39 -0700 (PDT), Goran <goran...@gmail.com> wrote:
>On Sep 5, 5:38�pm, Joseph M. Newcomer <newco...@flounder.com> wrote:
>> Why shouldn't a thread know about an HWND? �It doesn't have to know about a specific C++
>> subclass of CWnd, but part of the abstract interface spec of the thread can safely be
>> stated as "from time to time the thread will notify a selected window, via PostMessage, of
>> events of interest, such as..." �And that facility, which is running in the thread, must
>> be able to assume that the CWnd* actually has a valid HWND; otherwise, you get into
>> situations such as the thread has terminated before the window was created but has no way
>> to notify the "owner" that it has terminated. �Requiring an HWND in a CWnd * is a valid
>> specification! �Requiring a specific *type* of CWnd* is not. �Yes, this means you can't
>> run the thread in a console app without some extra effort, but this is not a killer
>> limitation. �The complexity of handling thread notifications if the window does not exist
>> is far more fragile, difficult to test, and likely to fail. �Simple limitation: There is a
>> CWnd* which is the message sink and it is created before the thread (with its HWND value)
>> and will not be destroyed until after the thread finishes. �I see nothing wrong with
>> building that requirement in as part of the thread specification.
>> ****
>
>Sure, and that's how I was doing it for a long time. It has, however,
>always irked me that I have to pass some sort of the "thread context"
>to dialog class and to OnInitDialog, so that there I can create and
>start the thread. But OnInitDialog is quite far away from my starting
>point. I "know" that it will be called, but... how do I know that?
****
Because it is specified that you will be called; it is not optional or negotiable!
****
>And
>__why__ do I have to go that far just to start the thread?
****
Because it isn't very far at all; it is just the right place. Why do you think this poses
any conceptual problem?
****
>On top of
>that, as I said up there, I might better avoid flicker of a dialog
>being quickly shown if I let the thread run first.
****
That's yet a different question. I would create a modeless dialog and delay the
ShowWindow of the dialog to hit on a WM_TIMER. Other messages could be handled in that
interval. In OnInitDialog, I would GetParent()->EnableWindow(FALSE) to simulate the modal
dialog. Solves the problem without the race condition of testing the thread-is-running
boolean.
****
>So I said, oh screw
>that, just start the thread right away and make it work in the dialog.
>So dialog only knows that there's a thread to wait on, and that it can
>be called with "progress" and "terminated" functions from another
>thread (and so it does PostMessage(WM_APP+X, ...) to itself).
>Consequence of that inversion is that thread lifetime spans that of
>the dialog, and is in the hands of a function that started the thread
>(so dialog dtor/OnDestroy don't WFSO on the thread, but it's just the
>same as they did).
>
>You are of course right that not limiting thread lifetime to
>OnInitDialog/Ondestroy is, ahem... gratuitous complexity. And I kinda
>regret to have written my code up here with ctor/dtor. (I see that
>now), giving impression to original poster of ctor/dtor being a
>solution is a bad idea, as he seems poorly versed in the matter.
>
>> >^e.g. code must set "thread done" flag...
>>
>> ****
>> This adds gratuitous complexity to what should be a simple task
>> ****
>
>Yes (on second thought, there's no flag there, but rather a
>GetExitCodeThread call).
****
Which is not actually reliable, because one of the valid exit codes is also the code that
indicates the thread is still running. If I were doing this, I would do a
WFSO(threadhandle, 0) which is robust and cannot be confused by GetExitCode values.
****
>
>>
>> >Same goes for the destructor versus OnDestroy, really.
>>
>> ****
>> There are three interesting points of closing a window
>>
>> OnClose/OnOK/OnCancel: the closing can be aborted or delayed if necessary
>>
>> OnDestroy: The HWND exists but will not upon completion, and the operation cannot be
>> delayed without blocking the thread.
>>
>> Destructor: Who knows what is valid? �Nothing can be delayed without blocking the thread.
>>
>> The destructor is the most fragile point, because it can introduce a surprising hang if
>> the thread has not finished. �Furthermore, there is no capability of notifying the user
>> that the thread is not terminating, because the delay has to require blocking the main GUI
>> thread *invisibly* (that is, no explicit code on the part of the programmer) and therefore
>> no notifications can be posted.
>
>True. To the defense of what I actually did: I do not finish the
>dialog before thread exits. I disable closing it and wait for
>"terminated" callback from the thread. That way, user sees that he's
>waiting for "cleanup". Sadly, that has to be done either way if thread
>and dialog lifetime are tied together. I really wish I could overcome
>that, and one day I'll god damn put up some code to do it!
>
>> Spawning a thread and waiting for it is just a clumsy implementation of a subroutine call.
>
>Yes, of course, but you seem to have forgotten that the thread is
>there because we want to keep the UI alive and inform user about the
>progress ;-)
****
But you said you were spawning the thread and waiting for it to complete; this is not the
same as saying you were spawning the thread and continuing to run while waiting for an
asynchronous completion notification.
****
>
>> ****>Since it's used for operations of varying duration, I wanted
>> >not to even show the dialog if background is quick. So what I did was
>> >to start the thread before the dialog instance even exists. I continue
>> >with the modal dialog, but don't show it immediately. I check
>> >"terminated" in OnInitDialog, too. I have aforementioned "interface"
>> >that thread uses, which does never see PostMessage or HWND. Instead,
>> >there are ::Progress and ::ThreadDone "callbacks". I wrapped all this
>> >in one function, where I pass an "executable" object. From there I
>> >built stuff up to passing a class instance and a member function to
>> >execute in the background.
>>
>> *****
>> Callbacks add their own problems; I try to avoid them if at all possible. �I just finished
>> a project that uses callbacks, but that is because the entire library is written in pure
>> C. �Otherwise, I would have done virtual methods. �I like positive-acknowledgement
>> protocols where there are no possible "race" conditions. �They are very insensitive to
>> accidents of timing.
>
>Yes, callbacks are like that. I have two of them: "terminated" and
>"progress". Again, to my defense, if HWND is alive, callbacks go to
>PostMessage. If not, "progress" stores received params that
>OnInitDialog picks up^^^. "terminated" is just lost (but
>GetExitCodeThread picks thread termination up).
****
There are still some race conditions, e.g.,
callback: main thread
PostMessage DestroyWindow
there is no possible test you can execute in the callback that tells you it is valid to
PostMessage. If you test a boolean that says the window exists, it tells you that the
window exists at time T, but the PostMessage occurs at time T+n for some time value n, and
therefore by the time the PostMessage occurs, the window is invalid. Simplistically
if(::IsWindow(wnd->m_hWnd))
wnd->PostMessage(...);
it can be that the sequence is
if(::IsWindow(wnd->m_hWnd))
**** Thread hits end of time slice****
DestroyWindow()
HWND rendered invalid
*** Thread hits end of time slice ***
wnd->PostMessage(...);
In a dual processor system, this is even worse. The two threads can interlace execution.
Essentially, there is no way a thread can test to see if the window is valid before
posting the message, which is why I avoid this particular model.
****
>
>^^^"progress" has to store params because otherwise they have to go on
>the heap; once there, they may never get delivered through
>PostMessage, and leaked (regardless of HWND being alive for the
>lifetime of the thread or not).
>
>And here and now, I just spotted a flaw in what I did: non-stable
>m_hWnd is accessed from another thread without proper synchronization.
>The way it is done, thread code sees NULL at first, NON-NULL later and
>doesn't get to see NULL again. It does not matter if I miss any
>message, but what if my code sees bad non-NULL HWND? E.g. perhaps it
>can see only 32 bits of it in a 64-bit build (not that we have 64)?
>No, it's better that HWND is stable for the lifetime of the thread.
>Crap... Seems like "gratuitous complexity", plus this issue, is reason
>enough to go back to the old AfxBeginThread in OnInitDialog that I
>dislike so much ;-).
****
If you PostMessage to an invalid handle, the message just disappears (actually,
PostMessage would return FALSE). If it had a pointer in it that had to be released by the
recipient, you have a storage leak.
It would be impossible to see only 32 bits of a 64-bit window handle, because in a 64-bit
build the DLL would be a 64-bit DLL.
joe
****
>
>Goran.
Doug Harrison [MVP]
<newc...@flounder.com> wrote:
>Surprisingly almost everyone ignores this rule. Component writers define WM_APP messages
>inside the DLLs. I've been done in even when my WM_APP messages were entirely within my
>app, defined by my app, for my app's purposes (back before I realized that WM_APP is a
>Really Bad Idea) by people doing broadcasts of WM_APP messages and nailing my app; and it
>is typical that I see this in other people's apps. So I gave up using anything but
>Registered Window Messages years ago. Never had a problem since.
I must be lucky; I've only heard of the problem, I've not experienced it.
Raymond Chen continues to use it in examples, and he makes a point of using
it:
http://blogs.msdn.com/oldnewthing/archive/2006/09/25/770536.aspx
Given that's he's written about errant broadcasts before:
http://blogs.msdn.com/oldnewthing/archive/2006/06/12/628193.aspx
This suggests he's either very stubborn, or broadcasting WM_APP is an
exceedingly rare problem. I thought I had read MS tightened up
HWND_BROADCAST a long time ago, but I can't find the reference now, and the
documentation doesn't really state this. However, I just ran this program
on Win7:
#include <windows.h>
#include <stdio.h>
int main()
{
SetLastError(0);
SendMessage(HWND_BROADCAST, WM_APP+11714, 0, 0);
printf("%d\n", GetLastError());
}
It prints 87, which is ERROR_INVALID_PARAMETER. In typical fashion, MSDN is
not clear that GetLastError is meaningful here, but that error value is an
awfully big coincidence if it isn't. I'd be interested to hear what this
does on other OSes.
>Sort of. But this only really makes sense if the component is a child window of some
>other window; it does not really work well when we are talking about a subroutine library,
>a DLL, or some other component that is a non-window component.
True, WM_NOTIFY requires the recipient to look at the ID of the control or
HWND of the sender to figure out what it means. OTOH, registered messages
live in a flattened space, where the message number completely identifies
the message, which is easier to deal with and extends to non-window
sources.
Joseph M. Newcomer
did HWND_BROADCAST of WM_APP names, and the worst was the one that used HWND_BROADCAST for
a WM_COPYDATA (hence my essay of putting a GUID at the front of a WM_COPYDATA!) In no
case was it an MS app that did me in, and in addition, I have been called in to help
people diagnose "malfunctioning code" when they have six or eight apps, nominally
cooperating, that use WM_APP-based messages for intercommunication.
More below...
****
Interesting. This suggests that HWND_BROADCAST will not accept other than Registered
Window Messages. I wonder if the new documentation will reflect this.
joe
****
> printf("%d\n", GetLastError());
>}
>
>It prints 87, which is ERROR_INVALID_PARAMETER. In typical fashion, MSDN is
>not clear that GetLastError is meaningful here, but that error value is an
>awfully big coincidence if it isn't. I'd be interested to hear what this
>does on other OSes.
>
>>Sort of. But this only really makes sense if the component is a child window of some
>>other window; it does not really work well when we are talking about a subroutine library,
>>a DLL, or some other component that is a non-window component.
>
>True, WM_NOTIFY requires the recipient to look at the ID of the control or
>HWND of the sender to figure out what it means. OTOH, registered messages
>live in a flattened space, where the message number completely identifies
>the message, which is easier to deal with and extends to non-window
>sources.
Doug Harrison [MVP]
<newc...@flounder.com> wrote:
>The problem is not MS; I was done in several times by locally-developed applications that
>did HWND_BROADCAST of WM_APP names, and the worst was the one that used HWND_BROADCAST for
>a WM_COPYDATA (hence my essay of putting a GUID at the front of a WM_COPYDATA!) In no
>case was it an MS app that did me in, and in addition, I have been called in to help
>people diagnose "malfunctioning code" when they have six or eight apps, nominally
>cooperating, that use WM_APP-based messages for intercommunication.
I understood you weren't saying that MS was sending the bogus
HWND_BROADCASTs. MS's failure was to ever allow broadcasting WM_COMMAND,
WM_USER, WM_APP, etc. There are probably just a handful of predefined
messages it makes sense to broadcast, and then there are registered
messages. They should have limited HWND_BROADCAST to those messages alone.
Maybe they do now. The SendMessage documentation doesn't quite say so, but
this blog entry suggests the issue was addressed back in Win95:
http://blogs.msdn.com/oldnewthing/archive/2004/05/05/126427.aspx
<q>
If you broadcast a message that was not safe to broadcast, Windows 95 would
send it only to old-style programs. New-style programs (marked as version
4.0 or higher) would not receive the messages.
</q>
If this means what I think it does, it's safe to use WM_USER and WM_APP as
they were intended to be used.