Re: CSingleLock - known behaviour?

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!!!!! 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


On Wed, 25 Jun 2008 02:01:51 -0700 (PDT), neilsolent <neil@xxxxxxxxxxxxxxxxxxxxxx> wrote:

In my multi-threaded program, a thread may call CSingeLock::Lock()
twice on the same underlying Critical Section. I was expecting the
second Lock() call to do nothing if the object is already locked.

However, if the CS was already locked, it seems that the second Lock()
call has the effect of permanently locking the CS. If I then call
Unlock() multiple times, or even let the CSingleLock stack object go
out of scope - still the CS is locked.

Has anyone else seen this before?
Is this known (bad) behaviour of CSingleLock?

Obvious workaround - call IsLocked() before calling Lock()..

..BUT.. unless I am missing something - programmers should be aware of
this nasty gotcha !
Joseph M. Newcomer [MVP]
email: newcomer@xxxxxxxxxxxx
Web: http://www.flounder.com
MVP Tips: http://www.flounder.com/mvp_tips.htm
.

Relevant Pages

  • Re: CSingleLock - known behaviour?
    ... CSingleLock lock; ... ...remove element from queue ... Note that in the first example, the lock cannot be acquired a second time, and in the ... Now look at the unlock code: ...
    (microsoft.public.vc.mfc)
  • Re: CSingleLock
    ... CSingleLock mylock; ... It is fine to have a lock holder class. ... locked in try-catch-clauses that unlock the mutex and rethrow. ...
    (comp.programming.threads)
  • Re: CSingleLock - known behaviour?
    ... second Lock() call to do nothing if the object is already locked. ... Is this known behaviour of CSingleLock? ... locking indicates less than optimal design. ... then you have called UnLock too many times. ...
    (microsoft.public.vc.mfc)
  • =?Utf-8?Q?Re:_Daten_an_einen_Thread_=C3=BCberge?= =?Utf-8?Q?ben_=28queued=29?=
    ... Zugriff auf die SendQueue - den Lock und Unlock machen wollte. ... Die Critical Section besitzen wie einige andere Synchronisationsobjekte auch einen ... Du kannst auch mehrere CSingleLock auf ein und dasselbe Synchronisationsobjekt ...
    (microsoft.public.de.vc)
  • 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)