Thread communication & thread safety example problem

Question

4.00/5 (1 vote)

See more:

I'm trying to write a class to deal with receiving a GPS signal over a serial connection. For the serial talk I use the CSerial class which so far I can warmly recommend.

The idea was to create a "listener" as part of a class constantly receiving incoming chatter. Then translate it to a "GPGGAMessage struct".

There are two issues which I am aware of with my current solution:

1. Communication with the thread:
In GPSCommunication::stopListening() I just set listeningThreadEnd = true which will cause to terminate the thread after one more while-loop.
It's important to terminate the thread before serialPort.Close() is called, so I just wait a second in between.
Kind of an unsafe solution, are there suggestions for safer ways to do this, i.e. better ways to communicate with the thread?

2. Thread safety:
So the only thing I will call manually regulary is the getCurrentGPGGAMessage() function. This accesses currentGPGGAMessage which is often changed from within the thread. What is the "clean" way to make sure no writing/reading occurs simultaneously?

class GPSCommunication {
	public:
		GPSCommunication();
		bool startListening();
		bool stopListening();
		GPGGAMessage getCurrentGPGGAMessage();

	private:
		CSerial serialPort;
		GPGGAMessage currentGPGGAMessage;
		bool listeningThreadEnd;
		CWinThread* pListeningThread;

		friend UINT listeningThread(LPVOID pClassInstance);
		bool readSerial(char* message);
                void translateGPGGA(char message[100]);
};

GPSCommunication::GPSCommunication()
{
	CSerial serialPort;
}

bool GPSCommunication::startListening()
{
	
	listeningThreadEnd = false;
	std::cout << "Try to open Serial Port ..." << std::endl;
	
	serialPort.Open(_T("COM1"));

	if (serialPort.IsOpen())
	{
		std::cout << "Serial Port Open" << std::endl;
		
		serialPort.SetMask();

		pListeningThread = AfxBeginThread(listeningThread, this);
		return true;
	}
	else
	{
		std::cout << "Serial Port NOT Open" << std::endl;
		Sleep(1000);
		return false;
	}
}

bool GPSCommunication::stopListening()
{	
	listeningThreadEnd = true;
	Sleep(1000);
	serialPort.Close();
	
	return true;
}

UINT listeningThread(LPVOID pClassInstance)
{	
	GPSCommunication* pInstance = (GPSCommunication*) pClassInstance;
	
	char  message[100];

	while(!(pInstance->listeningThreadEnd))
	{
		if (pInstance->readSerial(message)) 
		{
			pInstance->translateGPGGA(message);
		}
		Sleep(50);
	}

	std::cout << "listeningThread terminates"<< std::endl;
	return 0;
}

GPGGAMessage GPSCommunication::getCurrentGPGGAMessage()
{
    return currentGPGGAMessage;
}

Posted 30-May-11 20:13pm

tschoni

Add a Solution

2 solutions

Solution 2

NOTE: This is a reply to Timerbirds comment, posted as 'answer' simply to make code easier to read

That's the message about the memory leak. I never dealt with handle leaks before, so I'm not sure if, if it says 'memory leak', it can still be a handle leak. Can it?

Detected memory leaks!
Dumping objects ->
{170} client block at 0x0039FC48, subtype c0, 68 bytes long.
a CWinThread object at $0039FC48, 68 bytes long

Below see how I hoped to prevent the leak.
Allocate using new directive in the class constructor.
Deleting again in the deconstructor.

GPSComm::GPSComm()
{
    CWinThread* pListeningThread = new CWinThread;
}

GPSComm::~GPSComm()
{
    delete pListeningThread;
}

bool GPSComm::startListening()
{
        listeningThreadEnd = false;
        pListeningThread = AfxBeginThread(listeningThread, this, 0, 0, 
                              CREATE_SUSPENDED, NULL);
        pListeningThread->m_bAutoDelete = FALSE;
        pListeningThread->ResumeThread();

}

C#

bool GPSComm::stopListening()
{
    listeningThreadEnd = true;
    // Wait until listeningThread properly terminates
    WaitForSingleObject( pListeningThread->m_hThread, 5000 );
    return true;
}

C#

UINT listeningThread(LPVOID pClassInstance)
{
    GPSComm* pInstance = (GPSComm*) pClassInstance;

    while(!(pInstance->listeningThreadEnd))
    {
         // do stuff
    }
    return 0;
}

Posted 31-May-11 21:21pm

tschoni

Comments

Timberbird 1-Jun-11 4:05am

Oh I see. Looks like the problem is in CWinThread* being overridden and lost. Let's see: you create new CWinThread object (let's call it "extra" object) in your constructor and set your pListeningThread variable to point to this object: CWinThread* pListeningThread = new CWinThread; Then, in startListening() method, you call AfxBeginThread() and override value stored in pListeningThread with pointer returned by that function. Now, there are two objects residing in memory: extra object, created in the constructor (and not used anywhere else - pointer to that object is lost by that moment) and the one created by AfxBeginThread() - let's call it "system" object. Finally, in your destructor you call delete pListeningThread;, but pListeningThread points to system objects; memory occupied by system object is released, but your extra object still stays in memory, lost and lone, until your process ends :). So the answer is simple - do not create extra object, leave the constructor empty

tschoni 1-Jun-11 4:47am

it's as always very clear if someone explains it properly :)
thanks a lot for all your help!!!
all questions answered, everyone happy! (everyone being me)

Add a Solution

Add your solution here

Treat my content as plain text, not as HTML

Preview 0

…

Existing Members

Sign in to your account

...or Join us

Download, Vote, Comment, Publish.

Your Email
Password
Forgot your password?

Your Email
This email is in use. Do you need your password?
Optional Password

I have read and agree to the Terms of Service and Privacy Policy
Please subscribe me to the CodeProject newsletters

When answering a question please:

Read the question carefully.
Understand that English isn't everyone's first language so be lenient of bad spelling and grammar.
If a question is poorly phrased then either ask for clarification, ignore it, or edit the question and fix the problem. Insults are not welcome.
Don't tell someone to read the manual. Chances are they have and don't get it. Provide an answer or move on to the next question.

Let's work to help developers, not make them feel stupid.

This content, along with any associated source code and files, is licensed under The Code Project Open License (CPOL)

Timberbird · Accepted Answer · 2011-05-30T21:27:00

Solution 1

1. You can duplicate thread handle and use it with WaitForSingleObject() function as described here[^]. Generally (regarding other cases, when you just need to wait for some time - your listeningThread function, for example), using Sleep() is not recommended, consider using WaitForSingleObject with dummy handle (event never set user, for example) instead

2. Lock currentGPGGAMessage, obviously :). Use critical sections or mutexes to lock and unlock you shared object in any read/write operation. If possible, it would be good to rewrite your getCurrentGPGGAMessage() in the following manner: you lock currentGPGGAMessage object, for example using EnterCriticalSection, then create its copy, unlock object (LeaveCriticalSection) and return created copy as result. All operations modifying currentGPGGAMessage should also be synchronized, of course. This way you don't need to bother about your object and access to it anywhere outside your class

Posted 30-May-11 21:27pm

Timberbird

Comments

tschoni 31-May-11 22:25pm

Regarding 1:

The link provided explains the solution well. One thing that is discussed is too is because
Thread->m_bAutoDelete = FALSE;
is used the Thread handle needs to be deleted manually.
The way I use this in a class I thought the best way to do it is using new directive when creating the thread handle in the constructor and use delete directive in the deconstructor.
This however still results in a memory leak caused by the not properly deleted thread handle, why is this?

Why is it not recommended to use Sleep();? And how to easiest create a safe dummy handle?

Timberbird 1-Jun-11 2:51am

Is it memory leak or handle leak (number of handles used by your process increases on start/stop threads)? If latter, this is usually due to missing CloseHandle() function. To clarify things, could you please provide code for your thread object (create, start, wait, delete)?

I personally avoid Sleep() because of several limitations: actual amount of time it waits may slightly differ from the value you passed, and it makes your thread to skip current time slice no matter what (as described in MSDN ). On the other hand, I believe Sleep() requires less system resources so there are cases when its usage makes sense. A simple way to wait is to create an event (outside any loops that will wait for it, of course, to reduce resource consuming) and then call WaitForSingleObject() repeatedly:

HANDLE dummyHandle = CreateEvent(NULL, TRUE, FALSE, NULL);

while()
{
...

WaitForSingleObject(dummyHandle, AMOUNT_OF_TIME_YOU_NEED_TO_WAIT);
}

CloseHandle(dummyHandle);

This realization, by the way, comes very handy when you need to exit loop by external event, either system (caused by waitable object - thread, process, timer, console and so on) or coming from your another thread.

tschoni 1-Jun-11 3:22am

thanks for the elaborate reply, please note that I posted a 'answer' which is actually a reply to your comment ... just to make code more readable

tschoni 31-May-11 23:55pm

Regarding 2:

At first it was unclear to me what the difference between mutex'es and critical sections are.
A good discussion on the topic can be found here:
http://www.codeguru.com/forum/showthread.php?t=333192
Summarized: Use critical sections if resources are shared only within one process.

On how to use the critical secions: the simplest syntax (or the one that suits me most) I found in this example here - very straightforward
http://msdn.microsoft.com/en-us/library/ms686908%28v=vs.85%29.aspx