Killing a thread properly

[scythe]

I'm working on a project that's almost done. When I started, the framework (Windows programming and some other stuff) was already completed by another programmer. Now that I'm almost done, I've encountered a bug that makes explorer crash from time to time. I suspect it might have something to do with a thread for the gui window in the application that occurs when the app is shutting down. I'm pretty green when it comes to windows programming, so it's hard for me to ensure that everything in the code below is OK.

Anyway, here is the code:

// Thread procedure. Needed for message handling
DWORD WINAPI WorkerThread(LPVOID lpParam) {

	GUIWindow *gui;
	pThreadParams parms = (pThreadParams)lpParam;
	gui = new GUIWindow();
	gui->Create(IDD_DIALOG2,NULL);
	HWND hWnd = gui->GetSafeHwnd();
	parms->pUIWindow = gui;
	SetEvent(parms->EInitDone);

	// The message loop
	while (1) {
		DWORD dwStatus = MsgWaitForMultipleObjectsEx(0, 
													NULL,
													2000, 
													QS_ALLINPUT, 
													MWMO_INPUTAVAILABLE);
		
		if (dwStatus == WAIT_OBJECT_0) {
			MSG msg;		

			while (PeekMessage(&msg, NULL, 0, 0, PM_REMOVE)) {
				if (!CallMsgFilter(&msg, 0x5300)) { 
					TranslateMessage(&msg);
					DispatchMessage(&msg);
				}
			}
		}

		dwStatus = WaitForSingleObject(parms->EKillDone,0);
		
		if (dwStatus == WAIT_OBJECT_0) {
			return 0;
		}
	}
	return 0;
}

The main app's init method creates the thread like this:

	
	thParams.EInitDone = CreateEvent(NULL,FALSE,FALSE,NULL); 
	thParams.EKillDone = CreateEvent(NULL,FALSE,FALSE,NULL);

	CreateThread(NULL,0,WorkerThread,(LPVOID)&thParams,0,NULL);

// *snip* 

And kills it when the app exits:
SetEvent(thParams.EKillDone);

Is the SetEvent code above supposed to make dwStatus == WAIT_OBJECT_0 and exit the thread? Because that never happens. And if so, could this mess with explorer?

I don't wanna be the guy who dumps a pile of code on you and expect you to solve my problem, but if you see something iffy in the thread code please let me know.

[kbw]

If the code ever gets to WaitForSingleObject(), setting the the event will terminate the thread. It's possible it may never get there when you need it to.

You may also be leaking the thread handle, you need to call CloseHandle() on it if you don't want to synchronise with it anymore.

[scythe]

Thank you for your reply. I discovered that the app doesn't make explorer crash on those occasions when reaching dwStatus == WAIT_OBJECT_0. What's the best way to make sure that this happens everytime the app closes?

[kbw]

It's difficult to answer your question with the code that's there, plus I'm not an expert in ShellAPI matters. However, CreateThread() is frowned upon because it doesn't cater for thread local runtime vars like errno. Instead, you should use _beginthreadex() or AfxBeginThread() for an MFC app.

[Mal Reynolds]

There are a couple of things to consider here.

Obviously i don't know much about your program, but, based on the code you posted, the events and waitforobject()'s probably aren't necessary. when the user clicks the 'x' on the window, a message will be sent to destroy the window, where you can execute any 'cleanup' code by handling the WM_CLOSE message ( i think it's that one ) in your window procedure. then, you can have your message loop terminate, at which point you can call ExitThread() to cleanly terminate the worker thread, back into the main application thread.
Here's a simple layout of what i'm talking about:

// Thread procedure
DWORD WINAPI WorkerThread(LPVOID lpParam)
{
	// here, create the window you want
    
    // here is a simple message loop, it will run the WM_QUIT message is posted
    // you can tailor it to quit the loop however you need
    while( 
    GetMessage(      
    LPMSG lpMsg,
    HWND hWnd,
    UINT wMsgFilterMin,
    UINT wMsgFilterMax
    ) )
    {
		if (!CallMsgFilter(&msg, 0x5300))
        { 
			TranslateMessage(&msg);
			DispatchMessage(&msg);
		}
	}
    
    // now exit the thread returning to main thread
    ExitThread( 0 );
}

[scythe]

Thanks!

The code you posted seems to work, but the loop never breaks. Atm, my code, based on the code above looks like this:

DWORD WINAPI WorkerThread(LPVOID lpParam) {
	// *snip* window creation and stuff


	int ret;
	while ((ret = GetMessage(&msg,NULL,0,0)) != 0) {

		if (!CallMsgFilter(&msg, 0x5300)) { 
			TranslateMessage(&msg);
			DispatchMessage(&msg);
		}

		if (ret == -1)
			break;
	} 
   
	ExitThread( 0 ); // never reaches this point
	return 0;
}

To my understanding, GetMessage will return 0 when a WM_QUIT message is posted.
My project is for a company at which I'm doing an internship. I'm writing a dll for an application they use. So basically, the dll opens a window inside the main app and then does its thing.
The parent app loads the dll on startup, creates a new instance everytime it needs it and then closes it afterwards, so my window and its thread aren't running constantly while the main app is running.

A bit redundant, but:

Main app
    |
  My app/dll
           \ 
           thread

[Mal Reynolds]

ah, lucky, i don't have an internship yet, i've been looking for one though, problem is i need money, so i've gotta find a paid one if i want groceries :)

hmm... i think i may know the problem. it may be getting to that part, but not visibly. what i forgot about ExitThread() is that when you are working with C++ code, ExitThread() instantly terminates the thread, without allowing any destructors to be called. so try removing ExitThread(), and just returning from the thread. beside that i don't see anything obviously wrong, considering that you only have the one window though, i would explicitly pass the window handle to GetMessage() instead of NULL. also, since i don't know what the code for the message hook is, i don't know what the message filter is doing. it may be possible that it is preventing it from exiting the loop. hopefully something above will do the trick, if not, maybe there's more code or info you could give that would help. good luck!

[anders43]

There are several ways to do this. It seems now you are relying on the parameter to contain the terminating flag or something. Another way to do this (the way I normally do it) is to create your own message which you post to your thread. When your message is caught by the message pump, just clean up and exit/return. The problem with your current approach is that it relies on that the caller/starter of your thread to terminate *after* the thread - but this may or may not be true since the thread lives its own life. The caller may have already started to clean up and terminate before the thread which then causes access to the parameter to throw access exceptions. Also there is then a risk the thread never ends since parent is gone.

hth

[scythe]

It works now. Thanks for your help, people!