Never use AfxEndThread!

[Matt Schaller]

Hello,

Thanks for the new forum.

I thought I would put in a tip / warning that I have discovered and if anyone would like to comment on it, please do.

Recently I developed a program that allowed the user to select a file, and then a worker thread would do some processing to it. It was very simple, because it was my first MFC application and I was doing it more for learning MFC than being able to use the actual program.

While the worker thread processed the file, it periodically sent messages to a modal dialog box that had a progress control that reflected how much data had been processed by the worker thread. Also on the modal dialog box was a Cancel button that set a global variable to true that the worker thread checks at every iteration to see if it should end itself.

Here is a very slimmed down version of my thread:


typedef struct tagTHREADPARMS
{
CString pathname;
} THREADPARMS;

void CMyClass::SomeCallingFunction()
{
THREADPARMS *ptp = new THREADPARMS;
ptp->pathname = m_pathname; // m_pathname is in our class

AfxBeginThread( ThreadFunc, (LPVOID) ptp );
}

DWORD ThreadFunc(LPVOID lpParam)
{
THREADPARMS *ptp = (THREADPARMS*) lpParam;
CString pathname = ptp->pathname;
delete ptp;

//open file with the pathname

while( !file.eof() )
{
if( g_bStopThread )
{
// send ''thread canceled" msg to modal dialog box
AfxEndThread( 0 );
}
}

// send "thread finished" msg to modal dialog box
AfxEndThread( 0 );
}


If the thread runs its course, AfxEndThread( 0 ) is called and it finishes completely without any user intervention. The thread exits fine and the program (in debug mode) exits fine with no memory leaks.

However, if g_bStopThread is set to true, AfxEndThread( 0 ) is called, and the thread exits normally. However, when I close the program (when I am in debug mode) it shows a memory leak at a certain allocation number. When I use _CrtBreakAlloc to locate where the memory is being allocated, it points me to this line:


CString pathname = ptp->m_pathname;


This leak had me going quite crazy -- after all, it was a local variable, placed on the stack, and when the thread exits, everything on the stack should be removed... right? Apparently not. Unless AfxEndThread() is called at the very end of your function, local variables will not be automatically destroyed. Fortunately there was someone who was as interested in this problem as I was, and he was the one that ultimately had it figured out, and the fix to this problem is to just return 0; because that will take care of the removal of local variables.

I thought I would share this information with all of you...

Matt Schaller
matt@swfla.rr.com

[Myself dot NET]

I know this is not relevant to your point, and your code sample is slimmed down, but your access to the g_bStopThread variable by two threads simultaneously is not thread-safe unless using interlocked functions or surrounded by a critical section or synchronization object. Just thought that should be noted. But you probably just didn't include that because it's not the issue here.

[Matt Schaller]

I thought that since the setting of a variable that is less than 32 bits in length requires only one machine instruction (correct me if I am wrong on this), there is no problem with synchronization because accessing variables in two threads becomes a problem when the mode of access requires two or more machine instructions.

Thank you for pointing that out... but I am careful when it comes to non-32bit values, such as STL vectors, in which case I use a critical section.

Matt Schaller
matt@swfla.rr.com

[Myself dot NET]

You are probably correct, but it is not safe to assume that in general. If Windows is running on a different architecture, that might not be true. The general rule is to surround access to any (non-readonly) data by two or more threads by critical sections or synchronization objects, or use the interlocked functions.

Additional note: even for accessing 32-bit values, the compiler may generate code where it is not one instruction for accessing/setting the variable. For example, use of an intermediate register. If one thread is preempted before the value is updated in memory, you will have corrupted data.

[Shloim]

it's not the issue whether it's a 32bit variable or not
the fact is that it should be mutexed,
however, since the worker thread doesn't change the variable you can do something else:
the main application keeps the int
and gives the thread a const boolean *stopThread
there's no need in a mutex because:
a. there is only one writer
b. the mutual data cannot be in an "Illegal" state*




* such as if you'd had a char[100] used as a string and the writer could change the length of the actual string wich might prevent the thread from finding the string's null termination