What 's wrong with WaitForSingleObject

Question

This code below is the full source code of my program so that you can copy to VC++ and compile it to have ' quick visual' of the problem i have.

#include <windows.h>
#include <process.h>    /* _beginthread, _endthread */

#include <stddef.h>
#include <stdlib.h>
#include <string.h>
#include <math.h>
#include <stdio.h>
#include <crtdbg.h>
#include <conio.h>
#define PI 3.14159265

const
 int
 mutexCount=30;
HANDLE hMutex[mutexCount];
HANDLE hMutexLock;
unsigned
 PASCAL Bounce( void
 *pti );
unsigned
 PASCAL CheckKey( void
 *dummy );

/* GetRandom returns a random integer between min and max. */





#define GetRandom( min, max ) ((rand() % (int
)(((max) + 1) - (min))) + (min))
struct
 threadinfo
{
	short
 index;
	CHAR key;
	short
 threadcount;
};
BOOL repeat = TRUE;     /* Global repeat flag and video variable */

HANDLE hStdOut;         /* Handle for console window */

CONSOLE_SCREEN_BUFFER_INFO csbi;    /* Console information structure */






int
 main()
{
	CHAR    ch = 'A';
	CHAR s[100];
	hStdOut = GetStdHandle( STD_OUTPUT_HANDLE );
	/* Get display screen's text row and column information. */

	GetConsoleScreenBufferInfo( hStdOut, &csbi );
	printf("Enter some characters: "
);
	scanf("%s"
,s);
	int
 i=strlen(s);
	DWORD writeChar;
	COORD init={0,0};
	FillConsoleOutputCharacter(hStdOut,0x20,csbi.dwSize.X*csbi.dwSize.Y,init,&writeChar);
	threadinfo ti;int
 j=0;
	ti.threadcount=i;
	for
(int
 n=0;n<i;++n)
		hMutex[n]=CreateMutex(NULL,FALSE,NULL);
	/* Launch CheckKey thread to check for terminating keystroke. */

        _beginthreadex(NULL,0, CheckKey, NULL, 0,NULL );
	
	/* Loop until CheckKey terminates program. */

        while
( repeat )
	{
		/* On first loops, launch character threads. */

		if
(i==0)
			continue
;
		ti.index=j++;
		ti.key=s[--i];
		_beginthreadex(NULL,0, Bounce, (void
 *) &ti,0,NULL );

		/* Wait one second between loops. */

		Sleep( 1000L );
	}
	FillConsoleOutputCharacter(hStdOut,0x20,csbi.dwSize.X*csbi.dwSize.Y,init,&writeChar);
	printf("Press any key to exit...."
);
	_getch();
}

/* CheckKey - Thread to wait for a keystroke, then clear repeat flag. */





unsigned
 PASCAL CheckKey( void
*dummy )
{
	_getch();
	repeat = 0;    /* _endthread implied */

	_endthreadex(0);
	return
 0;

}

/* Bounce - Thread to create and and control a colored letter that moves
* around on the screen.
*
* Params: ch - the letter to be moved
*/





unsigned
 PASCAL Bounce( void
 *pti )
{
	/* Generate letter and color attribute from thread argument. */

	threadinfo thisthread=*(threadinfo*)pti;
	char
 blankcell = 0x20;
	char
 blockcell = thisthread.key;
	short
 index =thisthread.index;
	short
 limit=thisthread.threadcount;
	BOOL    first = TRUE;
	COORD   oldcoord, newcoord;
	DWORD   result;
	int
 i;
	_RPT1(_CRT_WARN,"Thread index %d starts\n"
,index);
	newcoord.X=0;
	newcoord.Y=10;
	result=1;
	while
( repeat )
	{
		/* Pause between loops. */

		Sleep( 100L );
		if
(newcoord.X==0)
			ReleaseMutex(hMutex[index]);//release its own

		//request thread's own mutex

		if
(newcoord.X>0 && result)
		{
			result=WaitForSingleObject(hMutex[index],INFINITE);
			_RPT1(_CRT_WARN,"Thread id %d acquires it own mutex\n"
,index);
			_ASSERTE(result==WAIT_OBJECT_0);
			result=0;
		}
		

		/* Blank out our old position on the screen, and draw new letter. */

		if
( first )
			first = FALSE;
		else

			WriteConsoleOutputCharacter( hStdOut, &blankcell, 1, oldcoord, &result );
		WriteConsoleOutputCharacter( hStdOut, &blockcell, 1, newcoord, &result );

		/* Increment the coordinate for next placement of the block. */

		oldcoord.X = newcoord.X;
		oldcoord.Y = newcoord.Y;
		
		++newcoord.X;
		if
(newcoord.X>csbi.dwSize.X)
		{
			
			Beep( (blockcell - 'A') * 100, 175 );
			if
(index==0)
				i=limit-1;
			else
 if
(index==limit-1)
			{
				BOOL success;
				success=ReleaseMutex(hMutex[index]);
				_ASSERTE(success);
				_RPT1(_CRT_WARN,"Thread id %d releases its mutex\n"
,index);
				i=index-1;
			} else

				i=index-1;
			_RPT2(_CRT_WARN,"Thread id %d is waiting for mutex %d\n"
,index,i);
			WaitForSingleObject(hMutex[i],INFINITE);
			ReleaseMutex(hMutex[i]);//release the mutex of other thread

			_RPT2(_CRT_WARN,"Thread id %d releases mutex %d\n"
,index,i);
			result=1;

                        newcoord.X=0;
		}
		newcoord.Y=10*sin((double
)2*PI/(double
)csbi.dwSize.X*(double
)newcoord.X)+10;

		
		
	}
	/* _endthread given to terminate */

	_endthreadex(0);
	return
 0;
}

I hope you wont be overwhelmed by this code, I post this so that you can use it to clarify the problem quickly. Actually, this is the code i modified from the example about _beginthread at http://msdn.microsoft.com/en-us/library/kdzttdcb%28VS.80%29.aspx because i want to get a better understanding of thread synchronization using mutex. When you run this code, it will ask you to enter sequence of characters. After that, a number of thread for controlling character's movement is created according to the string you enter. I want each thread (character thread)  to follow this rule:

• character in thread created later will follow character in thread created sooner
• each charater will move like the sine curve
• if a character go off the screen border, it will wait until all chars following it go off the screen border, too. And it will reset it position. This was done by this code

  if(newcoord.X>csbi.dwSize.X)
  {
  .....
  
  
  }

  You can see this in the first code block.

All character threads start with their own mutex. When thread index n go off screen, it will wait for mutex n-1 owned by thread n-1,with the exception that thread index 0 will wait for the last thread to go off screen. In order to prevent deadlock, when the last thread go off screen, it will release its mutex before it wait for the nearest index thread, so the thead index 0 which go off screen first will have a chance to grab this mutex and continue its movement. When thread 0 in postion x>0, it will release its own mutex so the second thread can grab it (and release immediately). This process is repeated for the third thread, and so on.
But when i ran this code, it never work. In the debugger output show that the problem appears when all characters go off the screen. What mistake could I make?. I need your help.

Answer 1

Note that after you pass a ti structure to a Bounce thread procedure, it is altered inside the while loop during creation of the next thread. Therefore threads do not work with correct data. Either create an array of threadinfo structures and pass to thread procedures an index, or allocate each threadinfo with new.

 

Declare repeat variable volatile since it is accessed by different threads.

 

Also check if the sequence of WaitForSingleObject and ReleaseMutex executed on the same hMutex[i] is correct.

Answer 2

hi, Viorel, thanks for reply. The problem you pointed out, i think i have already solved in my code. Here it is:
In the Bounce function:

threadinfo thisthread=*(threadinfo*)pti; 
char blankcell = 0x20; 
char blockcell = thisthread.key; 
short index =thisthread.index; 
short limit=thisthread.threadcount;

Because ti's members such as  key and index are subject to change, so i made a copy for each. In addition, in the while loop of the main thread:

while( repeat )
	{
		/* On first loops, launch character threads. */

		if(i==0)
			continue;
		ti.index=j++;
		ti.key=s[--i];
		_beginthreadex(NULL,0, Bounce, (void*) &ti,0,NULL );

		/* Wait one second between loops. */
		Sleep( 1000L );
	}

there is a sleep function that slows the process of making character thread, allowing each character thread to have a chance to copy ti's member to its own private variable.
The repeat variable in the example code provided at http://msdn.microsoft.com/en-us/library/kdzttdcb%28VS.80%29.aspx doesn't need to be volatile but still run properly

Also check if the sequence of WaitForSingleObject and ReleaseMutex executed on the same hMutex[i] is correct.

I also use _RPTx function to report the thread' activity so i dont think it is the problem.

Answer 3

Mutex objects are intended for coordinating mutually exclusive access to a shared resource. That is not the case i your application. I suppose mutex object somehow could be used anyhow. But I suggest you use event objects to make it less complicated:

...
ti.threadcount=i;
for (int n = 0; n < i; ++n )
   hGoal[n] = CreateEvent(NULL, FALSE, FALSE, NULL);
/* Launch CheckKey thread to check for terminating keystroke. */
_beginthreadex(NULL,0, CheckKey, NULL, 0,NULL );
...

unsigned PASCAL Bounce( void *pti )
{
   /* Generate letter and color attribute from thread argument. */
   threadinfo thisthread=*(threadinfo*)pti;
   char blankcell = 0x20;
   char blockcell = thisthread.key;
   short index = thisthread.index;
   short limit = thisthread.threadcount;
   BOOL first = TRUE;
   COORD oldcoord, newcoord;
   newcoord.X=0;
   newcoord.Y=10;
   while ( repeat )
   {
      Sleep( 100L );
      if ( first )
         first = FALSE;
      else
         WriteConsoleOutputCharacter( hStdOut, &blankcell, 1, oldcoord, &result );
      WriteConsoleOutputCharacter( hStdOut, &blockcell, 1, newcoord, &result );
      oldcoord.X = newcoord.X;
      oldcoord.Y = newcoord.Y;
      ++newcoord.X;
      if (newcoord.X >= csbi.dwSize.X)
      {
         if ( index == 0 )
         {
            WaitForSingleObject(hGoal[limit-1],INFINITE); // wait for the last one in line
            Sleep( 500L );
            SetEvent(hGoal[index]); // release number two in line and go
         }
         else if ( index == limit - 1 )
         {
            SetEvent(hGoal[index]); // I am last. Release the first one
            WaitForSingleObject(hGoal[limit-2],INFINITE); // wait for the one in front of me
            Sleep( 500L ); // wait and start again. No one after me to release
         }
         else
         {
            WaitForSingleObject(hGoal[index-1],INFINITE); // wait for the one in front of me
            Sleep( 500L );
            SetEvent(hGoal[index]); // release next in line and go
         }
         newcoord.X = 0;
     }
     newcoord.Y = 10*sin((double)2*PI/(double)csbi.dwSize.X*(double)newcoord.X)+10;
   }
   _endthreadex(0);
   return 0;
}

Answer 4

well, it works. But i still dont understand what are the mistakes in my code? Anyway, thank you,hgn.

Answer 5

Using "result" as switch here

if ( newcoord.X>0 && result)
{
   result=WaitForSingleObject(hMutex[index],INFINITE);
   _RPT1(_CRT_WARN,"Thread id %d acquires it own mutex\n" ,index);
   _ASSERTE(result==WAIT_OBJECT_0);
  result=0;
}

does not serve any purpose, as "result" is reset to one here:

if ( first )
  first = FALSE;
else
  WriteConsoleOutputCharacter( hStdOut, &blankcell, 1, oldcoord, &result );
WriteConsoleOutputCharacter( hStdOut, &blockcell, 1, newcoord, &result );

As a result WaitForSingleObject is executed many times without matching  ReleaseMutex (watch the output view!).

"A thread can specify a mutex that it already owns in a call to one of the wait functions without blocking its execution. This prevents a thread from deadlocking itself while waiting for a mutex that it already owns. However, to release its ownership, the thread must call ReleaseMutex one time for each time that it obtained ownership"

You can correct your code by using another DWORD for WriteConsoleOutputCharacter.

Answer 6

@hgn: I had seen this issue before i post this thread (i miss the debug's output window, should i?) but i didn't realize this was the reason for my problem so it didnt fix it. Now the mutex method works, but your method is of course better. your quote reminds me of one thing that is not new but never hurt to repeat: Read documentation carefully. Again, thank you so much.