🚀 go-pugleaf

"SubZero" <g_ates@hotmail.com> wrote in message 
news:4633777f$1@newsgroups.borland.com...

> void inline __fastcall lockCriticalSection(TCriticalSection 
> *criticalSection)
> {
>  HANDLE hMutex;
>
>  // Create a mutex with no initial owner.
>
>  hMutex = CreateMutex(
>   NULL,                       // no security attributes
>   true,                      // initially owned
>   String((int)(void*)criticalSection).c_str());  // name of mutex
>
>  DWORD dwWaitResult;
>
>  // Request ownership of mutex.
>
>  if(hMutex)
>  {
>   dwWaitResult = WaitForSingleObject(
>   hMutex,   // handle to mutex
>   5000L);   // five-second time-out interval

Perhaps I am mistaken, but it looks to me like the problem is obvious...your 
comment says, "Create a mutex with no initial owner", but the code creates 
it owned (conflicts with the comment).  Then you attempt to wait for the 
mutex, and 5 seconds later, due to timeout, the exception is thrown.

- Dennis 

SubZero wrote:

>which does not work for no obvious reason. It simply raises the exception 
>when there is no reason!

What exception would that be?

Why would you want to use _both_ mutex and CriticalSection ?

I understand you want a timeout, so you use mutex, but that should be
enough.

I think it would be better to have a global handle for the mutex so
you aren't constantly creating and opening the mutex (which you don't
use CloseHandle on, so are leaking resources).

Your lockCriticalSection() calls CreateMutex() with true, which is not
correct for this. Think what happens if 2 threads (or applications)
try to lock at the same time.  "Create with ownership" should fail for
one of them.

Furthermore:
<help>
The thread that owns a mutex can specify the same mutex in repeated
wait function calls without blocking its execution. Typically, you
would not wait repeatedly for the same mutex, but this mechanism
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 once for each time that the mutex satisfied a
wait. 
</help>
What this means is your CreateMutex(true) satisfies one "ownership",
and the wait() satisfies a second, but you only release once.

IMO you should CreateMutex(false) in application startup, with
CloseHandle() at shutdown.
Then the Lock() and and Release() would just use the global mutex
handle.
BTW, since your code should not reach releaseLock() without having
aquired a lock, releaseLock() should not wait(), but simply
ReleaseMutex().

Dennis Jones wrote:

>Perhaps I am mistaken, but it looks to me like the problem is obvious...your 
>comment says, "Create a mutex with no initial owner", but the code creates 
>it owned (conflicts with the comment).  Then you attempt to wait for the 
>mutex, and 5 seconds later, due to timeout, the exception is thrown.

The CreateMutex(true) returns with ownership.
The wait() satisfies with pre-ownership to eliminate deadlock.
Drawback is now the mutex is owned twice and should be released twice.

Ok. The original functions will be used with only TCriticalSection (single
line). The reason for all the code is that it is needed for detecting the
deadlock. I want to raise an exception so that MadExcept can show the stack!
Hope you understand now. Sorry for the confusion.

I cannot create the mutexes at startup because there are countless
criticalsections. It must work within these two functions with named
mutexes.

Best Regards,

SZ

"Bob Gonder" <notbg@notmindspring.invalid>, iletisinde sunu yazdi, 
news:aev6339dmkoh5ih93feqeofs77v59f6iic@4ax.com...
> SubZero wrote:
>
>>which does not work for no obvious reason. It simply raises the exception
>>when there is no reason!
>
> What exception would that be?
>
> Why would you want to use _both_ mutex and CriticalSection ?
>
> I understand you want a timeout, so you use mutex, but that should be
> enough.
>
> I think it would be better to have a global handle for the mutex so
> you aren't constantly creating and opening the mutex (which you don't
> use CloseHandle on, so are leaking resources).
>
> Your lockCriticalSection() calls CreateMutex() with true, which is not
> correct for this. Think what happens if 2 threads (or applications)
> try to lock at the same time.  "Create with ownership" should fail for
> one of them.
>
> Furthermore:
> <help>
> The thread that owns a mutex can specify the same mutex in repeated
> wait function calls without blocking its execution. Typically, you
> would not wait repeatedly for the same mutex, but this mechanism
> 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 once for each time that the mutex satisfied a
> wait.
> </help>
> What this means is your CreateMutex(true) satisfies one "ownership",
> and the wait() satisfies a second, but you only release once.
>
> IMO you should CreateMutex(false) in application startup, with
> CloseHandle() at shutdown.
> Then the Lock() and and Release() would just use the global mutex
> handle.
> BTW, since your code should not reach releaseLock() without having
> aquired a lock, releaseLock() should not wait(), but simply
> ReleaseMutex().
>
>
> 

SubZero wrote:

>Ok. The original functions will be used with only TCriticalSection (single
>line). The reason for all the code is that it is needed for detecting the
>deadlock. I want to raise an exception so that MadExcept can show the stack!
>Hope you understand now. Sorry for the confusion.
>
>I cannot create the mutexes at startup because there are countless
>criticalsections. It must work within these two functions with named
>mutexes.

You still need to call CreateMutex with false, not true.
You still need to CloseHandle for every CreateMutex and OpenMutex.

Suggested outline:
whatever lockCritSec( whatever)
{
	HANDLE	 M;
	BOOL	opened = true;
	M = OpenMutex( flag, false, name);
	if( M == NULL )
	{	// create only if needed (first time)
		opened = false;
		M = CreateMutex( flag, false, name );	
		if( M == NULL )
		{handle failure and return};
	};
	switch( WaitForSingleObject( M, 5000 ) )
	{case WAIT_TIMEOUT:
		/*whatever */
	default:
		break;
	};
	if( opened )
		CloseHandle( M );
}
whatever releaseCritSec( whatever)
{
	HANDLE	 M;
	M = OpenMutex( flag, false, name);
	if( M == NULL )
	{handle failure and return};

	// note that there is no wait() nor aquire at this point,
	// we should already own the mutex
	// and we only want to release it.

	ReleaseMutex( M );
	CloseHandle( M );
}

This way, at least you only have one dangling handle for each CritSec.

"SubZero" <g_ates@hotmail.com> wrote in message 
news:4633887b@newsgroups.borland.com...
> Ok. The original functions will be used with only TCriticalSection (single
> line). The reason for all the code is that it is needed for detecting the
> deadlock. I want to raise an exception so that MadExcept can show the 
> stack!
> Hope you understand now. Sorry for the confusion.

If you have madExcept, you don't need to throw an exception to get a stack 
trace.  madExcept provides routines to get a stack trace at *any* point 
during program execution, not just when an exception is thrown.  See the 
descriptions for StackTrace() and GetBugReport().

- Dennis 

"Bob Gonder" <notbg@notmindspring.invalid> wrote in message 
news:i717339g3u8esppurihllgq3aj0a5qelig@4ax.com...
> Dennis Jones wrote:
>
>>Perhaps I am mistaken, but it looks to me like the problem is 
>>obvious...your
>>comment says, "Create a mutex with no initial owner", but the code creates
>>it owned (conflicts with the comment).  Then you attempt to wait for the
>>mutex, and 5 seconds later, due to timeout, the exception is thrown.
>
> The CreateMutex(true) returns with ownership.
> The wait() satisfies with pre-ownership to eliminate deadlock.
> Drawback is now the mutex is owned twice and should be released twice.

I'm intrigued, but my brain is functioning on overload and a lack of sleep 
(only to get worse in the near future, I'm afraid).  Could you please 
explain how this methodology works to eliminate deadlock?

- Dennis 

The problem is I am not sure when to get the stack trace! Because it 
deadlocks at some location we do not know...

Thanks,

SZ

"Dennis Jones" <nospam@nospam.com>, iletisinde �unu yazd�, 
news:4634170d$1@newsgroups.borland.com...
>
> "SubZero" <g_ates@hotmail.com> wrote in message 
> news:4633887b@newsgroups.borland.com...
>> Ok. The original functions will be used with only TCriticalSection 
>> (single
>> line). The reason for all the code is that it is needed for detecting the
>> deadlock. I want to raise an exception so that MadExcept can show the 
>> stack!
>> Hope you understand now. Sorry for the confusion.
>
> If you have madExcept, you don't need to throw an exception to get a stack 
> trace.  madExcept provides routines to get a stack trace at *any* point 
> during program execution, not just when an exception is thrown.  See the 
> descriptions for StackTrace() and GetBugReport().
>
> - Dennis
> 

Hello,

I thought I could get rid of the mutexes with the below code:

void inline __fastcall lockCriticalSection(TCriticalSection 
*criticalSection)
{
  for(int i = 0; i < 5000; ++i)
  {
   if(criticalSection->TryEnter())
    return;

   ::Sleep(1);
  }

  throw Exception("BUG FOUND, CHECK CALL STACK!");

  //criticalSection->Acquire();
};

but it never fires the exception yet it still deadlocks! :((

Regards,

SZ

"SubZero" <g_ates@hotmail.com>, iletisinde �unu yazd�, 
news:4633777f$1@newsgroups.borland.com...
> Hello,
>
> I used to have an application with the following functions for thread 
> sync'in.
>
> void inline __fastcall lockCriticalSection(TCriticalSection 
> *criticalSection)
> {
>  criticalSection->Acquire();
> };
>
> void inline __fastcall releaseCriticalSection(TCriticalSection 
> *criticalSection)
> {
>  criticalSection->Release();
> };
>
>
> To detect a deadlock, I implemented the below:
>
> void inline __fastcall lockCriticalSection(TCriticalSection 
> *criticalSection)
> {
>  HANDLE hMutex;
>
>  // Create a mutex with no initial owner.
>
>  hMutex = CreateMutex(
>   NULL,                       // no security attributes
>   true,                      // initially owned
>   String((int)(void*)criticalSection).c_str());  // name of mutex
>
>  DWORD dwWaitResult;
>
>  // Request ownership of mutex.
>
>  if(hMutex)
>  {
>   dwWaitResult = WaitForSingleObject(
>   hMutex,   // handle to mutex
>   5000L);   // five-second time-out interval
>
>   switch(dwWaitResult)
>   {
>    // Cannot get mutex ownership due to time-out.
>    case WAIT_TIMEOUT:
>     throw Exception("BUG FOUND, CHECK CALL STACK!");
>
>    // Got ownership of the abandoned mutex object.
>    case WAIT_ABANDONED:
>     break;
>   }
>  }
>
>  criticalSection->Acquire();
> };
>
> void inline __fastcall releaseCriticalSection(TCriticalSection 
> *criticalSection)
> {
>  criticalSection->Release();
>
>  HANDLE hMutex;
>
>  // Create a mutex with no initial owner.
>
>  hMutex = OpenMutex(
>    MUTEX_ALL_ACCESS,
>    false,
>    String((int)(void*)criticalSection).c_str()
>  );  // name of mutex
>
>  if(hMutex)
>  {
>   DWORD dwWaitResult;
>
>   // Request ownership of mutex.
>
>   dwWaitResult = WaitForSingleObject(
>    hMutex,   // handle to mutex
>    5000L);   // five-second time-out interval
>
>   switch(dwWaitResult)
>   {
>    // The thread got mutex ownership.
>    case WAIT_OBJECT_0:
>     // Release ownership of the mutex object.
>     if(!ReleaseMutex(hMutex))
>     {
>      // Deal with error.
>     }
>
>     break;
>
>    // Cannot get mutex ownership due to time-out.
>    case WAIT_TIMEOUT:
>     break;
>
>    // Got ownership of the abandoned mutex object.
>    case WAIT_ABANDONED:
>     break;
>   }
>  }
> };
>
> which does not work for no obvious reason. It simply raises the exception 
> when there is no reason! The Critical section variable is -1, 0 at that 
> time!
>
> Please help!
>
> Best Regards,
>
> SZ 

Dennis Jones wrote:

> Could you please 
>explain how this methodology works to eliminate deadlock?

>> The wait() satisfies with pre-ownership to eliminate deadlock.
Rephrase:
pre-ownership satisfies the wait (eliminating deadlocking on self)

If I own the mutex, and issue a wait, the wait returns with another
ownership, it doesn't wait for me to release my original ownership.

This allows you to re-use functions that lock/modify/release without
affecting your own ownership.
 

Bob Gonder <notbg@notmindspring.invalid> wrote:
>
> [...] which you don't use CloseHandle on, so are leaking
> resources

From win32.hlp CreateMutex:

    Use the CloseHandle function to close the handle. The
    system closes the handle automatically when the process
    terminates. The mutex object is destroyed when its last
    handle has been closed.

~ JD

JD wrote:
>Bob Gonder  wrote:
>>
>> [...] which you don't use CloseHandle on, so are leaking
>> resources
>
>From win32.hlp CreateMutex:

And your point is?
That the mutex is released if he closes the handle?

My point was he was creating 2 handles for every lock/release, and
never closing them. Won't take long to run out of handles.

In my final example I showed how to create/leak only one handle per
TCriticalSection object, no matter how many times it was used.
Acceptable for testing purposes, as a few leaked handles shouldn't
create it's own set of problems (like the run-away handles could).
For on-going usage, I'd make the handle part of the class and have the
destructor close it.

But then, I prefere to write in a non-deadlocking manner in the first
place.

Bob Gonder <notbg@notmindspring.invalid> wrote:
>JD wrote:
>>Bob Gonder  wrote:
>>>
>>> [...] which you don't use CloseHandle on, so are leaking
>>> resources
>>
>>From win32.hlp CreateMutex:
>
>And your point is?

That it's impossible to leak resources in this case because
the OS cleans up.

~ JD

JD wrote:

>>And your point is?
>
>That it's impossible to leak resources in this case because
>the OS cleans up.

When the program exits.
Until then, the application can use up all available handles and then
fail.. I think it would only take around 32 thousand lock cycles
before failure.

Adding another assured (but not intended) failure point in an attempt
to track down an existing failure, seems a bit odd don't you think?