C++/MFC: Problems with critical section

[Nevermind]

Can anyone tell me why i reach the breakpoint on the int dummy = 1 row?
I had thought that it would be impossible (thats why I put the critical section stuff there, without it my app crashes)

[color=white]//I do this line in OnInitApp[/color]
[color=white]::InitializeCriticalSection( &g_closedoc_lock );[/color]

[color=white]// This is done in another procedure[/color]
[color=white]::EnterCriticalSection(&g_closedoc_lock);[/color]
[color=white]static int iInCloseCount = 0;[/color]
[color=white]if(iInCloseCount > 0)[/color]
[color=white]   int dummy = 1; // I reach a breakpoint here... [/color]
[color=white]iInCloseCount++;[/color]
[color=white]// Some other stuff is done here, but the lock or the static isn't[/color]
[color=white]// accessed in any other place in the app...[/color]
[color=white]iInCloseCount--;[/color]
[color=white]::LeaveCriticalSection(&g_closedoc_lock);[/color]

 

[CodeWeasel]

when you hit the breakpoint, what is the value of iInCloseCount?

Have you tried doing a line by line debug of the program and watched the variables?

 

[mikeblas]

Is that static local to the locking function?

Which threads are entering the locking procedure? If a given thread owns a particular critical section, it will not block on itself when calling EnterCriticalSection() again on that same critical section.

What does the call stack tell you when you hit your breakpoint?

If you're using MFC, why aren't you using CCriticalSection, BTW?

.B ekiM

 

[Nevermind]

The value of iInCloseCount is usually 0, but can sometimes be 1.

I would do a line-by-line debuging, but the error only occurs if i close many documents in rapid sucession (the code is in OnCloseDocument).

The static is local to the function.

I assume that it is entierly possible that OnCloseDocument is called twice (if i click the close button fast enough). Would this count as the same thread? Can i lock access even to the same thread?

I was under the impression that CCriticalSection was just a wrapper around CRITICAL_SECTION, but I don't really know a lot about thread management. What would the benifits of using CCriticalSection be? How would I need to modify my code?

 

[mikeblas]

I assume that it is entierly possible that OnCloseDocument is called twice (if i click the close button fast enough). Would this count as the same thread?

If you've left OnCloseDocument() to be called in response to the WM_CLOSE or WM_EXIT of your application, yes: it can be re-entered.

Say you close a docment with the UI. OnCloseDocument() gets called. Then, it decides to do some message handling; maybe it waits for something and that thing passes messages, perhaps it makes an OLE call that's marshalled and the marashaller decides to pump messages while waiting. You do another close document in the UI. If the code that is handling the original close is now pumping messages, it will end up finding the message you created by using the UI, requesting that the close happen. In processing that message, OnCloseDocument() will be called again.

How can iInCloseCount be zero if your breakpoint is hit? Is your if statement broken? Or is your working theory that some other thread is modifying the value of iInCloseCount back to zero after it has been tested and found to be greater than zero?

You didn't answer my question about the call stack. That was intended to get you to prove to yourself what's happening. If your code is re-entering itself, it will be very obvious on the call stack. If you have more than one thread in that function at the same time, it will also be very obvious in the debugger; just look at the call stacks for each thread and see which goes through your function.

Is your application actively multithreaded, or is it just that you're trying to work around this crashing bug you have?

What error ocurrs if you "click the close button fast enough"? Do you crash? What is the call stack at the point of the crash?

Can i lock access even to the same thread?

Yes: it's called hanging. If OnCloseDocument() ends up reentering itself and you have the second entry wait for the first entry to complete, it never will complete because it's waiting for the second entry to return.

If a single thread were to block on a critical section because it had already acquired it, it would always hang because it is blocked and waiting for itself to release that object.

What would the benifits of using CCriticalSection be? How would I need to modify my code?

Using CCrticialSection won't solve this problem. It can make your code cleaner, though.

.B ekiM

 

[Nevermind]

How can iInCloseCount be zero if your breakpoint is hit? Is your if statement broken? Or is your working theory that some other thread is modifying the value of iInCloseCount back to zero after it has been tested and found to be greater than zero?

Of course it isn't zero when the breakpoint is hit. If I close documents in rapid succession, the breakpoint is reached one time in ten or so, so I conclude that it is 0 the other nine times.

You didn't answer my question about the call stack. That was intended to get you to prove to yourself what's happening. If your code is re-entering itself, it will be very obvious on the call stack. If you have more than one thread in that function at the same time, it will also be very obvious in the debugger; just look at the call stacks for each thread and see which goes through your function.

It doesn't reenter itself, it comes from WM_CLOSE. How can I look at the callstacks for other threads int VS2003?

Is your application actively multithreaded, or is it just that you're trying to work around this crashing bug you have?

The part where the problem occurs is multithreaded, yes.

What error ocurrs if you "click the close button fast enough"? Do you crash? What is the call stack at the point of the crash?

It crashes when it tries to delete objects (the objects have already been deleted´). The call stack looks like it should (starting at the WM_CLOSE message).

Yes: it's called hanging. If OnCloseDocument() ends up reentering itself and you have the second entry wait for the first entry to complete, it never will complete because it's waiting for the second entry to return.

Yes, but i don't need to wait for the other call to OnCloseDocumet to finish, i just need to exit and do nothing if it is locked.

 

[mikeblas]

Of course it isn't zero when the breakpoint is hit.

Sorry, I thought you were answering CodeWeasel, who had asked: "when you hit the breakpoint, what is the value of iInCloseCount?"

It doesn't reenter itself, it comes from WM_CLOSE.

Sorry, I guess I wasn't clear.

Say someone sends WM_CLOSE. That causes Your OnCloseDocument() function to get called. Your OnCloseDocument() function does some work, and it ends up causing other messages to be processed. OnCloseDocument(), then, has called back into Windows. Windows has found a message, and called back to your app with a WM_CLOSE message. That causes OnCloseDocument() to be entered.

This doesn't mean your function re-enters itself. The call is still coming from WM_CLOSE. It's just that the re-entry happens indirectly through Windows.

What work are you doing in response to OnCloseDocument()?

How can I look at the callstacks for other threads int VS2003?

With execution stopped in the debugger (either at a breakpoint or after jitting into the application after it crashes), open the "threads" window. (It's on the "Windows" tear-off menu in the "Debug" drop-down menu.) The yellow arrow in the Threads window shows you the current thread context you're working with.

You can click on another thread in this window to set the context to that thread. After you change contexts, go back to the call stack window and see the stack for the thread that currently has context.

The selected thread sets the context for lots of things: the call stack window, and all the evaluation windows, too (like the watch window, and autos and locals and so on).

The call stack looks like it should (starting at the WM_CLOSE message).

Would you mind showing the exact content of the callstack at the crash?

In your application, what gates the deletion of these objects that should only be deleted once? Are you relying on some cleanup function to be called only once? Why not reset the pointers you're deleting to NULL so deleting them again would be harmless? Or is there other code in this same area that would be harmful to run twice?

Yes, but i don't need to wait for the other call to OnCloseDocumet to finish, i just need to exit and do nothing if it is locked.

Then just mark the entry to the function in a thread-safe way against a static variable. Like this:

void YourFunction()
{
	static volatile LONG lEntries = 0;

	long lResult = InterlockedIncrement(&lEntries);
	if (lResult == 1)
	{
		// we're the only ones here.
		// let's get to work and do whatever one-shot stuff we want
	}

	InterlockedDecrement(&lEntries);
	return;

}

Using InterlockedIncrement() and InterlockedDecrement() is important because you have multiple threads in the function. If you use a simple static variable like this:

void YourFunction()
{
	static LONG lEntries = 0;

	if (lEntries == 0)
	{
		lEntries = 1;
		// we're the only ones here.
		// let's get to work and do whatever one-shot stuff we want

		lEntries = 0;
	}

	return;
}

... it's still possible that you have a race condition. If two threads check lEntries at the same time, they'll both discover it is zero. Then, they'll both set it to one, then they'll both enter the if block and run the code you were trying to protect. InterlockedIncrement() is atomic, and guarantees that only one thread is touching the value at a time. By testing the return of the InterlockedIncrement(), we know we're testing what we incremented and noth what another thread is touching.

.B ekiM

 

[Nevermind]

Sorry, I guess I wasn't clear.

Say someone sends WM_CLOSE. That causes Your OnCloseDocument() function to get called. Your OnCloseDocument() function does some work, and it ends up causing other messages to be processed. OnCloseDocument(), then, has called back into Windows. Windows has found a message, and called back to your app with a WM_CLOSE message. That causes OnCloseDocument() to be entered.

This doesn't mean your function re-enters itself. The call is still coming from WM_CLOSE. It's just that the re-entry happens indirectly through Windows.

I'm with you, i just don't know how to find out if this is happening?

What work are you doing in response to OnCloseDocument()?

I delete some members, then I call the base class

With execution stopped in the debugger (either at a breakpoint or after jitting into the application after it crashes), open the "threads" window. (It's on the "Windows" tear-off menu in the "Debug" drop-down menu.) The yellow arrow in the Threads window shows you the current thread context you're working with.

You can click on another thread in this window to set the context to that thread. After you change contexts, go back to the call stack window and see the stack for the thread that currently has context.

The selected thread sets the context for lots of things: the call stack window, and all the evaluation windows, too (like the watch window, and autos and locals and so on).

Thanks! That's really good to know.

Would you mind showing the exact content of the callstack at the crash?

It crashes in sligthly different places. Here is a dump when it crashes in the baseclass OnCloseDocument

> mfc71d.dll!AfxAssertValidObject(const CObject * pOb=0x0b0705c8, const char * lpszFileName=0x7c15e9c4, int nLine=680)  Line 92 C++
  mfc71d.dll!COleServerDoc::OnCloseDocument()  Line 684 C++
  DV6.exe!CTrinDoc::OnCloseDocument()  Line 2039 C++
  mfc71d.dll!CFrameWnd::OnClose()  Line 842 C++
  mfc71d.dll!CWnd::OnWndMsg(unsigned int message=16, unsigned int wParam=0, long lParam=0, long * pResult=0x0012f28c)  Line 2023 C++
  mfc71d.dll!CWnd::WindowProc(unsigned int message=16, unsigned int wParam=0, long lParam=0)  Line 1745 + 0x1e C++
  mfc71d.dll!AfxCallWndProc(CWnd * pWnd=0x0b06f358, HWND__ * hWnd=0x002906dc, unsigned int nMsg=16, unsigned int wParam=0, long lParam=0)  Line 241 + 0x1a C++
  mfc71d.dll!AfxWndProc(HWND__ * hWnd=0x002906dc, unsigned int nMsg=16, unsigned int wParam=0, long lParam=0)  Line 389 C++
  mfc71d.dll!AfxWndProcBase(HWND__ * hWnd=0x002906dc, unsigned int nMsg=16, unsigned int wParam=0, long lParam=0)  Line 209 + 0x15 C++
  user32.dll!77d38709()  
  user32.dll!77d387eb()  
  user32.dll!77d60494()  
  user32.dll!77d3b743()  
  user32.dll!77d3e2f7()  
  mfc71d.dll!CWnd::SendMessageA(unsigned int message=16, unsigned int wParam=0, long lParam=0)  Line 44 + 0x42 C++
  DV6.exe!CBCGTabWnd::OnCommand(unsigned int wParam=65535, long lParam=459864)  Line 3301 C++
  mfc71d.dll!CWnd::OnWndMsg(unsigned int message=273, unsigned int wParam=65535, long lParam=459864, long * pResult=0x0012f6b0)  Line 1759 + 0x1c C++
  mfc71d.dll!CWnd::WindowProc(unsigned int message=273, unsigned int wParam=65535, long lParam=459864)  Line 1745 + 0x1e C++
  mfc71d.dll!AfxCallWndProc(CWnd * pWnd=0x021075d4, HWND__ * hWnd=0x0009045a, unsigned int nMsg=273, unsigned int wParam=65535, long lParam=459864)  Line 241 + 0x1a C++
  mfc71d.dll!AfxWndProc(HWND__ * hWnd=0x0009045a, unsigned int nMsg=273, unsigned int wParam=65535, long lParam=459864)  Line 389 C++
  mfc71d.dll!AfxWndProcBase(HWND__ * hWnd=0x0009045a, unsigned int nMsg=273, unsigned int wParam=65535, long lParam=459864)  Line 209 + 0x15 C++
  user32.dll!77d38709()  
  user32.dll!77d387eb()  
  user32.dll!77d387ff()  
  user32.dll!77d60494()  
  user32.dll!77d3b743()  
  user32.dll!77d3b7ab()  
  comctl32.dll!773e6ff6()  
  comctl32.dll!773e70d8()  
  comctl32.dll!773e93dd()  
  mfc71d.dll!AfxWndProc(HWND__ * hWnd=0x00000000, unsigned int nMsg=1392824, unsigned int wParam=1391984, long lParam=1243544)  Line 389 C++
  mfc71d.dll!AfxWndProcBase(HWND__ * hWnd=0xa1647c2f, unsigned int nMsg=0, unsigned int wParam=629761104, long lParam=0)  Line 209 + 0x27 C++
  99b068ff()

In your application, what gates the deletion of these objects that should only be deleted once? Are you relying on some cleanup function to be called only once? Why not reset the pointers you're deleting to NULL so deleting them again would be harmless? Or is there other code in this same area that would be harmful to run twice?

I do, but one thread can delete an object, and then switch out just before the NULL assignment to another which deletes it again. This is why I wondered if the two calls was considered the same thread, it certainly seems so since the critical section stuff doesn't stop it.

Then just mark the entry to the function in a thread-safe way against a static variable. Like this:

void YourFunction()
{
	static volatile LONG lEntries = 0;
 
	long lResult = InterlockedIncrement(&lEntries);
	if (lResult == 1)
	{
		// we're the only ones here.
		// let's get to work and do whatever one-shot stuff we want
	}
 
	InterlockedDecrement(&lEntries);
	return;
 
}

Using InterlockedIncrement() and InterlockedDecrement() is important because you have multiple threads in the function. If you use a simple static variable like this:

void YourFunction()
{
	static LONG lEntries = 0;
 
	if (lEntries == 0)
	{
		lEntries = 1;
		// we're the only ones here.
		// let's get to work and do whatever one-shot stuff we want
 
		lEntries = 0;
	}
 
	return;
}

... it's still possible that you have a race condition. If two threads check lEntries at the same time, they'll both discover it is zero. Then, they'll both set it to one, then they'll both enter the if block and run the code you were trying to protect. InterlockedIncrement() is atomic, and guarantees that only one thread is touching the value at a time. By testing the return of the InterlockedIncrement(), we know we're testing what we incremented and noth what another thread is touching.

.B ekiM

This works great. Problem solved. Thank you. I still wonder about the same-thread-but-different-context thing though.

 

[mikeblas]

It crashes in sligthly different places. Here is a dump when it crashes in the baseclass OnCloseDocument

This isn't a crash; it's an assertion failure.

This works great. Problem solved. Thank you. I still wonder about the same-thread-but-different-context thing though.

Why not try to trap the case using the opposite test? If the function has not been entered, do you work. If it has been entered, trap into the debugger:

void YourFunction()
{
	static volatile LONG lEntries = 0;
 
	long lResult = InterlockedIncrement(&lEntries);
	if (lResult == 1)
	{
		// we're the only ones here.
		// let's get to work and do whatever one-shot stuff we want
	}
	else
	{
		// why the hell were we given two overlapping calls?
		// stop in the debugger, right now, so someone can
		// come by and help us!
		DebugBreak();
	}
 
	InterlockedDecrement(&lEntries);
	return;
 
}

When that DeubgBreak call traps you into the debugger, what are the stacks like?

.B ekiM

 

[Nevermind]

This isn't a crash; it's an assertion failure.

On an AfxAssertValidObject(this), yes. In realease it would crash.

Why not try to trap the case using the opposite test? If the function has not been entered, do you work.
...
When that DeubgBreak call traps you into the debugger, what are the stacks like?

.B ekiM

The same as what i posted above, except that it stops in OnCloseDocument, of course.

Edit:
The other threads look like this:

Win32 Thread
 
> ntdll.dll!7c90eb94()  
  ntdll.dll!7c90e399()  
  rpcrt4.dll!77e76703()

Win32 Thread
 
> ntdll.dll!7c90eb94()  
  ntdll.dll!7c90d85c()  
  kernel32.dll!7c8023ed()  
  kernel32.dll!7c8399f3()  
  kernel32.dll!7c802451()  
  ole32.dll!774ed6ae()  
  ole32.dll!774ed83b()  
  ntdll.dll!7c910945()  
  ole32.dll!774ed8a3()  
  ntdll.dll!7c91094e()  
  kernel32.dll!7c80b50b()  
  ntdll.dll!7c910945()  
  ntdll.dll!7c91094e()  
  kernel32.dll!7c8399f3()

_threadstart
 
  ntdll.dll!7c90eb94()  
  ntdll.dll!7c90d85c()  
  kernel32.dll!7c8023ed()  
  kernel32.dll!7c8399f3()  
  kernel32.dll!7c802451()  
> DV6.exe!BCGSoundThreadProc(void * __formal=0x00000000)  Line 42 + 0xa C++
  msvcr71d.dll!_threadstart(void * ptd=0x003888b8)  Line 196 + 0xd C
  kernel32.dll!7c80b50b()  
  kernel32.dll!7c8399f3()

Win32 thread
 
> ntdll.dll!7c90eb94()  
  ntdll.dll!7c90e9ab()  
  kernel32.dll!7c8094f2()  
  kernel32.dll!7c8399f3()  
  kernel32.dll!7c809c86()  
  wdmaud.drv!72cf312a()  
  ntdll.dll!7c90e2dc()  
  kernel32.dll!7c80b50b()  
  kernel32.dll!7c8399f3()

Win32 thread
 
> ntdll.dll!7c90eb94()  
  ntdll.dll!7c90d85c()  
  ntdll.dll!7c9279d4()  
  kernel32.dll!7c80b50b()  
  kernel32.dll!7c8399f3()  
  ntdll.dll!7c92798d()

_threadstartex
 
  ntdll.dll!7c90eb94()  
  user32.dll!77d394e3()  
  user32.dll!77d3c0de()  
  user32.dll!77d3e2f7()  
  mfc71d.dll!CTreeCtrl::GetRootItem()  Line 286 + 0x42 C++
> DV6.exe!CTreeCtrlEx::DeleteAllItems()  Line 516 + 0x8 C++
  DV6.exe!CTreeCtrlEx::__CreateTreeProcedure()  Line 590 C++
  DV6.exe!CreateTreeThread(void * param=0x05742a08)  Line 76 C++
  mfc71d.dll!_AfxThreadEntry(void * pParam=0x00128758)  Line 114 + 0xd C++
  msvcr71d.dll!_threadstartex(void * ptd=0x0a93ec98)  Line 241 + 0xd C
  kernel32.dll!7c80b50b()  
  kernel32.dll!7c8399f3()

_threadstartex
 
	ntdll.dll!7c90eb94()  
  user32.dll!77d394e3()  
  user32.dll!77d3c0de()  
  user32.dll!77d3e2f7()  
  mfc71d.dll!CTreeCtrl::GetRootItem()  Line 286 + 0x42 C++
> DV6.exe!CTreeCtrlEx::DeleteAllItems()  Line 516 + 0x8 C++
  DV6.exe!CTreeCtrlEx::__CreateTreeProcedure()  Line 590 C++
  DV6.exe!CreateTreeThread(void * param=0x020db55c)  Line 76 C++
  mfc71d.dll!_AfxThreadEntry(void * pParam=0x0012849c)  Line 114 + 0xd C++
  msvcr71d.dll!_threadstartex(void * ptd=0x058f35c0)  Line 241 + 0xd C
  kernel32.dll!7c80b50b()  
  kernel32.dll!7c8399f3()

 

[mikeblas]

On an AfxAssertValidObject(this), yes. In realease it would crash.

How so? Because you've passed it a bad pointer?

The same as what i posted above, except that it stops in OnCloseDocument, of course.
The other threads look like this:

That's curious. These stacks mean, then, that you're entering the function from two distinct threads and you should have been able to block the two threads from concurrent access using your original critical section.

You said the solution with the interlocked operations worked and fixed your problem. One difference in that sotluion is that you don't execution the function if it's already executing. The critical section will wait until the function is done executing, but then execute it anyway.

.B ekiM

 

[Nevermind]

How so? Because you've passed it a bad pointer?

Yes. The this pointer is dead.

The critical section will wait until the function is done executing, but then execute it anyway.

.B ekiM

Of course. I'm an idiot.