Re: CSingleLock - known behaviour?
On Wed, 25 Jun 2008 09:52:38 -0700 (PDT), malachy.moses@gmail.com wrote:
On Jun 25, 8:48?am, "Doug Harrison [MVP]" <d...@mvps.org> wrote:
On Wed, 25 Jun 2008 02:01:51 -0700 (PDT), neilsolent
<n...@solenttechnology.co.uk> 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()..
I think it would be very strange for you to logically not know whether or
not a Lock object is locked or not.
I tend to disagree with this. The very existence of an IsLocked()
function is ridiculous, for the reason that its existence gives the
mis-impression that the returned value is reliable for more than the
present instant of time and thus promotes faulty programming logic.
IsLocked() returns the lock state now, at the present instant, but
immediately after the call returns the lock state can be changed out
from under you by another thread. So, reliance on the returned value
results in faulty multi-threade logic.
Well, no. A given CSingleLock object is to be used by a single thread as an
auto (i.e. "stack") variable, and that is the correct design for lock
types. When used correctly, there is no danger of another thread messing
with the lock object, because it doesn't have access to it. The idea is for
multiple threads to call functions that protect data structures with
CSingleLock, e.g.
void f()
{
CSingleLock lk(mx, true);
... mess with data structure
}
Multiple threads can call f(), and access to the protected data structure
is protected by the lock. Each thread creates its own lock object which
operates on the same mutex, mx.
..BUT.. unless I am missing something - programmers should be aware of
this nasty gotcha !
Mutexes in windows are recursive; MFC Lock types are not and will assert in
debug mode if you attempt to lock a lock that is already locked. It should
be exceedingly rare to call Lock/Unlock on a lock object.
I tend to disagree with this as well. At least for the
CCriticalSection lock type, nested/recursive calls to lock are
perfectly permissible, and will work exactly the same as
nestedrecursive calls to the underlying CRITICAL_SECTION functions of
EnterCriticalSection and LeaveCriticalSection.
That's just plain wrong for the reason I already gave.
For example, the following code is completely acceptable and will work
as expected. It will not "permanently lock" the critical section.
Moreover, assuming the the Y() function is invoked through the X()
function, then the second call to Lock() (i.e., in the Y() function)
is guaranteed to always return immediately and successfully, and never
block, since the thread has already acquired the critical section in
the X() function.
void CClass::X()
{
CSingleLock lock(&m_cs);
lock.Lock();
...
Y();
...
}
void CClass::Y()
{
CSingleLock lock(&m_cs);
lock.Lock();
...
}
Those are *different* lock objects, and so you are *not* recursively
locking a CSingleLock object; you are recursively locking the *mutex*,
which as I said, is permissible. However, this sort of recursive mutex
locking indicates less than optimal design. It's often far more efficient
to define a locking API for normal use and a non-locking API that can be
used when an outer lock is first obtained. For example, MS has done this
extensively in the CRT.
Also, the normal way to use CSingleLock is not to call Lock explicitly, but
to say:
void CClass::X()
{
CSingleLock lock(&m_cs, true);
...
Y();
...
}
That's what I meant by using it in "RAII style".
So, in response to the OP, if you see a permanently locked
CCriticalSection, then you have called UnLock too many times. The
count of calls to UnLock must exactly match the count of calls to
Lock.
That applies only when calling the CCriticalSection Lock/Unlock functions.
It does not apply to CSingleLock objects, which either are locked or not
locked (i.e. the "count" is a boolean).
The vast majority
of the time, the RAII usage should be all you need, e.g.
Yes, RAII is probably the best technique to ensure that the count of
calls to Lock and Unlock match each other.
There is no "count" of Lock and Unlock calls for CSingleLock, at least not
for the count > 1 you are suggesting exists.
void f()
{
? ?CSingleLock lk(x); // See below...
? ?...
}
Quiz: What's wrong with this function, and how would you fix it? (Hint: It
demonstrates a truly glaring flaw in the design of CSingleLock, one of many
in the design of the MFC synchronization classes, which are IMO the most
error-ridden in all of MFC.)
No attempt at the quiz?
I agree that the design of the sync classes is poor and poorly
documented, but they are not particularly error-ridden.
The design is not just bad or inconvenient; it's erroneous in many ways.
--
Doug Harrison
Visual C++ MVP