[RESOLVED] I need thread sychronization help

[JohnBartle]

Skill Level: intermediate/novice C++ programmer
OS: Windows 10
IDE: Visual Studio Community 2017


Hello.

I don't know much about threaded programming, and I am trying to write a win32 program that uses threads. I have written a class called ''PickEnumerationThreadSynchronizer'' that is meant to control the synchronization of multiple methods.

There are two particular methods should be focused on:
1) synchronizePrimary()
2) synchronizeSecondary()


Within these methods synchronization is lost and my program sometimes permanently hangs. So THIS is the location of the problem.


The method synchronizePrimary() is used to synchronize the source of a vector of vector of strings.

The method synchronizeSecondary( int (*inFunction)(int&), int& parameter) is used to synchronize the users/clients of that vector of vector of strings. And inFunction() is the user/client

Q) How should I write my code so it doesn't hang???


(let me know if you need more to make sense out of it)
Here is my code:



PickEnumerationThreadSynchronizer.cpp
PickEnumeration.h
stdafx.h

[2kaud]

#include "PickEnumerationThreadSynchronizer.h"

PickEnumerationThreadSynchronizer::PickEnumerationThreadSynchronizer()
{
}

PickEnumerationThreadSynchronizer::PickEnumerationThreadSynchronizer(int inIntermediateTicketLimit) : intermediateTicketLimit(inIntermediateTicketLimit)
{
	currentPlayedTickets = 0;
	signalFullCapacityReached = false;

	signalAllUsersAreDone = (LONG*)_aligned_malloc(
		sizeof(LONG),
		32
	);

	*signalAllUsersAreDone = 0;

	totalTicketsPlayed = 0;
	synchronizedUsers = 0;
	isIntermediateTicketLimitFull = false;
}

PickEnumerationThreadSynchronizer::~PickEnumerationThreadSynchronizer()
{
	if (signalAllUsersAreDone)
	{
		_aligned_free(signalAllUsersAreDone);
	}
}

void PickEnumerationThreadSynchronizer::setTicketLimit(int limit)
{
	ticketLimit = limit;
}

bool PickEnumerationThreadSynchronizer::getIsIntermediateTicketLimitFull()
{
	return isIntermediateTicketLimitFull;
}

void PickEnumerationThreadSynchronizer::resetIsIntermediateTicketLimitFull()
{
	isIntermediateTicketLimitFull = false;
}

void PickEnumerationThreadSynchronizer::synchronizePrimary()
{
	totalTicketsPlayed++;
	currentPlayedTickets++;

	if ((currentPlayedTickets == intermediateTicketLimit) || (totalTicketsPlayed == ticketLimit))
	{
		isIntermediateTicketLimitFull = true;
		currentPlayedTickets = 0;
		signalFullCapacityReached = true;

		while (*signalAllUsersAreDone < synchronizedUsers)
		{
			Sleep(800);
		}
		signalFullCapacityReached = false;

		InterlockedExchange(signalAllUsersAreDone, 0);
		resetTotalTicketsPlayed(totalTicketsPlayed == ticketLimit);
	}
}

void PickEnumerationThreadSynchronizer::synchronizeSecondary(  int (*inFunction)(int&), int& parameter)
{
	int ticketsProcessed = 0;

	while (ticketsProcessed < ticketLimit)
	{
		while (signalFullCapacityReached == false)
		{
			Sleep(800);
		}

		ticketsProcessed = inFunction(parameter);

		InterlockedIncrement(signalAllUsersAreDone);

		while (*signalAllUsersAreDone > 0)
		{
			Sleep(800);
		}
	}
}

void PickEnumerationThreadSynchronizer::resetTotalTicketsPlayed(int doReset)
{
	if (doReset)
	{
		totalTicketsPlayed = 0;
	}
}

void PickEnumerationThreadSynchronizer::setSynchronizedUsers(int newValue)
{
	synchronizedUsers = newValue;
}

InterlockedIncrement(signalAllUsersAreDone);

		while (*signalAllUsersAreDone > 0)
		{
			Sleep(800);
		}

I'm having trouble understanding the logic here. You are incrementing *signalAllUsersAreDone and then spinning in a loop until *signalAllUsersAreDone is zero. The only other place that *signalAllUsersAreDone is changes is

while (*signalAllUsersAreDone < synchronizedUsers)
{
	Sleep(800);
}
signalFullCapacityReached = false;

InterlockedExchange(signalAllUsersAreDone, 0);

which sets it to 0 only when *signalAllUsersAreDone >= synchronizedUsers.

Also

while (signalFullCapacityReached == false)
		{
			Sleep(800);
		}

		ticketsProcessed = inFunction(parameter);

So inFunction() is only processed when signalFullCapacityReached is true??

my program sometimes permanently hangs.

I suspect the problem is that at least one of the spin loops doesn't terminate.

[JohnBartle]

You know, I just figured I would post the all of the code.


Here it is:

test5.cpp




PickEnumerationThreadSynchronizer.cpp
PickEnumeration.h
stdafx.h

[JohnBartle]

I suspect the problem is that at least one of the spin loops doesn't terminate.

Yeah, that's the problem. But I was wondering either why OR what other design could I use?

[2kaud]

Rather than using spin loops, I would be considering using events. See https://msdn.microsoft.com/en-us/lib...(v=vs.85).aspx for info re Wait Functions - especially single object wait functions (eg WaitForSingleObject() https://msdn.microsoft.com/en-us/lib...v=vs.85).aspx). Rather than have a spin loop, use WaitForSingleObject().

eg if you have created (see https://msdn.microsoft.com/en-us/lib...v=vs.85).aspx) an event say evtFullCap, then

if (WaitForSingleObject(evtFullCap, 5000) != WAIT_OBJECT_0) {
    //Error 
}
...

SetEvent(evtFullCap);

and the same for other spin loops using different event handles.

[JohnBartle]

Rather than using spin loops, I would be considering using events.

Okay, if you I think that will fix the problem then I'll try it. I was actually going to try that before I started this thread but I didn't think that would work

Thanks

[2kaud]

Okay, if you I think that will fix the problem then I'll try it. I was actually going to try that before I started this thread but I didn't think that would work

Thanks

It may not solve the problem by itself, but it makes the code a lot easier to understand and debug as it allows for 'error' conditions as per post #5. Also changing the code to use WaitForSingleObject() and SetEvent() will need some logic change.

I've had a quick look at the code when I've had time and I'm not sure I fully understand what you are trying to achieve. Perhaps as you suggest in post #1 you could provide more details as to what you are trying to accomplish.

Note that you potentially have a thread 'race' condition in that in primaryThread() you use

p.setTicketLimit(tickets_begin.size());
p.setSynchronizedUsers(2);

and in secondaryThread() you use both tickitLimit and synchronizedUsers which may not have been initialised correctly in primaryThread() before being used in secondayThread(). You need to sync the start of secondayThread() to be when p has been correctly initialised in primaryThread() so IMO I would have an event which is set in primaryThread() when P is correct and in secondayThread() use WaitForSingleObject() on that event before calling p.synchronizeSecondary.

[JohnBartle]

Perhaps as you suggest in post #1 you could provide more details as to what you are trying to accomplish

My original code was designed to generate large volumes of combinations of numbers incrementally.

1) primaryThread() is suppose to be the source of the numbers generated. The numbers are stored in a vector of vectors of strings

2) secondaryThread() and tertiaryThread() are the users/clients of the numbers generated by primaryThread()

3) I believe that I need threading to manage the incurred memory issues from generating such large volumes of numbers.

Note that you potentially have a thread 'race' condition in that in primaryThread()

My original code didn't have those particular methods in them and the problem still occurred. But I'm very glad that you pointed out this potential problem though. When I'm at my computer I'll check later to see how this race problem affects my program.

Thanks for catching this bug for me!

[2kaud]

while (signalFullCapacityReached == false)
		{
			Sleep(800);
		}

You may also have a code optimisation issue here. Depending upon how the compiler optimizes, it might generate code similar to

     mov ebx, signalFullCapacityReached
l1: test ebx, ebx
     jnz l2
     push 800
     call Sleep
     jmp l1
l2: ;loop terminated

In this case initially the contents of signalFullCapacityReached is loaded into eax and then the contents of eax is repeatedly compared to 0 with the loop only terminating when non zero. However as the contents of eax are only set once before the loop is started, even if the value of signalFullCapacityReached is changed, the new value doesn't get moved to eax. So either the loop terminates immediately if signalFullCapacityReached is non-zero or the loop never terminates.

To avoid this in spin loops, the variables used should be defined as volatile. This means that no optimisation is performed for these variables and there value is re-loaded for every use. In this case the generated code could be

l1:
     mov ebx, signalFullCapacityReached
     test ebx, ebx
     jnz l2
     push 800
     call Sleep
     jmp l1
l2: ;loop terminated

where the value of signalFullCapacity is used every time through the loop. But if events are used for signalling, you don't have spin loops so don't don't potentially have this issue.

[JohnBartle]

@2kaud

Thanks for your post #9. Now I understand at least one problem with my code.

I decided to rewrite some of my code to use WaitForMultipleObjects(), but my program now hangs on the WaitForMultipleObjects() functions. Would you, or anyone who else for that matter, happen to know what the problem is now?


Here is my new code:

PickEnumerationThreadSynchronizer2.cpp
PickEnumeration.h
test5_2.cpp
stdafx.h





Thanks

[2kaud]

SetEvent(eventsBegin[inThreadNumber]);

		WaitForMultipleObjects(
			numberOfEvents,           // number of objects in array
			eventsBegin,     // array of objects
			TRUE,       // wait for any object
			INFINITE);

Note that your comment is incorrect. TRUE means wait for all objects to be signalled, not any. See https://msdn.microsoft.com/en-us/lib...(v=vs.85).aspx Hence this code means wait for all eventsBegin objects to be signalled before Wait...() returns.

Also I would suggest that you don't use INFINITE but a value such as 5000 so that you get a WAIT_TIMEOUT return value which can be tested. You then know which Wait...() is causing the problem.

[2kaud]

My original code was designed to generate large volumes of combinations of numbers incrementally.

1) primaryThread() is suppose to be the source of the numbers generated. The numbers are stored in a vector of vectors of strings

2) secondaryThread() and tertiaryThread() are the users/clients of the numbers generated by primaryThread()

3) I believe that I need threading to manage the incurred memory issues from generating such large volumes of numbers.

So you have a producer of numbers and one or more consumers of these numbers? Once the consumer(s) have used the numbers then they are no longer required? As there are a large volume of numbers to be processed you need to restrict the memory used?

In this case I would consider investigating a thread-safe circular buffer. There's various info available on the internet.

[wolle]

I don't know much about threaded programming, and I am trying to write a win32 program that uses threads.

C++ has native support for multi-threading since version 11 (so it's available in VS 2017).

The advantage of using standard C++ rather than OS specifics is that both the programs you write and the skills you acquire become portable. Also the trend is less on controlling threads and more on solving problems efficiently and the C++ supported concurrency is developing in that direction.

Here's a popular book (soon in a second edition),

https://www.manning.com/books/c-plus...ency-in-action

The first two chapters can be downloaded for free. It's enough to get started with the basics.

[JohnBartle]

In my last coded example, which uses WaitForMultipleObjects(), it very much seems as if WaitForMultipleObjects() sets all of the waiting and signaled event objects to non-signaled once just one of the WaitForMultipleObjects() functions returns first. In other words, even if all of the events are signaled only one WaitForMultipleObjects() will actually return -- because THAT instance of WaitForMultipleObjects() will set all of the events to non-signaled.


This behavior seems wrong, and I don't suppose that I read about it in MS documentation. (EDIT: Yeah, after taking another look I saw that the documentation does state that WaitForMultipleObjects() modifies synchronization objects after all. )

Does anyone know what is going on with WaitForMultipleObjects() ?

Also, does anyone know how I should deal with this behavior ?




Thanks

[JohnBartle]

C++ has native support for multi-threading since version 11 (so it's available in VS 2017).

Oh yeah, I know. But my program is thoroughly integrated with Windows api so portability isn't even a consideration.

Here's a popular book (soon in a second edition),

https://www.manning.com/books/c-plus...ency-in-action

Thanks for the recommendation. The book actually seems pretty good. I'll keep this in mind. I have a large list of programming books that I plan on studying so I don't know when I'll get to your recommended book. Also, right now I only needed to make use of a tiny bit of concurrent programming - so I wasn't wanting to take the time and effort to go in depth at this moment.