Avoid CMutex, CEvent, CSemaphore and CCrticalSection

Home Back To Tips Page

First, for those of you who may have already read my essays on Avoiding GetDlgItem and Avoiding UpdateData, this is not a flame. This is a warning. While reasonable people might argue that my view of UpdateData is flawed, or that GetDlgItem makes perfect sense. this essay tells you to avoid these constructs because they are broken. They do not and cannot work. Attempting to use them will lead to incomprehensible bugs.

 As far as I can tell, and this is my opinion, the task of coding these classes was relegated to someone at Microsoft who has never understood synchronization. The errors are so egregious that it is hard to imagine that anyone who understood the problem could ever make them. The result is a collection of classes that are so deeply broken that they cannot be made to work short of rewriting them.

All Synchronization Objects are broken

NEVER, UNDER ANY CIRCUMSTANCES IMAGINABLE, EVER, USE A SYNCHRONIZATION PRIMITIVE DEFINED BY MFC!!!!!!!!!

CMutex is broken

You can't tell a timeout from an abandoned mutex!

Here's the code from the abstract superclass CSyncObject::Lock, which is used to perform the ::WaitFor operation. 

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

FALSE is not sufficient. If you are using a mutex, you must check for both WAIT_TIMEOUT and WAIT_ABANDONED if the operation failed. It is very important to know which condition ensued, assuming, of course, you want your program to run correctly. Yet the critical information is discarded, and GetLastError() does not return the reason the operation failed! Therefore, you cannot possibly use this operation in any program that has pretensions of being correct, because you can't tell if you have failed because of timeout (legitimate, you may just wait again), or because the mutex was abandoned (which means the data structure is potentially corrupt, so if you wait again and pass, there is an excellent chance you will crash, possibly corrupting critical data in an irretrievable manner.

The code has been "improved" in later releases; it now says

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

which merely makes the failure explicit.  The code is still grossly wrong.

Therefore, it is impossible to believe that this operation could make sense.

You can't acquire a mutex recursively with CSingleLock!

Part of the specification of a mutex is that it can be acquired recursively by the same thread that owns it, and this is one of the interesting features of a mutex. You are only required to do a ::ReleaseMutex as many times as you do a ::WaitFor on the mutex.

The MFC library goes out of its way to make this impossible if you use CSingleLock. This is inexcusable.

The code for CSingleLock

BOOL CSingleLock::Lock(DWORD dwTimeOut /* = INFINITE */)
{
	ASSERT(m_pObject != NULL || m_hObject != NULL);
	ASSERT(!m_bAcquired);

	m_bAcquired = m_pObject->Lock(dwTimeOut);
	return m_bAcquired;
}

That second ASSERT is stupid. If you try to acquire a mutex recursively, which would make a lot of sense in a recursive graph walker on a shared graph (and lots of other contexts), an attempt to declare a CSingleLock in the same thread on the same mutex will generate an ASSERT that the mutex is already acquired. Hello. Is anyone reading the documentation? A mutex is supposed to be acquirable recursively!!! Furthermore, this insane restriction is not documented, so someone expecting this is just a "nice wrapper" around the API is going to be startled when it misbehaves and issues an ASSERT. Therefore, code which puts the recursive acquisition in a loop will fail on the ASSERT. 

Now, let's look at that m_bAcquired member. Note that it can be set FALSE if another thread tries to acquire the CSingleLock and times out, meaning if the first thread attempts a recursive acquisition after another thread fails, it won't ASSERT! Now, this might make sense if the CSingleLock were declared in a stack context, but there is nothing in the documentation of CSingleObject that requires that it only be declared in a stack context. If it is a member variable, the m_bAcquired flag is shared among all the threads.

Now, let's look at the unlock code:

BOOL CSingleLock::Unlock()
{ 
        ASSERT(m_pObject != NULL);
	if (m_bAcquired)
		m_bAcquired = !m_pObject->Unlock();

	// successfully unlocking means it isn't acquired
	return !m_bAcquired;
}

Note that this erroneously assumes that the m_bAcquired variable has meaning, and that it is not accessed from two or more threads.  Wow!  We have here an interthread syncrhonization primitive that is only correct if it is not executed in more than one thread!  Have I missed something here?  And, just to add to the confusion, if the mutex is successfully released once, it will never be released again!  The following code will work if using the raw primitive:

HANDLE lock = ::CreateMutex(NULL, FALSE, NULL);
::WaitForSingleObject(lock, INFINITE);
::WaitForSingleObject(lock, INFINITE);
::ReleaseMutex(lock);
::ReleaseMutex(lock);
::CloseHandle(lock);

but the MFC code cannot possibly work.  It gives ASSERT failures in debug mode and simply fails to work correctly (and leaves a lock permanently locked) in release mode:

CMutex crit;
CSingleLock lock(&crit);
lock.Lock();
lock.Lock();
lock.Unlock();
lock.Unlock();

I declare this code unsalvageable.

Note that the failure exhibited by the inclusion of the remarkably stupid use of a boolean variable means that if you name the Mutex and use it in two different processes, this code cannot possibly work.

CEvent is broken

For the same reasons CMutex is broken, CEvent is broken.  Including the inability to use a named event between processes.

CCriticalSection is broken

For the same reasons CMutex is broken, CCriticalSection is broken.  It is not possible to recursively acquire a CSingleLock using Lock.  This code will fail, even though it is supposed to work correctly:

CCriticalSection crit;
CSingleLock lock(&crit);
lock.Lock();
lock.Lock();
lock.Unlock();
lock.Unlock();

The fact that the above code cannot be written is ipso facto proof of the incompetence of the implementor of this code, and a serious condemnation of the oversight process that would let code written this way actually be delivered.

CSemaphore is broken

If you have a loop that is manipulating a semaphore, it cannot possibly work.  It would want to Lock the semaphore in each iteration, which is impossible given the incorrect implementation of CSyncObject::Lock, this is impossible.  And it is impossible for a second thread using the same semaphore to unlock it more than once!

In addition, the whole point of a semaphore is that one thread will increment it and another thread will wait on it.  So if a thread waits on a semaphore using CSingleLock, when the CSingleLock goes out of scope, the semaphore is released, even though this cannot possibly make sense.  There is no documentation that says that CSingleLock malfunctions grossly if it is applied to a CSemaphore.

Summary

What's not to like here?  Every possible mistake in implementation that can be imagined has been made.  Well, I take that back.  Anyone who understands synchronization would be far too smart to actually imagine that anyone could possibly make this many mistakes in a single class.