[C++/Win32] My thread class is not working

Started by
14 comments, last by the_edd 14 years, 3 months ago
I wrote down the class below for general threading needs in Win32 programs. Win32 API is written for C, and sometimes can have inconsistancies with CPP. For instance, the third parameter of the Win32 API function CreateThread() must be a pointer to a function in the form of "DWORD WINAPI FunctionName(LPVOID lpParameter);". However, if you decide to use this API function in a CPP class you encounter a problem. When I wrote down my threading class code for the first time, couldn't compile it. The compiler gave an irrelevent error message which wasn't describing the real problem in the code. After googling and comparing with sample codes I understood what the problem was. I learned that all C++ member functions had a hidden first parameter, which is nothing but a pointer to the class they are in. I didn't know how to deal with this hidden parameter, actually I had no idea, I was stuck. So I used sample codes I found on the net to write my own code. Every sample code has these common similarities: 1) Class method which will be passed to CreateThread() API function (in my code, it is the ThreadProc() method) is declared to be static. 2) The parameter passed to ThreadProc() is the pointer to the class itself (i.e.; the "this" pointer); 3) "this" pointer is passed to the fourth parameter of CreateThread(). My code is working if you define only one instance of the class. But it gives a runtime error if you define a second instance. I think, it has something to do with the static ThreadProc() method. Can anyone plase answer my questions about all these confusion: 1) Why do we have to define ThreadProc() static? What does it change? 2) I didn't understand the relation between every method having a hidden first parameter "this", and the fact that we passed "this" (via the fourth parameter of CreateThread()) as the first parameter. It already had a hidden "this", and we passed a second one. It made two "this"es, didn't it? Why did we need to pass a second "this"? What is the logic behind this? 3) How should I modify my code, so that I can declare multiple instances of my class without any runtime errors? (IDE : Visual Studio 2005 SP 1) GenericThread.cpp
#pragma once
#include <Windows.h>

class GenericThread
{
	public:
		GenericThread();
		~GenericThread();
		
		// Create and run the thread
		HANDLE StartThread(int nPriority = 0, bool create_suspended = false, SIZE_T stack_size = 0, bool reserve_stack = false);
		
		// This member function will run when you call StartThread()
		static DWORD WINAPI ThreadProc(void * lpParameter);
		
		// The run() method which will be called for threading
		virtual void run();
		
		// -15 : lowest; +15 : highest
		void SetThreadPriority(int nPriority);
		
		// Get currently assigned priority level
		int GetThreadPriority();
		
		// Get thread status
		bool IsThreadRunning();
		
		// Pause execution
		void SuspendThread();
		
		// Resume execution
		void ResumeThread();
		
		// Terminate execution
		void TerminateThread();
		
		// Return thread handle
		HANDLE GetThreadHandle();
		
	private:
		// Handle to the thread
		HANDLE m_hThread;
		
		// The thread id assigned by Windows, there is no use for now
		LPDWORD m_lpdwThreadID;
		
		// Thread status
		bool m_bRunning;
};
GenericThread.h
#include "GenericThread.h"

GenericThread::GenericThread()
{
	m_bExiting = false;
	m_bRunning = false;
}

GenericThread::~GenericThread()
{
	if (m_bRunning) CloseHandle(m_hThread);
}

HANDLE GenericThread::StartThread(int nPriority /*= 0*/, bool create_suspended /*= false*/, SIZE_T stack_size /*= 0*/, bool reserve_stack /*= false*/)
{
	DWORD dwCreationFlags = 0;
	if (create_suspended) dwCreationFlags |= CREATE_SUSPENDED;
	if (reserve_stack) dwCreationFlags |= STACK_SIZE_PARAM_IS_A_RESERVATION;
	m_hThread = CreateThread(NULL, stack_size, (LPTHREAD_START_ROUTINE) ThreadProc, (void *) this, dwCreationFlags, m_lpdwThreadID);
	SetThreadPriority(nPriority);
	m_bRunning = true;
	return m_hThread;
}

DWORD WINAPI GenericThread::ThreadProc(void * lpThis)
{
	((GenericThread *) lpThis)->run();
	return 0;
}

void GenericThread::run()
{

}

void GenericThread::SetThreadPriority(int nPriority)
{
	::SetThreadPriority(m_hThread, nPriority);
}

int GenericThread::GetThreadPriority()
{
	return ::GetThreadPriority(m_hThread);
}

bool GenericThread::IsThreadRunning()
{
	return m_bRunning;
}

void GenericThread::SuspendThread()
{
	::SuspendThread(m_hThread);
}

void GenericThread::ResumeThread()
{
	::ResumeThread(m_hThread);
}

void GenericThread::TerminateThread()
{
	CloseHandle(m_hThread);
	m_bRunning = false;
}

HANDLE GenericThread::GetThreadHandle()
{
	return m_hThread;
}
Advertisement
A static member function operates independently to any instance of a class. There's no implicit "this" argument. So as far as calling conventions go, it doesn't look any different to a regular C function.

However, since a static member function is indeed a member, it can access non-public members of instances of the class it belongs to.

This is why the pass a pointer-to-a-static-method is so common when writing thread classes.

Because a static member function has no "this" pointer available to it, data has to be passed in via the opaque parameter slot that CreateThread provides.

Other stuff:

0. Don't do this yourself, unless it's just for fun. There's already a defacto-standard C++ threading interface.

1. Make your thread class un-copyable. This is usually done by declaring a private copy constructor and assignment operator.

2. Don't terminate threads. In general, it's really bad. Your destructors won't run, resources will leak (including locks... deadlock ahoy). Instead, ask them to stop what they're doing and wait for them to finish.

3. Priority and suspend/resume functionality are almost always useless. I'd also have the threads you create "inherit" the stack size from their parent.

4. I think _beginthread[ex] is generally preferred to CreateThread as it interacts better with the C runtime library.
Thank you for these hand tips.

Quote:Original post by the_edd
0. Don't do this yourself, unless it's just for fun. There's already a defacto-standard C++ threading interface.
Actually, one of the main reasons is fun. I want to use my own code.

Quote:Original post by the_edd
1. Make your thread class un-copyable. This is usually done by declaring a private copy constructor and assignment operator.
If I use the singleton pattern I won't be able to create more than one instance of my class. I want to this class in big project, I need more than one instances of it.

Quote:Original post by the_edd
2. Don't terminate threads. In general, it's really bad. Your destructors won't run, resources will leak (including locks... deadlock ahoy). Instead, ask them to stop what they're doing and wait for them to finish.
Yeah, this is quite important, I am going to modify the code to prevent these possible leaks.

Quote:Original post by the_edd
3. Priority and suspend/resume functionality are almost always useless. I'd also have the threads you create "inherit" the stack size from their parent.
If so, that's bad.

Quote:Original post by the_edd
4. I think _beginthread[ex] is generally preferred to CreateThread as it interacts better with the C runtime library.
I saw that some of the examples are using _beginthread() rather than CreateThread(). But since I won't be using any C runtime libraries in my code, I think it will be just fine using CreateThread().
Quote:Original post by Battousai
Quote:Original post by the_edd
1. Make your thread class un-copyable. This is usually done by declaring a private copy constructor and assignment operator.
If I use the singleton pattern I won't be able to create more than one instance of my class. I want to this class in big project, I need more than one instances of it.

Preventing copying is not the same thing as preventing multiple instances. With a private copy constructor and assignment operator, you cannot make copies of the objects. But there is nothing with it that prevents multiple instances to be created.
What is the code for your "main" function that is creating the threads.
Considering I don't see any code there to wait for the thread to finish i have this advice:
Remember you need to wait in your main for the thread objects to finish, or you will get a crash! Expecially the way you have your destructors made up.
Thus it could be a fluke of timing that your program doesn't crash with only one thread.

Quote:Original post by KulSeran
What is the code for your "main" function that is creating the threads.
Considering I don't see any code there to wait for the thread to finish i have this advice:
Remember you need to wait in your main for the thread objects to finish, or you will get a crash! Expecially the way you have your destructors made up.
Thus it could be a fluke of timing that your program doesn't crash with only one thread.


main.cpp
#include <QtCore/QCoreApplication>#include "TestClassA.h"#include "TestClassB.h"int main(int argc, char *argv[]){	QCoreApplication application(argc, argv);		TestClassA a;	//TestClassB b;		a.SetThreadPriority(-15);	//b.SetThreadPriority(+15);		a.StartThread();	//b.StartThread();			return application.exec();}

I crashes if I uncomment those three lines above.
I used QT toolkit, so the program doesn't end up immediately.

TestClassA.cpp
#include "TestClassA.h"TestClassA::TestClassA(void){}TestClassA::~TestClassA(void){}void TestClassA::run(){	for (int i=0; i<=20; i++) std::cout << "Fat cat ate the rat." << std::endl;}


TestClassA.h
#pragma once#include "GenericThread.h"#include <string>#include <iostream>class TestClassA : public GenericThread{	public:		TestClassA();		~TestClassA();			private:		void run();};


"TestClassB" is exactly same with "TestClassA", except that it print a different string.
You are invoking CreateThread incorrectly. The last argument is an output parameter - a (optional) pointer to a DWORD where the thread ID will be stored.

In your code, you're passing in m_lpdwThreadID, which is an uninitialized pointer. You're getting lucky the first the time call is made, but not so much on the second call (undefined behaviour, bets are off etc ;])

Instead, define an actual DWORD member variable for the thread ID like so:
// The thread id assigned by Windows, there is no use for nowDWORD m_dwThreadID;


.. and call CreateThread with a pointer to this variable:
m_hThread = CreateThread(NULL, stack_size, (LPTHREAD_START_ROUTINE) ThreadProc, (void *) this, dwCreationFlags, &m_dwThreadID);
Quote:Original post by mattd
You are invoking CreateThread incorrectly. The last argument is an output parameter - a (optional) pointer to a DWORD where the thread ID will be stored.

In your code, you're passing in m_lpdwThreadID, which is an uninitialized pointer. You're getting lucky the first the time call is made, but not so much on the second call (undefined behaviour, bets are off etc ;])

Instead, define an actual DWORD member variable for the thread ID like so:
*** Source Snippet Removed ***

.. and call CreateThread with a pointer to this variable:
*** Source Snippet Removed ***


Wow, you were write, now my thread class is working just fine. I declared two objects from it, didn't crash and run both two threads simultanously.


Quote:Original post by KulSeran
Remember you need to wait in your main for the thread objects to finish, or you will get a crash!


Now, I want to add a join() method to my class, any ideas how?
Note 1. Since your threads aren't really using any data from your program, that explains why you don't see a crash from not waiting. Once you have a join function,
you should add code to the thread class destructor to join the thread and then close the handle. That way the thread object can't be destructed while the thread itself is running.

To "join" a thread, you want to look into adding a call to WaitForSingleObject( m_hThread )
. It will lock the calling thread untill the m_hThread handle is unlocked, which will only happen when the thread terminates. If the thread terminated long before your join call, then the wait call should immediately return. If the thread is locked up for some reason (actual lockup, waiting on some other mutex, or suspended), then the call will never return, so you may want to add a "try_join" type call that calls wait with a timeout.


If you want to synchronize with the thread for reasons other than the thread terminating, either create a mutex, or a critical section.
Quote:Original post by KulSeran
Note 1. Since your threads aren't really using any data from your program, that explains why you don't see a crash from not waiting. Once you have a join function,
you should add code to the thread class destructor to join the thread and then close the handle. That way the thread object can't be destructed while the thread itself is running.

To "join" a thread, you want to look into adding a call to WaitForSingleObject( m_hThread )
. It will lock the calling thread untill the m_hThread handle is unlocked, which will only happen when the thread terminates. If the thread terminated long before your join call, then the wait call should immediately return. If the thread is locked up for some reason (actual lockup, waiting on some other mutex, or suspended), then the call will never return, so you may want to add a "try_join" type call that calls wait with a timeout.


If you want to synchronize with the thread for reasons other than the thread terminating, either create a mutex, or a critical section.


Thank you very much for your helps, now I think my simple thread class is complete:

GenericThread.h
#ifndef _GENERIC_THREAD_H_#define _GENERIC_THREAD_H_#include <Windows.h>class GenericThread{	public:		GenericThread();		virtual ~GenericThread();				// Create and run the thread		HANDLE StartThread(int nPriority = 0, bool create_suspended = false, SIZE_T stack_size = 0, bool reserve_stack = false);				// This member function will run when you call StartThread()		static DWORD WINAPI ThreadProc(void * lpParameter);				// The run() method which will be called for threading		virtual void run();				// -15 : lowest; +15 : highest		void SetThreadPriority(int nPriority);				// Get currently assigned priority level		int GetThreadPriority();				// Get thread status		bool IsThreadRunning();				// WaitForSingleObject()		bool JoinThread();				// Make sure that the thread has joined		bool IsThreadJoined();				// Pause execution		void SuspendThread();				// Resume execution		void ResumeThread();				// Terminate execution		void TerminateThread();				// Return thread handle		HANDLE GetThreadHandle();			private:				// Handle to the thread		HANDLE m_hThread;				// The thread id assigned by Windows, there is no use for now		DWORD m_dwThreadID;				// Thread status		bool m_bRunning;				// Thread joined or not		bool m_bJoined;};#endif


GenericThread.cpp
#include "GenericThread.h"GenericThread::GenericThread(){	m_bRunning = false;	m_bJoined = false;}GenericThread::~GenericThread(){	JoinThread();	if (m_bRunning) CloseHandle(m_hThread);}HANDLE GenericThread::StartThread(int nPriority /*= 0*/, bool create_suspended /*= false*/, SIZE_T stack_size /*= 0*/, bool reserve_stack /*= false*/){	DWORD dwCreationFlags = 0;	if (create_suspended) dwCreationFlags |= CREATE_SUSPENDED;	//if (reserve_stack) dwCreationFlags |= STACK_SIZE_PARAM_IS_A_RESERVATION;	m_hThread = CreateThread(NULL, stack_size, (LPTHREAD_START_ROUTINE) ThreadProc, (void *) this, dwCreationFlags, &m_dwThreadID);	SetThreadPriority(nPriority);	m_bRunning = true;	return m_hThread;}DWORD WINAPI GenericThread::ThreadProc(void * lpThis){	((GenericThread *) lpThis)->run();	return 0;}void GenericThread::run(){}void GenericThread::SetThreadPriority(int nPriority){	::SetThreadPriority(m_hThread, nPriority);}int GenericThread::GetThreadPriority(){	return ::GetThreadPriority(m_hThread);}bool GenericThread::IsThreadRunning(){	return m_bRunning;}bool GenericThread::IsThreadJoined(){	return m_bJoined;}void GenericThread::SuspendThread(){	::SuspendThread(m_hThread);}void GenericThread::ResumeThread(){	::ResumeThread(m_hThread);}bool GenericThread::JoinThread(){	if (!m_bJoined)		if (WaitForSingleObject(m_hThread, INFINITE) == WAIT_FAILED)			return m_bJoined = false;		else			return m_bJoined = true;	return m_bJoined;}void GenericThread::TerminateThread(){	CloseHandle(m_hThread);	m_bRunning = false;}HANDLE GenericThread::GetThreadHandle(){	return m_hThread;}


This topic is closed to new replies.

Advertisement