Stopping threads in DLLs

[MWP]

Hi all,

My code is somthing like the following:

//global
BOOL terminated;

BOOL APIENTRY DllMain(HANDLE hinst, DWORD  reason, LPVOID lpReserved)
{
	if (reason == DLL_PROCESS_ATTACH)
	{
		//starting
		DisableThreadLibraryCalls((HINSTANCE)hinst);
		terminated = FALSE;
		thread_handle = CreateThread(NULL, 0, run_thread, 0, CREATE_SUSPENDED, &thread_id);
		SetThreadPriority(thread_handle, THREAD_PRIORITY_HIGHEST);
		ResumeThread(thread_handle);
	}

	if (reason == DLL_PROCESS_DETACH)
	{
		if (lpReserved != NULL)
			log_log(LOG_CRITICAL, "DllMain", "plugin exception error, terminated");

		//ending
		terminated = TRUE;
	}

	return 1;
}
//---------------------------------------------------------------------------
DWORD __stdcall run_thread(LPVOID lparam)
{
	//setup the log
	log_start("thingie");

	//init timers
	timer_calibrate();	

	//start main thread loop
	while (terminated == FALSE)
	{
		//do stuff here
		timer_lazysleep(0.020);
	}

	//stop logging
	log_end();

	//done
	FreeLibraryAndExitThread((HMODULE)lparam, 0);

	return 0;
}

Is this the correct way to shutdown a thread on calling FreeLibrary() from the app that loaded the DLL?

Im guessing its not, due to using the "terminated" global var, but i cant think of how it should be done properly.

Thanks.

[anonytmouse]

After the system has called DLL_PROCESS_DETACH, it will remove the DLL from memory. In your code, the thread function may still be running, which would cause a crash.

Here is one way you could do it:

//global
BOOL terminated;

BOOL APIENTRY DllMain(HANDLE hinst, DWORD  reason, LPVOID lpReserved)
{
	if (reason == DLL_PROCESS_ATTACH)
	{
		//starting
		DisableThreadLibraryCalls((HINSTANCE)hinst);
		terminated = FALSE;
		thread_handle = CreateThread(NULL, 0, run_thread, 0, CREATE_SUSPENDED, &thread_id);
		SetThreadPriority(thread_handle, THREAD_PRIORITY_HIGHEST);
		ResumeThread(thread_handle);
	}

	if (reason == DLL_PROCESS_DETACH)
	{
		if (lpReserved != NULL)
			log_log(LOG_CRITICAL, "DllMain", "plugin exception error, terminated");

		//ending
		terminated = TRUE;

		// Wait for thread to finish before returning...
		WaitForSingleObject(thread_handle, INFINITE);

		CloseHandle(thread_handle);
	}

	return 1;
}
//---------------------------------------------------------------------------
DWORD __stdcall run_thread(LPVOID lparam)
{
	//setup the log
	log_start("thingie");

	//init timers
	timer_calibrate();	

	//start main thread loop
	while (terminated == FALSE)
	{
		//do stuff here
		timer_lazysleep(0.020);
	}

	//stop logging
	log_end();

	// Don't unload here...

	return 0;
}

I would also strongly recommend that you move the start and stop code to seperate functions (say Start and Stop) that are called by the calling application. Running code like this from DllMain may cause problems as outlined in the DllMain documentation page.

You would probably be better off replacing the terminated variable with an event. This can be waited on instead of calling Sleep. Something like this:

	while (WaitForSingleObject(hTerminationEvent, 200) == WAIT_TIMEOUT)
	{
		//do stuff here
	}

[Mad_guy]

When you declare BOOL terminate; you do not declare it as volatile. Meaning that the compiler doesn't bother to reload it in it's implementation. What if the compiler optimizes that while loop to this:

mov eax,[terminated]
whileloop:
cmp eax,0 ;Remember, TRUE == anything that's NOT 0
je whileloop

This would obviously cause a infinate loop. Therefore, if you take this route (which I advise you do not) that you declare the global Bool as Volatile. An alternative is to use an event + WaitFor*, or declare the global as long and instead use the Interlocked* functions.

[MWP]

Thanks for the advice guys, but im still having problems.

I have changed to "volatile BOOL terminated" and am using WaitForSingleObject() as above.

I have stepped through the code, and the thread does return after the "terminated" var stops the processing loop, but WaitForSingleObject() never returns.

Any ideas?

Thanks again.

[bithub]

When you declare BOOL terminate; you do not declare it as volatile. Meaning that the compiler doesn't bother to reload it in it's implementation. What if the compiler optimizes that while loop to this:

mov eax,[terminated]
whileloop:
cmp eax,0 ;Remember, TRUE == anything that's NOT 0
je whileloop

This would obviously cause a infinate loop. Therefore, if you take this route (which I advise you do not) that you declare the global Bool as Volatile. An alternative is to use an event + WaitFor*, or declare the global as long and instead use the Interlocked* functions.

The volatile keyword is only used on variables which can change outside of the scope of the application. In this case, the variable terminated is only shared among threads, thus volatile is not needed. The optimization you outlined would never take place with a global variable.

[anonytmouse]

The volatile keyword is only used on variables which can change outside of the scope of the application. In this case, the variable terminated is only shared among threads, thus volatile is not needed. The optimization you outlined would never take place with a global variable.

I did a search and this seems to be a controversial topic. My conclusions from usenet posts:

• The C standard does not deal with threads so gives no guidance about the use of volatile in this situation.
• Behaviour is compiler specific.

In that vain, I did some testing on my two installed compilers, MSVC.NET and GCC (Dev C++/MinGW). The results are reproduced below.

Code
The volatile keyword was included or taken out as noted.

#include <windows.h>

volatile int i;

DWORD CALLBACK ThreadA(LPVOID lparam)
{
	int j = 0;

	while (i == 1)
	{
		j++;
	}

	return 0;
}

DWORD CALLBACK ThreadB(LPVOID lparam)
{
	int j = 0;

	while (i == 2)
	{
		j++;
	}

	return 0;
}

DWORD CALLBACK ThreadC(LPVOID lparam)
{
	int j = 0;
	while (j++ < 10000)  ;
	i = 2;
	return 0;
}

int main(void)
{
	HANDLE hThread = CreateThread(NULL, 0, ThreadA, 0, 0, NULL);
	CloseHandle(hThread);

	hThread = CreateThread(NULL, 0, ThreadC, 0, 0, NULL);
	CloseHandle(hThread);

	ThreadB(NULL);

	return 0;
}

VC using /O1, no volatile
This does the comparison once and goes into an infinite loop if it matches.

_ThreadA@4 PROC NEAR					; COMDAT

; 8    : 	int j = 0;
; 9    : 
; 10   : 	while (i == 1)

	cmp	DWORD PTR _i, 1
	jne	SHORT $L74000
$L73999:
	jmp	SHORT $L73999
$L74000:

; 11   : 	{
; 12   : 		j++;
; 13   : 	}
; 14   : 
; 15   : 	return 0;

	xor	eax, eax

; 16   : }

	ret	4

VC using /O2, no volatile
This does the comparison once and goes into an infinite loop if it matches.

_ThreadA@4 PROC NEAR					; COMDAT

; 8    : 	int j = 0;
; 9    : 
; 10   : 	while (i == 1)

	cmp	DWORD PTR _i, 1
	jne	SHORT $L74081
$L74048:
	jmp	SHORT $L74048
$L74081:

; 11   : 	{
; 12   : 		j++;
; 13   : 	}
; 14   : 
; 15   : 	return 0;

	xor	eax, eax

; 16   : }

	ret	4

VC using /O1, with volatile
This does the comparison every loop iteration

_ThreadA@4 PROC NEAR					; COMDAT

; 7    : {

$L74032:

; 8    : 	int j = 0;
; 9    : 
; 10   : 	while (i == 1)

	cmp	DWORD PTR _i, 1
	je	SHORT $L74032

; 11   : 	{
; 12   : 		j++;
; 13   : 	}
; 14   : 
; 15   : 	return 0;

	xor	eax, eax

; 16   : }

	ret	4

VC using /O2, with volatile
This does the comparison every loop iteration

_ThreadA@4 PROC NEAR					; COMDAT

; 8    : 	int j = 0;
; 9    : 
; 10   : 	while (i == 1)

	mov	ecx, DWORD PTR _i
	mov	eax, 1
	cmp	ecx, eax
	jne	SHORT $L74081
$L74048:
	cmp	DWORD PTR _i, eax
	je	SHORT $L74048
$L74081:

; 11   : 	{
; 12   : 		j++;
; 13   : 	}
; 14   : 
; 15   : 	return 0;

	xor	eax, eax

; 16   : }

	ret	4

GCC using /O2, no volatile
This does the comparison once and goes into an infinite loop if it matches.

_ThreadB@4:
	pushl	%ebp
	movl	_i, %eax     // Next 4 lines: if (i == 2) goto L12;
	movl	%esp, %ebp
	cmpl	$2, %eax
	je	L12
L14:
	xorl	%eax, %eax    // Next 3 lines: return 0;
	popl	%ebp
	ret	$4
L12:
	je	L12
	jmp	L14             // This is never reached

GCC using /O2, with volatile
This does the comparison every loop iteration

_ThreadB@4:
	pushl	%ebp
	movl	_i, %eax     // Next 4 lines: if (i == 2) goto L12;
	movl	%esp, %ebp
	cmpl	$2, %eax
	je	L12
L14:
	xorl	%eax, %eax   // Next 3 lines: return 0;
	popl	%ebp
	ret	$4
L12:
	movl	_i, %eax   // Next 3 lines: if (i == 2) goto L12;
	cmpl	$2, %eax
	je	L12
	jmp	L14                // Next 1 lines: goto L14;

Conclusion
Although there is no guarantee that volatile will help when using a global variable accross threads, this testing clearly suggests that it may be required when using MSVC or MinGW and it is probably a good idea to use it.

[bithub]

I stand corrected. I guess I've just never come across this problem before. To be fair I rarely turn on optimizations on my projects though .

[Mad_guy]

The volatile keyword is only used on variables which can change outside of the scope of the application. In this case, the variable terminated is only shared among threads, thus volatile is not needed. The optimization you outlined would never take place with a global variable.

The reason the keyword is there is because volatility says it could be changed by basically _any_ force, and therefore the compiler should reload it when it is used rather than using a consistant copy loaded into a register. The 'outside' force here, is the other thread when you think about it, if you don't declare it as volatile, there is a pretty good chance that it won't bothered to reload it which is very much bad.

And people wonder why I think that threading is the bane of all programming. It's a very powerful technique if you can successfully do it, but it is quite dangerous and should try and be avoided if you ask me.