Sign in to follow this  
hkBattousai

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

Recommended Posts

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;
}

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites
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().

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites
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 now
DWORD 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);

Share this post


Link to post
Share on other sites
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?

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites
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;
}


Share this post


Link to post
Share on other sites
Just a few quick notes:

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.

Boost threads is one of these libraries in Boost that somehow leaves you scratching your head. The designers of that library made some really strange design decisions, that make some real world MT systems very awkward and inefficient to implement. The heavily Unix-biased choice of synchronisation primitives is also very problematic under Windows (eg. conditions variables, which are non-native on XP, and the lack of events, which are native and commonly encountered throughout Windows). The technical details of all this are a different topic altogether, but suffice to say that writing your own threading library is actually rather common, mainly due to Boost:thread's weirdness. It is, however, rather non-trivial and requires a very good understanding of MT concepts.

Quote:
Original post by the_edd
3. Priority and suspend/resume functionality are almost always useless.

Actually, lowering thread priority is very useful. I would even go as far as saying that priority support is vital for a threading library.

Share this post


Link to post
Share on other sites
Quote:
Original post by Yann L
Boost threads is one of these libraries in Boost that somehow leaves you scratching your head. The designers of that library made some really strange design decisions, that make some real world MT systems very awkward and inefficient to implement. The heavily Unix-biased choice of synchronisation primitives is also very problematic under Windows (eg. conditions variables, which are non-native on XP, and the lack of events, which are native and commonly encountered throughout Windows).

Efficient implementations of the primitives in boost threads are possible on Windows (though the Windows API might not be used) else C++ wouldn't be standardising on it. Of course, possible != available, necessarily.

Quote:
The technical details of all this are a different topic altogether, but suffice to say that writing your own threading library is actually rather common, mainly due to Boost:thread's weirdness. It is, however, rather non-trivial and requires a very good understanding of MT concepts.


I tend to find the the Windows threading functionality far stranger e.g. the naming of CRITICAL_SECTION, the way you have to bend over backwards to use WaitForMultipleObjects with more than MAXIMUM_WAIT_OBJECTS handles, the special meanings for Sleep() with special values as arguments. To each their own though, I guess.

Quote:
Actually, lowering thread priority is very useful. I would even go as far as saying that priority support is vital for a threading library.

I can't say I've ever found the need to do that. I tend to design things so that either a thread is doing some work that I want done ASAP or waiting (idling) for that work to arrive.

Share this post


Link to post
Share on other sites
Battousai: if I'm getting your class to thread mapping concept right, then m_bRunning is not correctly reset if the thread ends itself.

Quote:
Original post by the_edd
Efficient implementations of the primitives in boost threads are possible on Windows (though the Windows API might not be used) else C++ wouldn't be standardising on it.

Implementation details, especially platform specific ones, were not even considered by the C++ standarization committee.

Quote:
Original post by the_edd
Of course, possible != available, necessarily.

It is impossible to efficiently implement condition variables on pre-Vista, since they are unsupported by the kernel. A good CV implementation on XP (ie. one that will never deadlock and is well balanced in all situations), relying on semaphores and mutexes, is surprisingly complex and dead inefficient. Have a look at how Boost::threads implements it.

While native kernel CVs exist in Vista and above, their use will lead to code that behaves radically differently on XP, performance wise.

Quote:
Original post by the_edd
I tend to find the the Windows threading functionality far stranger e.g. the naming of CRITICAL_SECTION, the way you have to bend over backwards to use WaitForMultipleObjects with more than MAXIMUM_WAIT_OBJECTS handles, the special meanings for Sleep() with special values as arguments. To each their own though, I guess.

I agree that Windows threading is a bit bizarre. But Windows representing 90%+ of the PC market, it should be the primary driving platform for standarization. Especially for a concept which is so OS dependent as threading.

Quote:
Original post by the_edd
I can't say I've ever found the need to do that. I tend to design things so that either a thread is doing some work that I want done ASAP or waiting (idling) for that work to arrive.

There are many scenarios where lowering the priority can be useful. It gives valuable hints to the scheduler on how to assign time slices amongst a set of candidate threads. An example would be a realtime 3D application that requires entirely smooth motion, with a background network thread that will periodically use a lot of CPU. Lowering the latter threads' priority can help to avoid sudden glitches or lags in the animation thread. It is also very important for single CPU systems, or multi-CPU scenarios with fixed core affinities.

Anyway, this is probably a bit offtopic :)

Share this post


Link to post
Share on other sites
Quote:
Original post by Yann L
Quote:
Original post by the_edd
Efficient implementations of the primitives in boost threads are possible on Windows (though the Windows API might not be used) else C++ wouldn't be standardising on it.

Implementation details, especially platform specific ones, were not even considered by the C++ standarization committee.

Surely they have to be? If the unofficial motto for C++ is that "you don't pay for what you don't need" and the threading functionality isn't widely implementable at minimal cost, then it wouldn't (and probably shouldn't IMVHO) be standardized.

Quote:

It is impossible to efficiently implement condition variables on pre-Vista, since they are unsupported by the kernel. A good CV implementation on XP (ie. one that will never deadlock and is well balanced in all situations), relying on semaphores and mutexes, is surprisingly complex and dead inefficient. Have a look at how Boost::threads implements it.

Is that even true given that an implementation can be written in assembler? I'm genuinely interested. I don't know enough about the kernel/user-space boundaries here.

Quote:

I agree that Windows threading is a bit bizarre. But Windows representing 90%+ of the PC market, it should be the primary driving platform for standarization. Especially for a concept which is so OS dependent as threading.

Windows is 90% of the desktop market. It is run on nowhere near 90% of the machines that can be targeted by C++.

EDIT: to clarify, I was taking issue with the "primary driving platform" part. I agree that implementation details for a platform as widespread as Windows should be considered.

Quote:

Quote:
Original post by the_edd
I can't say I've ever found the need to do that. I tend to design things so that either a thread is doing some work that I want done ASAP or waiting (idling) for that work to arrive.

There are many scenarios where lowering the priority can be useful. It gives valuable hints to the scheduler on how to assign time slices amongst a set of candidate threads.

These hints are also OS-specific though, no? I mean XP vs Vista, not just Windows vs non-Windows.

Quote:
An example would be a realtime 3D application that requires entirely smooth motion, with a background network thread that will periodically use a lot of CPU.


If you mean realtime in the stricter sense, then I can see how that would be useful. But on your common or garden desktop OS, I would use select()/WaitForMultipleObjects() for such a thing. The handling code is short and sharp and creates a response that is thrown at a thread pool or task queue.

Quote:

Lowering the latter threads' priority can help to avoid sudden glitches or lags in the animation thread.

I think this is where the crux of my "designs" differ; I tend not to give any thread one particular responsibility. I think anything that helps you think in terms of tasks rather than threads can only be a good thing.

Quote:
Anyway, this is probably a bit offtopic :)

Indeed, this will be my last. You now have a free rein on rebuttals :)

[Edited by - the_edd on December 27, 2009 9:41:20 PM]

Share this post


Link to post
Share on other sites
Quote:
Original post by Yann L
Battousai: if I'm getting your class to thread mapping concept right, then m_bRunning is not correctly reset if the thread ends itself.

That's one to-do, you are right. I have to write a few lines for it.

Quote:
Original post by Yann L
Anyway, this is probably a bit offtopic :)

I enjoy good debates even if they are off topic. It is good to know all of these.


I want to improve my class to obtain maximum performance on multi-core CPU systems. Do you have any idea how?

Share this post


Link to post
Share on other sites
Quote:
Original post by Battousai
I want to improve my class to obtain maximum performance on multi-core CPU systems. Do you have any idea how?


Your class won't be a bottleneck. The act of creating a thread should occur rarely.

I tend to create a pool of threads once (of size equal to boost::thread::hardware_concurrency()) and have everything go through that.

Designing performance in to threaded code from the outset is hard. The two basic things you can do are 1. make sure that you hold any locks you have for the shortest time possible and 2. avoid false sharing.

These articles are a good place to start reading about the remaining intricacies.

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this