Memory Leak within worker thread

[99bobster99]

I have a memory leak whenever this code is executed, any ideas on what I am doing wrong?

typedef struct ACC_MessageBox_Params
{
	int i;
	CString ACC_ErrorBuffer;

	ACC_MessageBox_Params(int Channel, CString AlarmMsg)
	{
		i = Channel;
		ACC_ErrorBuffer = (LPCTSTR)AlarmMsg;
	}
}
	PARAMS, *PPARAMS;


===============================================================
THREAD #1

AfxBeginThread(ACC_MessageBox, (PVOID) new ACC_MessageBox_Params(i, (LPCTSTR)ACC_ErrorBuffer));

===============================================================


UINT ACC_MessageBox(PVOID p)
{
// Worker Thread Pass-In Variables (Channel & Alarm Message) -> i(int), ACC_ErrorBuffer(CString)
PPARAMS pParams = (PPARAMS)p;

// Header Include Files

// Variable Declarations
int	response;

// User Defined Constants

// Set Abnormal (Exception Error Handling) 
set_terminate (myterminate);

// Multithread Synchronization (Single Lock Object)
CSingleLock	lock1(&GlobalMutex); // locks all SQL shared variables (RecordSet1) between threads
CSingleLock	lock2(&GlobalMutex); // locks all TimeStamp and OutputFile shared variables between threads

// Initialize

// Start of Main Code
	response = AfxMessageBox ((LPCTSTR)pParams->ACC_ErrorBuffer, MB_OKCANCEL|MB_ICONEXCLAMATION);
	if (response == IDOK)
	{
		if (!lock2.IsLocked())
		{
			lock2.Lock();				
			OutputFile << (LPCTSTR)TimeStampDT() << "Accucure.Main: MESSAGEBOX - Operator acknowledged alarm on Accucure Ch.0" << (pParams->i) + 1 << "\n";
			lock2.Unlock();
		}	
		CureRemainTime[pParams->i] = 0.0;
		ActualRadiance[pParams->i] = 0.0;
	}
	if(response == IDCANCEL)
	{		
		if (!lock2.IsLocked())
		{
			lock2.Lock();				
			OutputFile << (LPCTSTR)TimeStampDT() << "Accucure.Main: MESSAGEBOX - Operator CLOSED window on Accucure alarm Ch.0" << (pParams->i) + 1 << ". Program will terminate.\n";
			lock2.Unlock();
		}	
		Sleep(100);
		delete pParams;
		pParams = 0;
		exit(0);
	}
ACC_Active_MessageBoxes[pParams->i] = false;
delete pParams;
pParams = 0;
return (0);
}

[VictorN]

1. Please, edit your post adding code tags around code snippet(s).
2. Why are you casting CString to LPCTSTR in ACC_MessageBox_Params ctor_
The correct ctor should look like>

ACC_MessageBox_Params(int Channel, const CString &AlarmMsg)
{
     i = Channel;
     ACC_ErrorBuffer = AlarmMsg;
}

[99bobster99]

Sorry about the code tags, I will use them next time.

That is the way I had it when I started, I added the (LPCTSTR) for testing. The same affect happens with or without this (LPCTSTR) casting.

I found on the web that anytime you copy a CString into another CString you have to cast it as a (LPCTSTR). I tried it and it does seal up any memory leaks when copying a CString into another CString. I thought this was the issue with the code the way it originally was written (without the (LPCTSTR))?? Any other ideas on why this code cause a memory leak? The memory leak is about 1347 bytes every time I select the "Ok" button??

[S_M_A]

Choose edit and add code tags. Details can be found here http://www.codeguru.com/forum/misc.php?do=bbcode

Also edit your profile to show signatures, they provide some useful links.

[99bobster99]

I fixed the code tags up, again sorry.

This memory leak is making me pull my hair out ... any ideas on why whenever this worker thread exists that immediately there is an increased use in memory?

I've tracked it down to the exiting of this thread. There are 2 calls made, one to thrdcore.cpp (line 306) and strcore.cpp (line 141)?? Since the "ACC_MessageBox_Params" is declared as new, should it not be deleted? I tried it but the compiler reports an error "error C2541: 'delete' : cannot delete objects that are not pointers" I am suspecting that this is the issue, does anyone else have any ideas?

[Arjay]

To trace down the memory leak, start by commenting out all but the most basic code in the thread proc.

UINT ACC_MessageBox(PVOID p)
{
	// Worker Thread Pass-In Variables (Channel & Alarm Message) -> i(int), ACC_ErrorBuffer(CString)
	PPARAMS pParams = (PPARAMS)p;

/*
  other code commented out

*/

	delete pParams;
	pParams = 0;
	return (0);
}

Run this code and see if you have leaks (you shouldn't).

Next uncomment the code a line or two at a time and see if you reintroduce the memory leaks. When you find the lines that create the memory leaks, be sure to add appropriate cleanup (delete pFoo, etc.) code at the end of the thread proc.

Lastly, what would help in threading is to change your coding style to use all C++ code, rather than a combination of C++ and C-styled code.

Using pure C++, you can leverage the use of destructors and put any required cleanup code inside the destructor. For example, your thread proc may create a class instance on the stack within the thread proc. Inside the class it performs the work that you need and is also responsible for cleaning up any resources its allocated. When the thread exits, the class leaves scope and its destructor is called (which in turn frees up the resources).

That way you can avoid the error prone conditions where one section of code allocates resources and another is expected to free the resources.


__________________________________________________________
Arjay

See my latest series on using WCF to communicate between a Windows Service and WPF task bar application.
Tray Notify - Part I Tray Notify - Part II

Need a little help with Win32 thread synchronization? Check out the following CG articles and posts:
Sharing a thread safe std::queue between threads w/progress bar updating
Simple Thread: Part I Simple Thread: Part II
Win32 Thread Synchronization, Part I: Overview Win32 Thread Synchronization, Part 2: Helper Classes

www.iridyn.com

[Paul McKenzie]

Since the "ACC_MessageBox_Params" is declared as new, should it not be deleted? I tried it but the compiler reports an error "error C2541: 'delete' : cannot delete objects that are not pointers" I am suspecting that this is the issue, does anyone else have any ideas?

You should strive to minimize the usage of "new" and "delete" in your program.

Also, your code just leads to more questions:

		delete pParams;
		pParams = 0;
		exit(0);
	}
ACC_Active_MessageBoxes[pParams->i] = false;
delete pParams;

Where is "pParams" allocated? I do not see where this allocation is done in the code you posted. If it's allocated in another module, this is another cause for memory leaks or crashing -- one module not knowing what the other module is doing. The module that allocated this "pParam" could have done so 10 times, while the code you posted is executed less than 10 times, causing memory leaks.

For us to solve a memory leak problem, we need to see all of the code, and know the flow of the code when it is run.

Regards,

Paul McKenzie

[99bobster99]

Really weird, if I remove all the "pass though" varaibles created in the structure "ACC_MessageBox_Params" basically not using any of these variables [pParams->i] or [pParams->ACC_ErrorBuffer] the memory leak goes away ... these pass through variables are creating the memory leak ... what am I doing wrong in their declarations?

pParams is allocated at the top of "UINT ACC_MessageBox(PVOID p)".

[Arjay]

Really weird, if I remove all the "pass though" varaibles created in the structure "ACC_MessageBox_Params" basically not using any of these variables [pParams->i] or [pParams->ACC_ErrorBuffer] the memory leak goes away ... these pass through variables are creating the memory leak ... what am I doing wrong in their declarations?

pParams is allocated at the top of "UINT ACC_MessageBox(PVOID p)".

Memory leaks are simple. All they mean is that you are calling 'new' without making a corresponding call to 'delete'.

So there is nothing 'weird' going on. You simply aren't cleaning up memory where you've allocated it.

Btw, read what Paul and I said about not using 'new' and moving to stack based (or class method) variables.

[Paul McKenzie]

Really weird, if I remove all the "pass though" varaibles created in the structure "ACC_MessageBox_Params" basically not using any of these variables [pParams->i] or [pParams->ACC_ErrorBuffer] the memory leak goes away ... these pass through variables are creating the memory leak ... what am I doing wrong in their declarations?

You never verified or told us you verified that any of those "delete" statements are called, or whether the number of times they are called match up with the number of times you called "new".

Just looking at code isn't enough. What is the actual flow of the program? What gets called when? How many times? etc...

Regards,

Paul McKenzie

[Nikitozz]

What is TimeStampDT()? Can you show this function definition?

[99bobster99]

Here is my TimestampDT() function;

CString TimeStampDT()
{
	// Variable Declarations
	SYSTEMTIME st;

	// Set Abnormal (Exception Error Handling) 
	set_terminate (myterminate);

	// Start of main code
	GetLocalTime(&st);
	TimeStampDTBuffer.Format("< %04d/%02d/%02d - %02d:%02d:%02d:%04d > ", st.wYear, st.wMonth, st.wDay, st.wHour, st.wMinute, st.wSecond, st.wMilliseconds);

	return (TimeStampDTBuffer);
}

There are "weird" memory leaks that happen within C++. i.e. why do you have to assign a (LPCTSTR) in front of another CString when copying into another CString?? This is flipp'in weird, all other variables can be copied from the same into the same. Where are all these "rules" written for C++? This is why I am never clear on how to tell where the memory leaks are coming from??

[Paul McKenzie]

There are "weird" memory leaks that happen within C++. i.e.

The only memory leaks are ones introduced by the programmer.

why do you have to assign a (LPCTSTR) in front of another CString when copying into another CString??

Show us where you need to do that. Wait, there is no need to, since there is no such thing as you describe-- a CString has an overloaded operator = that takes a CString reference, so your claim is impossible.

Where are all these "rules" written for C++?

You are confusing badly written code with what you think is a problem with the language. The rules of C++ are developed by the ANSI/ISO committee. If you want a copy of them, you can get the PDF version from them for 18-20 US dollars. It is over 700 pages, describing within it what is defined, undefined, and implementation-defined behaviour.

Secondly, what you have written uses proprietary classes such as CString and threading functions -- these are not spoken of at all in the rules of C++. The only thing that is spoken of is whatever classes you write or use, the code follows the basic C++ rules, otherwise behaviour is undefined/implementation-defined.

Regards,

Paul McKenzie

[Arjay]

This is flipp'in weird, all other variables can be copied from the same into the same. Where are all these "rules" written for C++? This is why I am never clear on how to tell where the memory leaks are coming from??

What is weird is why some programmers can't simply follow advice. Along with other's advice, I specifically gave you a strategy to track down the memory leaks by suggesting you comment out portions of the code in the thread proc. Did you do that? If not, why didn't you?