Re: MFC and Worker Thread issue

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)

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!