• Announcements

    • khawk

      Download the Game Design and Indie Game Marketing Freebook   07/19/17

      GameDev.net and CRC Press have teamed up to bring a free ebook of content curated from top titles published by CRC Press. The freebook, Practices of Game Design & Indie Game Marketing, includes chapters from The Art of Game Design: A Book of Lenses, A Practical Guide to Indie Game Marketing, and An Architectural Approach to Level Design. The GameDev.net FreeBook is relevant to game designers, developers, and those interested in learning more about the challenges in game development. We know game development can be a tough discipline and business, so we picked several chapters from CRC Press titles that we thought would be of interest to you, the GameDev.net audience, in your journey to design, develop, and market your next game. The free ebook is available through CRC Press by clicking here. The Curated Books The Art of Game Design: A Book of Lenses, Second Edition, by Jesse Schell Presents 100+ sets of questions, or different lenses, for viewing a game’s design, encompassing diverse fields such as psychology, architecture, music, film, software engineering, theme park design, mathematics, anthropology, and more. Written by one of the world's top game designers, this book describes the deepest and most fundamental principles of game design, demonstrating how tactics used in board, card, and athletic games also work in video games. It provides practical instruction on creating world-class games that will be played again and again. View it here. A Practical Guide to Indie Game Marketing, by Joel Dreskin Marketing is an essential but too frequently overlooked or minimized component of the release plan for indie games. A Practical Guide to Indie Game Marketing provides you with the tools needed to build visibility and sell your indie games. With special focus on those developers with small budgets and limited staff and resources, this book is packed with tangible recommendations and techniques that you can put to use immediately. As a seasoned professional of the indie game arena, author Joel Dreskin gives you insight into practical, real-world experiences of marketing numerous successful games and also provides stories of the failures. View it here. An Architectural Approach to Level Design This is one of the first books to integrate architectural and spatial design theory with the field of level design. The book presents architectural techniques and theories for level designers to use in their own work. It connects architecture and level design in different ways that address the practical elements of how designers construct space and the experiential elements of how and why humans interact with this space. Throughout the text, readers learn skills for spatial layout, evoking emotion through gamespaces, and creating better levels through architectural theory. View it here. Learn more and download the ebook by clicking here. Did you know? GameDev.net and CRC Press also recently teamed up to bring GDNet+ Members up to a 20% discount on all CRC Press books. Learn more about this and other benefits here.
Sign in to follow this  
Followers 0
Tispe

Calling a virtual function, illegal call of non-static member function?

25 posts in this topic

Hello

 

I am writing a class that spawns and communicates with a worker thread. The class is going to be Polymorphic such that derived classes can implement their own work functions. This way I can have the thread-safe communication interface (mutex and STL containers) in the WorkerThread class.

class WorkerThread
{
public:
	WorkerThread(void);
	~WorkerThread(void);
private:
	HANDLE hThread;
	static DWORD WINAPI ThreadRoutine(LPVOID param);
	virtual void ThreadFunction(LPVOID param) = 0;     //implemented in derived classes
};
DWORD WINAPI WorkerThread::ThreadRoutine(LPVOID param)
{
	ThreadFunction(param);		//error C2352: 'WorkerThread::ThreadFunction' : illegal call of non-static member function
	return 0;
}

Is this bad? Should I instead pass an object which has its own work function (derived from a base class)? But then I would have to pass some interface to the object.

DWORD WINAPI WorkerThread::ThreadRoutine(LPVOID param)
{
	(somecastings)param->ThreadFunction(/*InterfaceToComm*/);
	return 0;
}
0

Share this post


Link to post
Share on other sites

Should I instead pass an object which has its own work function (derived from a base class)?

Yes.

 

 

But then I would have to pass some interface to the object.

No.

DerivedWorkerThread *object = new DerivedWorkerThread("some interface");

CreateThread(...., WorkerThread::ThreadRoutine, (LPVOID)object);
1

Share this post


Link to post
Share on other sites

In order to completely encapsulate this in a simple class the thread function has to be static. But now the static function can't call a member function.

 

Perhaps this is a viable workaround:

class WorkerThread
{
public:
	WorkerThread(void);
	~WorkerThread(void);

	virtual void ThreadFunction(LPVOID param) = 0;
private:
	HANDLE hThread;
	static DWORD WINAPI ThreadRoutine(LPVOID param);
};
 
WorkerThread::WorkerThread(void)
{
	hThread = CreateThread(NULL, 0, WorkerThread::ThreadRoutine, this, NULL, NULL);
}

DWORD WINAPI WorkerThread::ThreadRoutine(LPVOID param)
{
	static_cast<WorkerThread*>(param)->ThreadFunction(param);
	return 0;
}
Edited by Tispe
1

Share this post


Link to post
Share on other sites

I noticed that the compiler now allows for the virtual function to be private, which I think is odd. But I like it since now it is not exposed to the client code.

class WorkerThread
{
public:
WorkerThread(void);
~WorkerThread(void);
private:
virtual void ThreadFunction(LPVOID param) = 0;
HANDLE hThread;
static DWORD WINAPI ThreadRoutine(LPVOID param);
};
0

Share this post


Link to post
Share on other sites

In this code the constructor creates a thread and passes "this" as a parameter to it. So the ThreadRoutine recieves an object, but how does it know that the object it recieves is its own instance and can start accessing the private members? That's why I think its Odd.

class WorkerThread
{
public:
WorkerThread(void);
~WorkerThread(void);
private:
virtual void ThreadFunction(LPVOID param) = 0;
HANDLE hThread;
static DWORD WINAPI ThreadRoutine(LPVOID param);
int Test;
};
WorkerThread::WorkerThread(void)
{
hThread = CreateThread(NULL, 0, WorkerThread::ThreadRoutine, this, NULL, NULL);
}

DWORD WINAPI WorkerThread::ThreadRoutine(LPVOID param)
{
static_cast(param)->ThreadFunction(param);    //But this is a private function, why is it allowed to be called here?
return 0;
}

Then would it not be pointless to pass anything to ThreadFunction? Because it can directly access all private members anyway? But this does not work out:

class Derived : WorkerThread{
	void ThreadFunction(LPVOID param)
	{
		static_cast<Derived*>(param)->Test = 123;	//If this is Valid:
		Test = 456;					//Then this should also be valid?
	}							//But both gives: error C2248: 'WorkerThread::Test' : cannot access private member declared in class 'WorkerThread'
}; 

I'm puzzled...

Edited by Tispe
0

Share this post


Link to post
Share on other sites

Thank you. I got the basics working. Will implement thread safe communication asap. Meanwhile, you can have a look at my code:

#pragma once
#include <windows.h>
#include <iostream>
class WorkerThread
{
public:
	WorkerThread(DWORD i):m_quit(false)
	{
		ID = i;
		std::cout << "Constructor called. ID: " << ID << std::endl;
		hThread = CreateThread(NULL, 0, WorkerThread::ThreadRoutine, this, NULL, NULL);
	}
	~WorkerThread(void)
	{
		std::cout << "Exiting thread ID: " << ID << std::endl;
		m_quit = true;
		TerminateThread(hThread, 0);		//unsafe?
	}

protected:
	bool m_quit;
	DWORD ID;
	virtual void ThreadFunction() = 0;     //implemented in derived classes

private:
	HANDLE hThread;
	static DWORD WINAPI ThreadRoutine(LPVOID param)
	{
		static_cast<WorkerThread*>(param)->ThreadFunction();
		return 0;
	}
};

class DerivedA : public WorkerThread
{
public:
	DerivedA(DWORD i) : WorkerThread(i)
	{
	}

private:
	void ThreadFunction()
	{
		while(!m_quit){
			std::cout << "Inside Loop of Derived class A. Thread ID: " << ID << std::endl;
			Sleep(1000+ID*32);
			/*
			Lock mutex
			Get Data
			Unlock mutex

			Computation

			Lock mutex
			Send Data
			Unlock mutex
			*/

		}
	}
}; 
#include <iostream>
#include <vector>
#include <memory>
#include "WorkerThread.h"

int main(){
	std::vector<std::shared_ptr<WorkerThread> > ThreadPool;
	SYSTEM_INFO sysinfo;
	GetSystemInfo( &sysinfo );

	while(true){
	std::cout << "Creating " << sysinfo.dwNumberOfProcessors << " Threads in 5 seconds!" << std::endl;
	Sleep(5000);

	for(DWORD i=0;i<sysinfo.dwNumberOfProcessors;i++)
	{
		ThreadPool.push_back(std::make_shared<DerivedA>(i) );
	}
	std::cout << "Pool size: " << ThreadPool.size() << std::endl;
	Sleep(20000);

	std::cout << ">>>>Clearing Threads<<<<" << std::endl;
	ThreadPool.clear();
	std::cout << ">>>>Clearing Complete<<<<" << std::endl;
	std::cout << "Pool size: " << ThreadPool.size() << std::endl;
	Sleep(5000);
	}

	return 0;
} 

...

 

 

...

0

Share this post


Link to post
Share on other sites

I'm wondering how I can gracefully exit/terminate a thread without sending it a message to break its main loop.

 

Using ThreadPool.clear(); all destructors are called and the member m_quit is set true. But if TerminateThread() is not called, the thread will keep on going, because m_quit is no longer in memory, since the object is released. So where m_quit used to be, the thread will read that address and keep on going with its while loop.

 

But, if I use TerminateThread() then a mutex may remain unlocked or some other members could remain in some undefined state if the thread was working with those members at the time it was terminated. 

 

What happens if I DeleteCriticalSection() on a locked mutex? What if the thread has some pending IO with Win32 during termination? Or it's in the middle of a large memcpy? What other non-blocking safe ways are there for terminating a thread?

 

What if the thread gets the shared_ptr to itself, which is only deleted when it exits. That way the thread object will remain in memory until it can finish and clean up?

Edited by Tispe
0

Share this post


Link to post
Share on other sites

I have not figured out how/if instances of this WorkerThread can deallocate themselves after being cleared from the ThreadPool. Until I resolve this I guess I have to send a message telling the worker function to break its loop and clean up. But I have a KillThread method just in case.

 

Any comments or suggestions for this code?

#pragma once
class WorkerThread
{
public:
	WorkerThread(DWORD i) : m_Quit(false), m_Shutdown(false)
	{
		InitializeCriticalSection(&Request_Mutex);
		InitializeCriticalSection(&Completed_Mutex);
		hThread = CreateThread(NULL, 0, WorkerThread::ThreadRoutine, this, NULL, NULL);
	}

	~WorkerThread(void)
	{
		if(hThread)
		{
			CloseHandle(hThread);
		}

		DeleteCriticalSection(&Request_Mutex);
		DeleteCriticalSection(&Completed_Mutex);
	}

	void TickMain();	//Only called by Client code
	bool SendRequest(std::shared_ptr<InstructionBase> spNewRequest);
	bool GetCompleted(std::shared_ptr<InstructionBase> &spNewRequest);
	void KillWorkerThread();

protected:
	bool m_Quit, m_Shutdown;
	virtual void ThreadFunction() = 0;

	//Functions called by ThreadFunction()
	void ThreadGet();
	void ThreadSend();

	std::queue<std::shared_ptr<InstructionBase> > Thread_Requests;
	std::vector<std::shared_ptr<InstructionBase> > Thread_Completed;

private:
	//Functions called by TickMain()
	void MainSend();
	void MainGet();

	//Main side containers
	std::vector<std::shared_ptr<InstructionBase> > Main_Requests;
	std::queue<std::shared_ptr<InstructionBase> > Main_Completed;

	//In between containers and mutex
	CRITICAL_SECTION Request_Mutex;
	CRITICAL_SECTION Completed_Mutex;
	std::vector<std::shared_ptr<InstructionBase> > Mutex_Requests;
	std::vector<std::shared_ptr<InstructionBase> > Mutex_Completed;

	//Thread system
	HANDLE hThread;
	static DWORD WINAPI ThreadRoutine(LPVOID param)
	{
		static_cast<WorkerThread*>(param)->ThreadFunction();
		return 0;
	}
};

class DerivedA : public WorkerThread
{
public:
	DerivedA(DWORD i) : WorkerThread(i)
	{
	}

private:
	void ThreadFunction()
	{
		//Thread variables

		while(!m_Quit){
			void ThreadGet();

			if(Thread_Requests.empty()){
				Sleep(1);
			} else 	{
				std::shared_ptr<InstructionBase> spNewRequest = Thread_Requests.front();
				Thread_Requests.pop();

				//Do work on spNewRequest

				Thread_Completed.push_back(spNewRequest);				
			}

			void ThreadSend();
		}
	}
}; 
#include "WorkerThread.h"

void WorkerThread::KillWorkerThread()
{
	m_Quit = true;
	m_Shutdown = true;
	if(hThread != NULL)
	{
		Sleep(50);
		TerminateThread(hThread, 0);
		hThread = NULL;
	}
}

bool WorkerThread::SendRequest(std::shared_ptr<InstructionBase> spNewRequest)
{
	if(m_Quit || m_Shutdown || hThread == NULL){
		return false;
	}

	Main_Requests.push_back(spNewRequest);
	return true;
}

bool WorkerThread::GetCompleted(std::shared_ptr<InstructionBase> &spNewRequest)
{
	if(Main_Completed.empty()){
		return false;
	}

	spNewRequest = Main_Completed.front();
	Main_Completed.pop();

	return true;
}

void WorkerThread::TickMain()
{
	MainSend();
	MainGet();
}

void WorkerThread::MainSend()
{
	if(Main_Requests.empty()){
		return;
	}

	if(TryEnterCriticalSection(&Request_Mutex)){
		for(size_t i=0;i<Main_Requests.size();i++){
			Mutex_Requests.push_back(Main_Requests[i]);
		}

		LeaveCriticalSection(&Request_Mutex);
		Main_Requests.clear();								
	}
}

void WorkerThread::MainGet()
{
	if(TryEnterCriticalSection(&Completed_Mutex)){
		for(size_t i=0;i<Mutex_Completed.size();i++){
			Main_Completed.push(Mutex_Completed[i]);
		}

		Mutex_Completed.clear();
		LeaveCriticalSection(&Completed_Mutex);
	}
}

void WorkerThread::ThreadSend()
{
	if(Thread_Completed.empty()){
		return;
	}

	if(TryEnterCriticalSection(&Completed_Mutex)){
		for(size_t i=0;i<Thread_Completed.size();i++){
			Mutex_Completed.push_back(Thread_Completed[i]);
		}

		LeaveCriticalSection(&Completed_Mutex);
		Thread_Completed.clear();
	}
}

void WorkerThread::ThreadGet()
{
	if(TryEnterCriticalSection(&Request_Mutex)){
		for(size_t i=0;i<Mutex_Requests.size();i++){
			Thread_Requests.push(Mutex_Requests[i]);
		}

		Mutex_Requests.clear();
		LeaveCriticalSection(&Request_Mutex);
	}
} 
0

Share this post


Link to post
Share on other sites

Just to note, you have a few places in your code where you're declaring functions, rather than calling them (e.g. void ThreadGet(); and void ThreadSend(); in ThreadFunction()). Also, you are reading and writing to the quit variables across multiple threads without synchronisation.

 

Seeing as you're using std::shared_ptr, have you considered using std::thread, etc? If not, look at that design to see how this issue was resolved.

0

Share this post


Link to post
Share on other sites

 

I noticed that the compiler now allows for the virtual function to be private, which I think is odd. But I like it since now it is not exposed to the client code.


It's not "odd" at all (and it's not just "now"; C++ has always allowed that). There's one commonly-held belief that all virtual methods should always be private and that the polymorphic behavior of well-encapsulated objects should be an implementation detail of the public interface (though not for "abstract interface" types, of course).

It's handy, too. Even if you have a particular method you know will always need to be overridden in its entirety, having a small non-virtual public wrapper makes it easier to do logging, set breakpoints that capture all calls to the interface, etc. I don't think it should be done in all cases like some people do, but it's good practice in general.

 

Can you explain that a bit more? My brain is clearly not working too well this morning and I'm having trouble understanding what you mean.

0

Share this post


Link to post
Share on other sites

I noticed that the compiler now allows for the virtual function to be private, which I think is odd. But I like it since now it is not exposed to the client code.


It's not "odd" at all (and it's not just "now"; C++ has always allowed that). There's one commonly-held belief that all virtual methods should always be private and that the polymorphic behavior of well-encapsulated objects should be an implementation detail of the public interface (though not for "abstract interface" types, of course).

It's handy, too. Even if you have a particular method you know will always need to be overridden in its entirety, having a small non-virtual public wrapper makes it easier to do logging, set breakpoints that capture all calls to the interface, etc. I don't think it should be done in all cases like some people do, but it's good practice in general.

Can you explain that a bit more? My brain is clearly not working too well this morning and I'm having trouble understanding what you mean.

Search "non-virtual public interface". You make your public functions non-virtual, but have them call private virtual functions. Those function can then be implemented be child classes, but are only called though the public interface functions in the parent.
1

Share this post


Link to post
Share on other sites

Just to note, you have a few places in your code where you're declaring functions, rather than calling them (e.g. void ThreadGet(); and void ThreadSend(); in ThreadFunction()).

 

Right, the "void" followed with copy/paste smile.png

 

 

 


Also, you are reading and writing to the quit variables across multiple threads without synchronisation.

Would it be better if I swapped out "bool m_Quit" with "std::atomic<bool> m_Quit"?

 

 

 


Seeing as you're using std::shared_ptr, have you considered using std::thread, etc? If not, look at that design to see how this issue was resolved.

 

I'm not sure how I can encapsulate the thread with its comm-interface such that I can clear it from ThreadPool and have the object clean itself up. I don't want to join the thread. I simply want an object that has a thread inside I can submit and retrieve work to/from. And it should be Polymorphic such that derived classes can implement their own work functions.

 

Getting run-time error, the std::thread is complaining about a pure virtual function.

class WorkerThread
{
public:
	WorkerThread(DWORD i) : m_Quit(false), m_Shutdown(false)
	{
		InitializeCriticalSection(&Request_Mutex);
		InitializeCriticalSection(&Completed_Mutex);
		Thread = std::thread(ThreadRoutine, this);
	}

	~WorkerThread(void)
	{
		m_Quit = true;
		Thread.detach();
	}

	void TickMain();	//Only called by Client code
	bool SendRequest(std::shared_ptr<InstructionBase> spNewRequest);
	bool GetCompleted(std::shared_ptr<InstructionBase> &spNewRequest);

protected:
	std::atomic<bool> m_Quit, m_Shutdown;
	virtual void ThreadFunction() = 0;

	//Functions called by ThreadFunction()
	void ThreadGet();
	void ThreadSend();
	void CleanUp()
	{
		DeleteCriticalSection(&Request_Mutex);
		DeleteCriticalSection(&Completed_Mutex);
	}

	std::queue<std::shared_ptr<InstructionBase> > Thread_Requests;
	std::vector<std::shared_ptr<InstructionBase> > Thread_Completed;

private:
	//Functions called by TickMain()
	void MainSend();
	void MainGet();

	//Main side containers
	std::vector<std::shared_ptr<InstructionBase> > Main_Requests;
	std::queue<std::shared_ptr<InstructionBase> > Main_Completed;

	//In between containers and mutex
	CRITICAL_SECTION Request_Mutex;
	CRITICAL_SECTION Completed_Mutex;
	std::vector<std::shared_ptr<InstructionBase> > Mutex_Requests;
	std::vector<std::shared_ptr<InstructionBase> > Mutex_Completed;

	//Thread system
	std::thread Thread;
	static DWORD WINAPI ThreadRoutine(LPVOID param)
	{
		static_cast<WorkerThread*>(param)->ThreadFunction();
		static_cast<WorkerThread*>(param)->CleanUp();
		return 0;
	}
};

class DerivedA : public WorkerThread
{
public:
	DerivedA(DWORD i) : WorkerThread(i)
	{
	}

private:
	void ThreadFunction()
	{
		//Thread variables

		while(!m_Quit){
			ThreadGet();

			if(Thread_Requests.empty()){
				Sleep(1);
			} else 	{
				std::shared_ptr<InstructionBase> spNewRequest = Thread_Requests.front();
				Thread_Requests.pop();

				//Do work on spNewRequest

				//If detatched in the middle of this loop
				//Then all these members become invalid

				Thread_Completed.push_back(spNewRequest);				
			}

			ThreadSend();
		}
	}
};
bool WorkerThread::SendRequest(std::shared_ptr<InstructionBase> spNewRequest)
{
	if(m_Quit || m_Shutdown){
		return false;
	}

	Main_Requests.push_back(spNewRequest);
	return true;
}

bool WorkerThread::GetCompleted(std::shared_ptr<InstructionBase> &spNewRequest)
{
	if(Main_Completed.empty()){
		return false;
	}

	spNewRequest = Main_Completed.front();
	Main_Completed.pop();

	return true;
}

void WorkerThread::TickMain()
{
	MainSend();
	MainGet();
}

void WorkerThread::MainSend()
{
	if(Main_Requests.empty()){
		return;
	}

	if(TryEnterCriticalSection(&Request_Mutex)){
		for(size_t i=0;i<Main_Requests.size();i++){
			Mutex_Requests.push_back(Main_Requests[i]);
		}

		LeaveCriticalSection(&Request_Mutex);
		Main_Requests.clear();								
	}
}

void WorkerThread::MainGet()
{
	if(TryEnterCriticalSection(&Completed_Mutex)){
		for(size_t i=0;i<Mutex_Completed.size();i++){
			Main_Completed.push(Mutex_Completed[i]);
		}

		Mutex_Completed.clear();
		LeaveCriticalSection(&Completed_Mutex);
	}
}

void WorkerThread::ThreadSend()
{
	if(Thread_Completed.empty()){
		return;
	}

	if(TryEnterCriticalSection(&Completed_Mutex)){
		for(size_t i=0;i<Thread_Completed.size();i++){
			Mutex_Completed.push_back(Thread_Completed[i]);
		}

		LeaveCriticalSection(&Completed_Mutex);
		Thread_Completed.clear();
	}
}

void WorkerThread::ThreadGet()
{
	if(TryEnterCriticalSection(&Request_Mutex)){
		for(size_t i=0;i<Mutex_Requests.size();i++){
			Thread_Requests.push(Mutex_Requests[i]);
		}

		Mutex_Requests.clear();
		LeaveCriticalSection(&Request_Mutex);
	}
}
Edited by Tispe
0

Share this post


Link to post
Share on other sites

I am trying a new approach to this whole thing. Instead of having the interface and the thread handling in the same object, I am seperating the interface into its own class.

 

The WorkerThread class will now only create a new thread and pass the address of a void pointer to the new thread. The new thread will create an interface and attach it to the passed **void.

 

Unfortunatly I get some errors: error C2664: 'std::_Ptr_base<_Ty>::_Reset0' : cannot convert parameter 1 from 'void *' to 'ThreadSafeCommunication *' 

#pragma once
#include <memory>
#include <vector>
#include <queue>
#include <atomic>
#include <thread>
#include <Windows.h>

struct InstructionBase
{
	int test;
};

#define spInstructionBase std::shared_ptr<InstructionBase>
class ThreadSafeCommunication
{
public:
	ThreadSafeCommunication() : m_Quit(false)
	{
		InitializeCriticalSection(&Request_Mutex);
		InitializeCriticalSection(&Completed_Mutex);
	}

	~ThreadSafeCommunication(void)
	{
		DeleteCriticalSection(&Request_Mutex);
		DeleteCriticalSection(&Completed_Mutex);
	}

	//Only called by Client code
	void Shutdown();
	void TickMain();	
	bool MainSendRequest(spInstructionBase NewRequest);
	bool MainGetCompleted(spInstructionBase &NewRequest);

	//Only called by Thread code
	bool Running();
	void TickThread();	
	bool ThreadGetRequest(spInstructionBase &NewRequest);
	bool ThreadSendCompleted(spInstructionBase NewRequest);

private:
	std::atomic<bool> m_Quit;

	//Main side functions and containers
	void MainSend();
	void MainGet();	
	std::vector<spInstructionBase> Main_Requests;
	std::queue<spInstructionBase> Main_Completed;

	//In between containers and mutex
	CRITICAL_SECTION Request_Mutex;
	CRITICAL_SECTION Completed_Mutex;
	std::vector<spInstructionBase> Mutex_Requests;
	std::vector<spInstructionBase> Mutex_Completed;

	//Thread side functions and containers
	void ThreadGet();
	void ThreadSend();
	std::queue<spInstructionBase> Thread_Requests;
	std::vector<spInstructionBase> Thread_Completed;
};


void ThreadFunction(void **pInterface);

class WorkerThread
{
public:
	WorkerThread()
	{
		Thread = std::thread(ThreadFunction, &pInterface);
	}

	~WorkerThread(void)
	{
		static_cast<std::shared_ptr<ThreadSafeCommunication> >(pInterface)->Shutdown();
		Thread.detach();
	}

	void *pInterface;

private:
	std::thread Thread;
};
void ThreadFunction(void **pInterface)
{
	std::shared_ptr<ThreadSafeCommunication> spNewInterface(new ThreadSafeCommunication); 
	*pInterface = (void*)&spNewInterface;

	while(spNewInterface->Running())
	{
		spNewInterface->TickThread();
		//Do stuff
	}
}
#include "Comm.h"
#include <Windows.h>
#include <memory>

void ThreadSafeCommunication::Shutdown()
{
	m_Quit = true;
}

bool ThreadSafeCommunication::Running()
{
	return !m_Quit;
}

bool ThreadSafeCommunication::MainSendRequest(std::shared_ptr<InstructionBase> spNewRequest)
{
	if(m_Quit){
		return false;
	}

	Main_Requests.push_back(spNewRequest);
	return true;
}

bool ThreadSafeCommunication::MainGetCompleted(std::shared_ptr<InstructionBase> &spNewRequest)
{
	if(Main_Completed.empty()){
		return false;
	}

	spNewRequest = Main_Completed.front();
	Main_Completed.pop();

	return true;
}

bool ThreadSafeCommunication::ThreadGetRequest(std::shared_ptr<InstructionBase> &spNewRequest)
{
	if(Thread_Requests.empty()){
		return false;
	}
	spNewRequest = Thread_Requests.front();
	Thread_Requests.pop();

	return true;
}

bool ThreadSafeCommunication::ThreadSendCompleted(std::shared_ptr<InstructionBase> spNewRequest)
{
	if(m_Quit){
		return false;
	}
	
	Thread_Completed.push_back(spNewRequest);
	return true;
}

void ThreadSafeCommunication::TickMain()
{
	MainSend();
	MainGet();
}

void ThreadSafeCommunication::TickThread()
{
	ThreadSend();
	ThreadGet();
}

void ThreadSafeCommunication::MainSend()
{
	if(Main_Requests.empty()){
		return;
	}

	if(TryEnterCriticalSection(&Request_Mutex)){
		for(size_t i=0;i<Main_Requests.size();i++){
			Mutex_Requests.push_back(Main_Requests[i]);
		}

		LeaveCriticalSection(&Request_Mutex);
		Main_Requests.clear();								
	}
}

void ThreadSafeCommunication::MainGet()
{
	if(TryEnterCriticalSection(&Completed_Mutex)){
		for(size_t i=0;i<Mutex_Completed.size();i++){
			Main_Completed.push(Mutex_Completed[i]);
		}

		Mutex_Completed.clear();
		LeaveCriticalSection(&Completed_Mutex);
	}
}

void ThreadSafeCommunication::ThreadSend()
{
	if(Thread_Completed.empty()){
		return;
	}

	if(TryEnterCriticalSection(&Completed_Mutex)){
		for(size_t i=0;i<Thread_Completed.size();i++){
			Mutex_Completed.push_back(Thread_Completed[i]);
		}

		LeaveCriticalSection(&Completed_Mutex);
		Thread_Completed.clear();
	}
}

void ThreadSafeCommunication::ThreadGet()
{
	if(TryEnterCriticalSection(&Request_Mutex)){
		for(size_t i=0;i<Mutex_Requests.size();i++){
			Thread_Requests.push(Mutex_Requests[i]);
		}
		
		Mutex_Requests.clear();
		LeaveCriticalSection(&Request_Mutex);
	}
} 
0

Share this post


Link to post
Share on other sites

You can pass a lambda to std::thread. I've not used C++11 extensively, I hope I'm using it correctly, I believe it should be something like this:

// Quick way to avoid changing lots of code
bool TryEnterCriticalSection(std::mutex *mutex) {
    return mutex->try_lock();
}
 
// Quick way to avoid changing lots of code
void LeaveCriticalSection(std::mutex *mutex) {
    mutex->unlock();
}
 
struct InstructionBase
{
    int test;
};
 
// No need to #define this
typedef std::shared_ptr<InstructionBase> spInstructionBase;
 
class ThreadSafeCommunication
{
public:
    ThreadSafeCommunication() : m_Quit(false)
    {
    }
 
    ~ThreadSafeCommunication()
    {
    }
 
    //Only called by Client code
    void Shutdown();
    void TickMain();    
    bool MainSendRequest(spInstructionBase NewRequest);
    bool MainGetCompleted(spInstructionBase &NewRequest);
 
    //Only called by Thread code
    bool Running();
    void TickThread();    
    bool ThreadGetRequest(spInstructionBase &NewRequest);
    bool ThreadSendCompleted(spInstructionBase NewRequest);
 
private:
    std::atomic<bool> m_Quit;
 
    //Main side functions and containers
    void MainSend();
    void MainGet();    
    std::vector<spInstructionBase> Main_Requests;
    std::queue<spInstructionBase> Main_Completed;
 
    //In between containers and mutex
    std::mutex Request_Mutex;
    std::mutex Completed_Mutex;
    std::vector<spInstructionBase> Mutex_Requests;
    std::vector<spInstructionBase> Mutex_Completed;
 
    //Thread side functions and containers
    void ThreadGet();
    void ThreadSend();
    std::queue<spInstructionBase> Thread_Requests;
    std::vector<spInstructionBase> Thread_Completed;
};
 
 
void ThreadFunction(std::shared_ptr<ThreadSafeCommunication> pInterface);
 
class WorkerThread
{
public:
    WorkerThread() : pInterface(new ThreadSafeCommunication)
    {
        Thread = std::thread([&] {
            ThreadFunction(pInterface);
        });
    }
 
    ~WorkerThread()
    {
        pInterface->Shutdown();
        Thread.detach();
    }
 
    std::shared_ptr<ThreadSafeCommunication> pInterface;
 
private:
    std::thread Thread;
};
 
void ThreadFunction(std::shared_ptr<ThreadSafeCommunication> pInterface)
{
    while(pInterface->Running())
    {
        pInterface->TickThread();
        //Do stuff
    }
}
 
void ThreadSafeCommunication::Shutdown()
{
    m_Quit = true;
}
 
bool ThreadSafeCommunication::Running()
{
    return !m_Quit;
}
 
bool ThreadSafeCommunication::MainSendRequest(std::shared_ptr<InstructionBase> spNewRequest)
{
    if(m_Quit){
        return false;
    }
 
    Main_Requests.push_back(spNewRequest);
    return true;
}
 
bool ThreadSafeCommunication::MainGetCompleted(std::shared_ptr<InstructionBase> &spNewRequest)
{
    if(Main_Completed.empty()){
        return false;
    }
 
    spNewRequest = Main_Completed.front();
    Main_Completed.pop();
 
    return true;
}
 
bool ThreadSafeCommunication::ThreadGetRequest(std::shared_ptr<InstructionBase> &spNewRequest)
{
    if(Thread_Requests.empty()){
        return false;
    }
    spNewRequest = Thread_Requests.front();
    Thread_Requests.pop();
 
    return true;
}
 
bool ThreadSafeCommunication::ThreadSendCompleted(std::shared_ptr<InstructionBase> spNewRequest)
{
    if(m_Quit){
        return false;
    }
    
    Thread_Completed.push_back(spNewRequest);
    return true;
}
 
void ThreadSafeCommunication::TickMain()
{
    MainSend();
    MainGet();
}
 
void ThreadSafeCommunication::TickThread()
{
    ThreadSend();
    ThreadGet();
}
 
void ThreadSafeCommunication::MainSend()
{
    if(Main_Requests.empty()){
        return;
    }
 
    if(TryEnterCriticalSection(&Request_Mutex)){
        for(size_t i=0;i<Main_Requests.size();i++){
            Mutex_Requests.push_back(Main_Requests[i]);
        }
 
        LeaveCriticalSection(&Request_Mutex);
        Main_Requests.clear();                                
    }
}
 
void ThreadSafeCommunication::MainGet()
{
    if(TryEnterCriticalSection(&Completed_Mutex)){
        for(size_t i=0;i<Mutex_Completed.size();i++){
            Main_Completed.push(Mutex_Completed[i]);
        }
 
        Mutex_Completed.clear();
        LeaveCriticalSection(&Completed_Mutex);
    }
}
 
void ThreadSafeCommunication::ThreadSend()
{
    if(Thread_Completed.empty()){
        return;
    }
 
    if(TryEnterCriticalSection(&Completed_Mutex)){
        for(size_t i=0;i<Thread_Completed.size();i++){
            Mutex_Completed.push_back(Thread_Completed[i]);
        }
 
        LeaveCriticalSection(&Completed_Mutex);
        Thread_Completed.clear();
    }
}
 
void ThreadSafeCommunication::ThreadGet()
{
    if(TryEnterCriticalSection(&Request_Mutex)){
        for(size_t i=0;i<Mutex_Requests.size();i++){
            Thread_Requests.push(Mutex_Requests[i]);
        }
        
        Mutex_Requests.clear();
        LeaveCriticalSection(&Request_Mutex);
    }
} 

I've used std::mutex instead of CRITICAL_SECTION. Also note the use of typedef over #define. I haven't examined the code in detail for multi-threaded correctness.

0

Share this post


Link to post
Share on other sites

It seems that passing a shared_ptr by reference is not thread safe. But passing by value is.

0

Share this post


Link to post
Share on other sites

Yes, passing a reference to anything to another thread is risky. You're racing the work the thread is doing against the lifetime of the object. For example, there is danger with the code I posted - imagine the WorkerThread object gets destroyed before the helper thread gets a chance to start.

 

The following is probably safer:

std::shared_ptr<ThreadSafeCommunication> copy = pInterface;
Thread = std::thread([copy] {
    ThreadFunction(copy);
});

You could probably drop the thread as a member, just have it as a local in the constructor and detach it immediately.

0

Share this post


Link to post
Share on other sites

I think I figured out a better way, which is C++03 compliant using std::enable_shared_from_this. It eliminates the need for std::atomic<bool> m_Quit by using the std::shared_ptr::use_count() instead, which is thread safe.

 

What do you think?

class ThreadSafeCommunication : public std::enable_shared_from_this<ThreadSafeCommunication>
{
[...]
};
class WorkerThread
{
public:
	WorkerThread(DWORD i)
	{
		spInterface = std::make_shared<ThreadSafeCommunication>(i);
		hThread = CreateThread(NULL, 0, WorkerThread::ThreadRoutine, spInterface.get(), NULL, NULL);
	}

	~WorkerThread(void)
	{
		if(hThread)
		{
			CloseHandle(hThread);
		}
	}

private:
	std::shared_ptr<ThreadSafeCommunication> spInterface;
	
	//Thread system
	HANDLE hThread;
	static DWORD WINAPI ThreadRoutine(LPVOID param)
	{
		std::shared_ptr<ThreadSafeCommunication> spInterface = static_cast<ThreadSafeCommunication*>(param)->shared_from_this();
		
		while(spInterface.use_count() == 2)		//Only 2 strong references, one from main and one from thread.
		{
			spInterface->TickThread();

			//do stuff
		}

		return 0;
	}
};
Edited by Tispe
0

Share this post


Link to post
Share on other sites


I think I figured out a better way, which is C++03 compliant using std::enable_shared_from_this.

I'm not sure what you mean, std::shared_ptr is itself part of C++11. Is C++11 not an option for you? Personally, I'd prefer to use the portable standard library over system specific code where possible.

 


What do you think?

You've reintroduced the race condition I was trying to address earlier.

 

I would be wary of a hard coded test for a use_count() of exactly two. You do not appear to have made your WorkerThread class noncopyable, so it is possible that you could end up with a higher value if you were doing something like passing it to a function by value, for example you might create one and add it to a container (though doing so would cause the thread handle to be closed multiple times). I would probably use something like while(!spInterface.unique()) instead.

0

Share this post


Link to post
Share on other sites


I'm not sure what you mean, std::shared_ptr is itself part of C++11. Is C++11 not an option for you? Personally, I'd prefer to use the portable standard library over system specific code where possible.

 

Sorry, I mixed up tr1 and c++03. Visual Studio 2010 implements tr1, not c++11.

 


You've reintroduced the race condition I was trying to address earlier.

Where is the race condition? will WorkerThread( const WorkerThread& ); fix it?

0

Share this post


Link to post
Share on other sites

The race is between the lifetime of the WorkerThread and the start of the ThreadRountine. If the WorkerThread object was destroyed before the ThreadRoutine gets to create a copy of the shared_ptr, then you lose!

 

One way to avoid it might be like so:

class ThreadSafeCommunication
{
    // ...
    std::shared_ptr<ThreadSafeCommunication> self;
};
 
class WorkerThread
{
    WorkerThread()
    {
        spInterface = std::make_shared<ThreadSafeCommunication>();
        spInterface->self = spInterface;
        hThread = CreateThread(NULL, 0, WorkerThread::ThreadRoutine, spInterface.get(), NULL, NULL);
    }
 
    // ...
 
private:
 
    // ...
 
    static DWORD WINAPI ThreadRoutine(LPVOID param)
    {
        ThreadSafeCommunication *communication = static_cast<ThreadSafeCommunication*>(param)
        std::shared_ptr<ThreadSafeCommunication> spInterface;
        std::swap(spInterface, communication->self);
 
        while(!spInterface.unique())
        {
            // ... 
        }
 
        return 0;
    }
};

This scheme will keep the ThreadSafeCommunication instance alive even if the WorkerThread has been destroyed in the mean time.

Edited by rip-off
1

Share this post


Link to post
Share on other sites

I see. I suppose this will also work?

	static DWORD WINAPI ThreadRoutine(LPVOID lpParam)
	{
		std::shared_ptr<ThreadSafeCommunication> spInterface = static_cast<ThreadSafeCommunication*>(lpParam)->self;
		static_cast<ThreadSafeCommunication*>(lpParam)->self.reset();

		while(!spInterface.unique())
		{
			spInterface->TickThread();
			//do stuff
		}

		return 0;
	}

I also tried to template the whole thing by swapping out std::shared_ptr<InstructionBase> with T. But all hell breaks loose on me. The linker starts complaining about undefined external. Something about CTRP I think. Because I template a class which is inside another templated class. It is in the ThreadRoutine when the T is passed down I can't T->TickThread();

 

ThreadSafeCommunication<T>

WorkerThread<T>

 

Is it possible to template out the std::shared_ptr<InstructionBase> as it stands now?

WorkerThread(DWORD i, LPTHREAD_START_ROUTINE lpStartAddress)
	{
		spInterface = std::make_shared<ThreadSafeCommunication<std::shared_ptr<InstructionBase> > >(i);
		spInterface->self = spInterface;
		hThread = CreateThread(NULL, 0, lpStartAddress, spInterface.get(), NULL, NULL);
	}
0

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  
Followers 0