We help IT Professionals succeed at work.

How do I (re-)enable heap memory allocation within a CWinThread-derived class thread function invoked directly from InitInstance?

RandyExEx
RandyExEx asked
on
This question has morphed into a "can you reproduce the symptom error" problem. - The first comment (from myself) below has a sample program attached, so if someone has a theory as to what may have happened, a quick modification to the sample program should reproduce it easily enough.

I have created a worker thread function wrapped by a CWinThread-derived object.(called ThreadProcObject) -

Within the thread routine (invoked directly from InitInstance - thus Run() and the message pump are not started up) an attempt to allocate some heap memory is made (see example program routines Run1() or Run2()) and, when the error occurs, _malloc_debug() is invoked within the runtime operator new() routine and returns NULL.

At least, this was the symptom in my orignal program, which I have yet to reproduce in the attached sample.

 I originally thought that what was happening is that in between the time the CWinThread-derived object's constructor is invoked and time the thread routine is called by InitInstance (note, before Run1() or Run2() is invoked, the thread goes to sleep first waiting on a "task op ready" event), and that some part of the (heap?) context changes, thus effectively disabling the operator new routine and that this could be resolved by calling some appropriate Afx...? routine to restore the heap context, but this appears to be wrong

An examination of the InitInstance() call to ThreadProc(), that routine's WaitForMultipleObjects-delayed calls to Run1() or Run2() and the memory allocation calls in Run1() or Run(2) should ring a bell if someone has any theories as to how _malloc_dbg() could return NULL if adequate heap memory exists.

I'll award the points if someone actually can reproduce the problem (see the comment below and its attached  program)  within the attached program  - and explain how it happened in the first place.
Comment
Watch Question

Author

Commented:
Turns out my assumption regarding heap context appears to be wrong. I attempted to reproduce the error with a somewhat simpler application (attached), and it wouldn't reproduce. I spent a weekend trying to correct my original program therefore and couldn't, and, in clearing out some the attempts I had made to correct it, accidentally did fix the error, but I didn't notice what I had cleared out to fix the error

In the attached program, the error in question would have occurred at the statement
    m_arrNodeInfo =
        (stNodeInfo(*)[NODES_ROWSET_SIZE]) new stNodeInfo[NODES_ROWSET_SIZE]; // cpp21
in ThreadProcObject::Run1() or at the statement
    m_arrInfo = new stInfo[500];
in in ThreadProcObject::Run2()

I thought it was because I had declared ThreadProc(), Run1(), Run2() static, but suing the conditional compilation (_DBGSTATIC) works too!?!

Actually, here, if somebody can reproduce the error (wherein the call to _malloc_dbg resulting from the allocation in Run1() or Run2() fails and as a result a memory exception is thrown), I'd award the points
(bonus ponts, if it were possible if anyone can tell why memory leak occurs in sample program when it closes down)

Attachments won't allow .vcproj, .rc, .rc2, .ico files in attachments (unbelievable!), so after attached file is expanded

Rename  cwinthreadtest.vbproj -> cwinthreadtest.vcproj,
cwinthreadtest.txt -> cwinthreadtest.rc,
res\cwinthreadtest.ini -. reg\cwinthreadtest.ico,
res\cwinthreadtest.reg -> reg\cwinthreadtest.rc2
cwinthreadtest.zip
Commented:
I think you have possibly experienced a rogue problem that could have been caused by any number of things, ranging from bad memory, fixed by a restart, to an incorrect link fixed by a rebuild.

I have been in these situations before; trying to reproduce a problem using a cut down version of production code.  I too have wasted days trying to no avail.  Then a rebuild of the original project fixes the issue.

Go back to your production code.  Rebuild and retry.  If the problem still occurs, then you really do have an issue that you yourself need to reproduce on a smaller scale.  Possible workarounds (and that is all they are) include allocating all memory that will possibly be needed in the thread's constructor, thereby in the main thread.  You could consider a placement new operator for this, if memory restrictions exist.

Now, I have downloaded your code and looked at it, and I feel you could improve your thread code.  Why have you added static "Run()" methods, passing "this" in as a parameter?  There is simply no need to do this!  C++ is object oriented therefore we can use this notion extensively...a thread is a perfect example of a self contained object.  I have posted some example code which I feel will help with this notion.  You do all of your work in the virtual Go() method, where you can make use of all member variable etc etc just as in any C++ object.

HTH
// .h

class CMyWorkerThread : public CWinThread
{
public:
	CMyWorkerThread();
	virtual ~CMyWorkerThread();
	virtual BOOL InitInstance();
	void Abort();

protected:

	virtual void Go();
	HANDLE m_hAbort;
};

// .cpp

CMyWorkerThread::CMyWorkerThread() 
{ 
	m_hAbort = ::CreateEvent(NULL, TRUE, FALSE, NULL);
}

BOOL CMyWorkerThread::InitInstance()
{
	Go();
	return FALSE;
}


void CMyWorkerThread::Abort()
{
	if (m_hThread)
	{
		::SetEvent(m_hAbort);
	}
}

/* virtual */ void CThreadSign::Go()
{
	while ( WaitForSingleObject ( m_hAbort, 0 ) != WAIT_OBJECT_0 ) 
	{
		// Do work, making use of member variables etc etc
	}	
}

Open in new window

Author

Commented:
Sorry, the original program was working so I crossed my fingers and have been going with it and took my eyes off the submitted question (so far, so good)

The conditional _DBGSTATIC was put in to see if I couldn't cause the error to happen by making the thread funtion static, which was one of the methods I had tried, primarily because the "normal" AfxBeginThread call "normally" references a global function (or at least one not contained as a class member), so in case the error was being caused by the function's being a class member, I made it static so its stack would be on the global heap as an experiment. I also tried declaring it as __cdecl and a few other experiments.

The "this" parameter to the static function actually is necessary in order to signal the appropriate auto-reset "thread completed" event member within the static function (remember, there's a thread object for each instance of the thread created) so that the main thread, which initiated each worker thread and is waiting for any of the threads to indicate completion, has the appropriate object's auto-reset "thread completed" event signalled for it. - Admittedly, making the function called directly by InitInstance static doesn't make a lot of sense, but I was trying to re-create an error.

An unusual feature of the original program is that I didn't think I could wait for task completion by waiting on the thread's handle  because the real program is accessing a SQL database over a network, and if either the SQL connection or the network connection fails, I want the wait in the main thread wait to time out and the cause a kill signal to be sent to the worker thread and the worker thread object eventually deletes itself (thus the use of the CWinThread-derived object and the FALSE return from InitInstance) - (Although, in afterthought, there may be no reason for not using the thread's handle anyway)

I like your while loop construction in your "Go" routine better than mine.

Author

Commented:
By the way, since you were able to reconstruct and build and presumedly run the attached sample, do you have any idea why, when it is terminated by clicking the 'x' button, the debugger indicates memory leaks?

(Also should have mentioned (to add to the confusion) that I had an older version of the original program restored when checking this problem out (old enough that I thought it was before the program began exhibiting the memory exception) and, truly bizarre, that copy failed, too - (No, I hadn't fooled around with the Visual Studio settings.)
Commented:

If the original program is working well, then that is good.  You could leave good enough alone, but I still think that you should change your thread logic to be more object oriented as I have mentioned.  As it stands, it is hard to pick up how things work from looking at your code: you have UI mixed with lower level functionality.  A thread class modularizes everything so much better and makes for easier reading, hence debugging if something should need looking at at a later date.
But hey, it is your project... :)
>> do you have any idea why, when it is terminated by clicking the 'x' button, the debugger indicates memory leaks?
I see the following after I start the debugger, press the start button, then close the app:
<code>
Detected memory leaks!
Dumping objects ->
{542} normal block at 0x00DB16A0, 16 bytes long.
&nbsp;Data: <&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; > A8 15 DB 00 CD CD CD CD CD CD CD CD CD CD CD CD
{541} normal block at 0x0003FBE8, 32 bytes long.
&nbsp;Data: <&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; > A0 16 DB 00 00 00 00 00 00 00 00 00 00 00 00 00
{526} normal block at 0x00DB3FA0, 28 bytes long.
&nbsp;Data: <&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; > 00 00 00 00 CD CD CD CD CD CD CD CD E8 FB 03 00
Object dump complete.
</code>
So I add the following to CWinThreadTestApp::InitInstance():
<code>
&nbsp;_CrtSetBreakAlloc ( 542 );
&nbsp;_CrtSetBreakAlloc ( 541 );
&nbsp;_CrtSetBreakAlloc ( 526 );
</code>
Now when the debugger is started, I immediately get a debug break on the line:
<code>
OpRequestQ = (queue<OpRequestObj*>*)
&nbsp;&nbsp;&nbsp;&nbsp;new queue<OpRequestObj*>;&nbsp;//[MAX_CONCURRENT_POLLS+1];
</code>
so, add
<code>
if ( OpRequestQ ) delete OpRequestQ;
</code>
to your destructor :)&nbsp; _CrtSetBreakAlloc really is a great function to make extensive use of when necessary :)

Commented:
Urg; sorry about the poor formatting above :)

Author

Commented:
Hmmm - I normally provide a loop pop-ing all elements off of stl queues or list structures and deleting the popped items in the destructor but I must have missed it here since I never 'new'ed the OpRequestQ queue. Thanks for spotting that

 And you are right, looking at your suggestion, it would seem to be more logical from an object-oriented viewpoint  to provide the Abort routine within the encapsulation of the (CWinThread-derived)  class (and addditionally does appear it would be easier to grasp and subsquently change from a debug standpoint)
Along those lines, an OpRequest routine, providing a signal that a task entry had been added to the OpRequestQ (and setting the associated event object there), could have been added perhaps as a static routine

Well, I guess we've beat this horse pretty much to death. Still, it strange I worked for several days without the memory exception occurring, then I couldn't get rid of it (and one of the steps I did take was cleaning out the obj files and re-building the project) .(more than once), and then afterwards couldn't reproduce in the simpler sample despite numerous attempts

So let's call it a solution and thanks again for your helpful comments and suggestions

Author

Commented:
Elusive problem probably caused by a procedural misstep on my part.

Commented:
Glad to help.  Again, if you do use a thread object as a member, you won't have any need for static functions in the thread at all...