Re: CSingleLock - known behaviour?

On Wed, 25 Jun 2008 13:47:04 -0400, Joseph M. Newcomer
<newcomer@xxxxxxxxxxxx> wrote:

CSingleLock and all the MFC locking classes are complete crap, and should be avoided.

While a CRITICAL_SECTION has recursive acquisition semantics, the MFC layers introduce so
many bugs that it would not at all surprise me if CCriticalSection was as bug-laden as the
rest of the code.

Don't call IsLocked, it won't help, and in fact will produce an erroneous result. The
correct solution is to never, ever use an MFC locking primitive under any conditions.
joe

P.S. I decided to check, and indeed, the moron who wrote the code got something this
simple wrong also! Here's the code:

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;
}

Note how this ASSUMES that it is an error to do a recursive acquisition!!!!!

The real mistake is that you can pass a dwTimeOut value != INFINITE when
locking a CCriticalSection.

It takes a
profound amount of either stupidity or irresponsibility to create a primitive that
violates the fundamental behavior of the underlying object, which this CAREFULLY goes out
of its way to accomplish.

Now 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 how the programmer, who clearly knew NOTHING about locks, wrote the code! It will
only do an unlock if it believes the lock is set, and the result of a successful unlock is
to clear the lock flag, so the second unlock cannot possibly work!

Further proof that whoever wrote this had NO understanding of synchronization,
concurrency, or fundamental principles of software design, and had NO adult supervision.

This code makes no sense; it doesn't make sense for a critical section, mutex, semaphore,
or event. It cannot be made to make sense. It is wrong.

It just confirms that these classes are complete and utter wastes of space.
joe

As I explained in my messages, while the MFC sync classes are bad, there's
nothing wrong with the non-recursive nature of the lock class. If you
really believe there is, present an example that would take advantage of a
recursive lock class. I expect it will demonstrate poor design.

--
Doug Harrison
Visual C++ MVP
.

Relevant Pages

  • Re: CSingleLock - known behaviour?
    ... While a CRITICAL_SECTION has recursive acquisition semantics, the MFC layers introduce so ... The Lock code for CCriticalSection has an ASSERT ... Now look at the unlock code: ...
    (microsoft.public.vc.mfc)
  • Re: New to CSingleLock
    ... I'm having trouble making sense of the documentation for CCriticalSection ... Do I need a CSingleLock, or is CCriticalSection enough by itself? ... because the default is to *not* acquire the lock, which is completely, ... it must unlock them as many times as it locked them). ...
    (microsoft.public.vc.mfc)
  • Re: Refresh Problem
    ... If I wait 3 seconds between mouse clicks on the lock / unlock button, ... workstation is looking at a current copy of the data. ... MESSAGEBOX statement after all of the database lookups have been done ...
    (microsoft.public.fox.programmer.exchange)
  • Re: A scoped lock/unlock implementation in C++.
    ... mutex_locker wrapping the same mutex, more than one thread won't share ... But beyond that it will still work with thread local mutex_locker instances, since no two threads can have a nonzero lock count anyway. ... When entering a locked region, if the last entry is negative, you know ... In case of unlocks even the unlock count is insignificant. ...
    (comp.programming.threads)
  • Re: [PATCH] Remove softlockup from invalidate_mapping_pages.
    ... These block devices are very likely to have just one ... Being locked for read is very unlikely because mdadm would have already ... memory reclaim could lock the page, ... core is free to unlock the page when the IO completes, ...
    (Linux-Kernel)