I was able to call CMutex::Lock from the same thread twice w/o calling 
CMutex::Unlock in between. I interpret this as it being re-entrant, is this 
correct?

I assume that if another thread (B) calls CMutex::Lock after thread A has 
called it, thread B will wait for thread A to call CMutex::Unlock. But i 
haven't verified this, and i'm not sure how it accomplishes this because all 
it does behind the scene is calling 

	if (::WaitForSingleObject(m_hObject, dwTimeout) == WAIT_OBJECT_0)
in Lock(). I'm not sure i'm convinced that thread B would block, if thread A 
itself didn't block calling this a second time. Since WaitForSingleObject 
returned immediately for thread A, it means object is signaled, and if that's 
the only check made, then thread B will return immediately too.

Unless somebody convinces me that thread B would actually block, i'm 
thinking about making a test app for this to be sure. Even though from the 
name of the class, i really assumed it was the same as Unix-style mutexes.

Thanks.

"ultranet" wrote:

> I was able to call CMutex::Lock from the same thread twice w/o calling 
> CMutex::Unlock in between. I interpret this as it being re-entrant, is this 
> correct?
> 
> I assume that if another thread (B) calls CMutex::Lock after thread A has 
> called it, thread B will wait for thread A to call CMutex::Unlock. But i 
> haven't verified this, and i'm not sure how it accomplishes this because all 
> it does behind the scene is calling 
> 
> 	if (::WaitForSingleObject(m_hObject, dwTimeout) == WAIT_OBJECT_0)
> in Lock(). I'm not sure i'm convinced that thread B would block, if thread A 
> itself didn't block calling this a second time. Since WaitForSingleObject 
> returned immediately for thread A, it means object is signaled, and if that's 
> the only check made, then thread B will return immediately too.
> 
> Unless somebody convinces me that thread B would actually block, i'm 
> thinking about making a test app for this to be sure. Even though from the 
> name of the class, i really assumed it was the same as Unix-style mutexes.
> 
> Thanks.
I guess one way this could work is if the lock object was treated as 
signalled for the thread that owns it, and non-signalled for alll other 
threads. Not being able to step into that source, kinda makes me raise this 
question though...

There have been any number of misimplementations of synchronization in the CWhatever
classes (for example, I once discovered an incredibly stupid bug in the implementation of
CEvent...the SetEvent would not actually set the event if it thought it had already set
the event, which worked fine in the childish programs the implementor had tested but would
not work if the event was set by application A and reset by application B; application A
thought it was already set and the MFC code refused to call the underlying ::SetEvent!
Reasons like this are why I would NEVER trust these synchronization objects in a real
program).

A mutex has what are called "recursive acquisition" semantics.  Mutice are "owned" by a
thread, and if that thread attempts to re-acquire them, it is able to pass without
blocking.  However, you must also call ::ReleaseMutex as many times as you have done a
::WaitFor... or it remains owned.  Only the thread that owns a mutex can unlock it.

A common mistake in coding ::WaitForSingleObject is the error you show below.  Code like
this is always wrong.  ::WaitForSingleObject can return, in the case of a mutex, FOUR
different values, and why you would test for only one of them makes no sense.  The four
cases are: (1) WAIT_OBJECT_0: the mutex has been acquired (2) WAIT_TIMEOUT if the timeout
ocurred (3) WAIT_ERROR if the mutex handle is erroneous, and (4) WAIT_ABANDONED if the
mutex is abandoned.  If you do not explicitly check for EVERY ONE OF THESE CONDITIONS your
code is erroneous and cannot be trusted (one special case: if you are doing INFINITE for
the wait, you can omit the WAIT_TIMEOUT test).  The ONLY correct way to write code like
this is

switch(::WaitForSingleObject(object, timeout))
    { /* wait */
      case WAIT_OBJECT_0:
             break;
      case WAIT_TIMEOUT:
            ... deal with timeout
      case WAIT_ERROR:
            ASSERT(FALSE);
            ... deal with illegal handle problems
      case WAIT_ABANDONED:
           ... you are now in such deep trouble that you can spend weeks writing this code
   } /* wait */

The fact that the code in MFC is NOT what you claimed, but is even worse:

BOOL CSyncObject::Lock(DWORD dwTimeout)
{
	DWORD dwRet = ::WaitForSingleObject(m_hObject, dwTimeout);
	if (dwRet == WAIT_OBJECT_0 || dwRet == WAIT_ABANDONED)
		return TRUE;
	else
		return FALSE;
}

proves that beyond any shadow of a doubt this implementation CANNOT be trusted!  It
actually claims that WAIT_ABANDONED means "lock has been acquired"! This goes so far
beyond stupidity and incompetence that it can only be classified as criminally
irresponsible.

(By the way, your interpretation of what happens in the simple case is incorrect; you
think that because a thread which owns the mutex would return immediately, that a thread
which doesn't own the mutex would return immediately.  This is because you have confused
signaled/nonsignaled with mutex ownership.  A mutex becomes nonsignaled, but it is also
owned, and that's the key difference; thread B would block, at least for its timeout
period, or until the mutex becomes abandoned; thread A doesn't care about the nonsignaled
state because it is already the owner).

An abandoned mutex is a catastrophe.  Not just a little glitch; a major catastrophe.  It
always represents a seriously erroneous program, but in the presence of such an error, you
must still recover properly.  This is nontrivial (having written code that can recover
from abandoned mutice, I can state this with some confidence.  We had to deal with shared
data in the presence of clients of unknown reliability).  Consequently, the simplest
approach is to handle abandoned mutex as invalidating all computations currently in
progress, and nothing that is protected by the mutex can be assumed to be intact any
longer, and will literally have to be abandoned.  And this, too, is nontrivial.  
					joe

On Fri, 9 Sep 2005 20:46:05 -0700, "ultranet" <ultranet@discussions.microsoft.com> wrote:

>I was able to call CMutex::Lock from the same thread twice w/o calling 
>CMutex::Unlock in between. I interpret this as it being re-entrant, is this 
>correct?
>
>I assume that if another thread (B) calls CMutex::Lock after thread A has 
>called it, thread B will wait for thread A to call CMutex::Unlock. But i 
>haven't verified this, and i'm not sure how it accomplishes this because all 
>it does behind the scene is calling 
>
>	if (::WaitForSingleObject(m_hObject, dwTimeout) == WAIT_OBJECT_0)
>in Lock(). I'm not sure i'm convinced that thread B would block, if thread A 
>itself didn't block calling this a second time. Since WaitForSingleObject 
>returned immediately for thread A, it means object is signaled, and if that's 
>the only check made, then thread B will return immediately too.
>
>Unless somebody convinces me that thread B would actually block, i'm 
>thinking about making a test app for this to be sure. Even though from the 
>name of the class, i really assumed it was the same as Unix-style mutexes.
>
>Thanks.
Joseph M. Newcomer [MVP]
email: newcomer@flounder.com
Web: http://www.flounder.com
MVP Tips: http://www.flounder.com/mvp_tips.htm

"Joseph M. Newcomer" wrote:

> The fact that the code in MFC is NOT what you claimed, but is even worse:
> 
> BOOL CSyncObject::Lock(DWORD dwTimeout)
> {
> 	DWORD dwRet = ::WaitForSingleObject(m_hObject, dwTimeout);
> 	if (dwRet == WAIT_OBJECT_0 || dwRet == WAIT_ABANDONED)
> 		return TRUE;
> 	else
> 		return FALSE;
> }
> 
> proves that beyond any shadow of a doubt this implementation CANNOT be trusted!  It
> actually claims that WAIT_ABANDONED means "lock has been acquired"! This goes so far
> beyond stupidity and incompetence that it can only be classified as criminally
> irresponsible.
> 
> (By the way, your interpretation of what happens in the simple case is incorrect; you
> think that because a thread which owns the mutex would return immediately, that a thread
> which doesn't own the mutex would return immediately.  This is because you have confused
> signaled/nonsignaled with mutex ownership.  A mutex becomes nonsignaled, but it is also
> owned, and that's the key difference; thread B would block, at least for its timeout
> period, or until the mutex becomes abandoned; thread A doesn't care about the nonsignaled
> state because it is already the owner).
> 
> An abandoned mutex is a catastrophe.  Not just a little glitch; a major catastrophe.  It
> always represents a seriously erroneous program, but in the presence of such an error, you
> must still recover properly.  This is nontrivial (having written code that can recover
> from abandoned mutice, I can state this with some confidence.  We had to deal with shared
> data in the presence of clients of unknown reliability).  Consequently, the simplest
> approach is to handle abandoned mutex as invalidating all computations currently in
> progress, and nothing that is protected by the mutex can be assumed to be intact any
> longer, and will literally have to be abandoned.  And this, too, is nontrivial.  
> 					joe
So basically, you are saying that thread B would in fact block after thread 
A locks the mutex, but in general CSomething synchronization objects aren't 
reliable. Given our time frame, i'll take your word, and not try to verify 
this.

Btw., in VC 6, i referenced the code correctly:
BOOL CSyncObject::Lock(DWORD dwTimeout)
{
	if (::WaitForSingleObject(m_hObject, dwTimeout) == WAIT_OBJECT_0)
		return TRUE;
	else
		return FALSE;
}

Apparently in VC version you are using, they screwed it up a little.

Finally, i guess the synchronization object has owning thread along w/ 
signaled/non-signalled state, which as you said makes non-owning threads 
block. That makes sense, but i wasn't sure that was the case, so wanted to 
get some feedback.

Thanks.

See below...
On Mon, 12 Sep 2005 15:06:03 -0700, "ultranet" <ultranet@discussions.microsoft.com> wrote:

>"Joseph M. Newcomer" wrote:
>
>> The fact that the code in MFC is NOT what you claimed, but is even worse:
>> 
>> BOOL CSyncObject::Lock(DWORD dwTimeout)
>> {
>> 	DWORD dwRet = ::WaitForSingleObject(m_hObject, dwTimeout);
>> 	if (dwRet == WAIT_OBJECT_0 || dwRet == WAIT_ABANDONED)
>> 		return TRUE;
>> 	else
>> 		return FALSE;
>> }
>> 
>> proves that beyond any shadow of a doubt this implementation CANNOT be trusted!  It
>> actually claims that WAIT_ABANDONED means "lock has been acquired"! This goes so far
>> beyond stupidity and incompetence that it can only be classified as criminally
>> irresponsible.
>> 
>> (By the way, your interpretation of what happens in the simple case is incorrect; you
>> think that because a thread which owns the mutex would return immediately, that a thread
>> which doesn't own the mutex would return immediately.  This is because you have confused
>> signaled/nonsignaled with mutex ownership.  A mutex becomes nonsignaled, but it is also
>> owned, and that's the key difference; thread B would block, at least for its timeout
>> period, or until the mutex becomes abandoned; thread A doesn't care about the nonsignaled
>> state because it is already the owner).
>> 
>> An abandoned mutex is a catastrophe.  Not just a little glitch; a major catastrophe.  It
>> always represents a seriously erroneous program, but in the presence of such an error, you
>> must still recover properly.  This is nontrivial (having written code that can recover
>> from abandoned mutice, I can state this with some confidence.  We had to deal with shared
>> data in the presence of clients of unknown reliability).  Consequently, the simplest
>> approach is to handle abandoned mutex as invalidating all computations currently in
>> progress, and nothing that is protected by the mutex can be assumed to be intact any
>> longer, and will literally have to be abandoned.  And this, too, is nontrivial.  
>> 					joe
>So basically, you are saying that thread B would in fact block after thread 
>A locks the mutex, 
***
Yes, the synchronization you expect *between* threads works, because the recursive
acquisition semantics are a different thing that is happening
****
>but in general CSomething synchronization objects aren't 
>reliable. Given our time frame, i'll take your word, and not try to verify 
>this.
***
Actually, the verification is easy; the code from VS.NET proves beyond any shadow of a
doubt that they cannot be trusted.  This actually returns "true", meaning "you have
acquired the lock successfully", when the error is "mutex abandoned", a fatal error.

The example below also demonstrates that they code is unreliable, because you can't tell
the difference bertween a timeout (meaning you might go back and retry the wait after,
say, checking for a thread shutdown event that was set) and anything else.  Note that this
returns TRUE if there IS a timeout, and FALSE on either successful acquisition or
abandonment; or, if you made a transcription error and that should have been !=, then it
returns FALSE on timeout and TRUE on either acquisition or abandonment.  Or on having an
illegal handle.  In any case, only one of four possible cases is covered correctly, and
that means you cannot write reliable code above it.

Example: assume that you get mutex abandoned.  This means "there is the possibility that
the data on which I was working is now in an inconsistent state relative to the
postconditions of mutex release", or rephrased simply, "my data is screwed".  The next
attempt to acquire the mutex may then successfully pass the mutex, and work with invalid
data.  This is generally considered to be in poor taste, and will either generate access
faults if you are lucky, or just silently propagate the error if you are not.
****
>
>Btw., in VC 6, i referenced the code correctly:
>BOOL CSyncObject::Lock(DWORD dwTimeout)
>{
>	if (::WaitForSingleObject(m_hObject, dwTimeout) == WAIT_OBJECT_0)
>		return TRUE;
>	else
>		return FALSE;
>}
>
>Apparently in VC version you are using, they screwed it up a little.
>
>Finally, i guess the synchronization object has owning thread along w/ 
>signaled/non-signalled state, which as you said makes non-owning threads 
>block. That makes sense, but i wasn't sure that was the case, so wanted to 
>get some feedback.
****
The real problem is that the C++ advantages of having the lock/release scoped by a
declaration are lost because they got this all wrong.  For example, CSingleLock should
throw an exception if it can't acquire the lock, e.g.,

swtich(::WaitForSingleObject(...))
   {
     case WAIT_ERROR:
            throw new CWaitException(...);  // CWaitException: public CSyncException
     case WAIT_ABANDONED:
           throw new CAbandonedException(...); // CAbandonedException : public
CSyncException
     case WAIT_TIMEOUT:
          return FALSE;
     case WAIT_OBJECT_0:
          return TRUE;
     default:
          ASSERT(FALSE); // error, unknown return from ::WaitForSingleObject
          throw CSomeWeirdSyncProblem(...);
    }

Now that's what I'd expect in a well-designed class. Lacking something like this makes the
value of the class quesiotnable.  By the way, I looked at the use of m_bAcquired as their
way of telling if the lock was acquired. m_bAcquired will be TRUE if the mutex was
abandoned.  I do not wish to have my life depend on the code  that uses these classes.
*****
>
>Thanks.
Joseph M. Newcomer [MVP]
email: newcomer@flounder.com
Web: http://www.flounder.com
MVP Tips: http://www.flounder.com/mvp_tips.htm

On Wed, 14 Sep 2005 13:52:07 -0400, Joseph M. Newcomer
<newcomer@flounder.com> wrote:

>The real problem is that the C++ advantages of having the lock/release scoped by a
>declaration are lost because they got this all wrong.

One of my favorite bits of wrongness is the choice of default value:

	explicit CSingleLock(CSyncObject* pObject, BOOL bInitialLock = FALSE);

Probably best not to contemplate how many race conditions this has failed
to avoid...

>For example, CSingleLock should
>throw an exception if it can't acquire the lock, e.g.,
>
>swtich(::WaitForSingleObject(...))
>   {
>     case WAIT_ERROR:
>            throw new CWaitException(...);  // CWaitException: public CSyncException
>     case WAIT_ABANDONED:
>           throw new CAbandonedException(...); // CAbandonedException : public
>CSyncException
>     case WAIT_TIMEOUT:
>          return FALSE;
>     case WAIT_OBJECT_0:
>          return TRUE;
>     default:
>          ASSERT(FALSE); // error, unknown return from ::WaitForSingleObject
>          throw CSomeWeirdSyncProblem(...);
>    }
>
>Now that's what I'd expect in a well-designed class. Lacking something like this makes the
>value of the class quesiotnable.  By the way, I looked at the use of m_bAcquired as their
>way of telling if the lock was acquired. m_bAcquired will be TRUE if the mutex was
>abandoned.  I do not wish to have my life depend on the code  that uses these classes.

That's all very true for kernel mutexes. Note the more commonly used
CRITICAL_SECTION is also subject to this problem, and the documentation
says this:

EnterCriticalSection
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/dllproc/base/entercriticalsection.asp
<q>
If a thread terminates while it has ownership of a critical section, the
state of the critical section is undefined.
</q>

Under Windows XP, at least, waiting threads appear to hang, which I suppose
is good, since there's no way to detect the problem and deal with it. Also,
it's mainly crashed threads that abandon, and a crashed thread should take
the whole process with it, which further mitigates the issue.

-- 
Doug Harrison
VC++ MVP