Re: CSingleLock - known behaviour?
On Jun 25, 10:22 am, "Doug Harrison [MVP]" <d...@mvps.org> wrote:
On Wed, 25 Jun 2008 09:52:38 -0700 (PDT), malachy.mo...@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 f=
or
multiple threads to call functions that protect data structures with
CSingleLock, e.g.
The CSingleLock::IsLocked() function returns the locked/signalled
state of the underlying synchronization object. See
http://msdn.microsoft.com/en-us/library/f36d0s0w(VS.71).aspx . There
is no such concept as the locked/signalled state of the CSingleLock
object (but from your post, I don't think that you were suggesting
that there was).
So, it indeed is entirely possible to "mess with" the state of the
underlying lock object, such as the underlying CCriticalSection.
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.
Yes, that's true, but it's entirely beside the point that was being
made. Even though one thread will not have access to the "lk" object
created by another thread, all threads have unrestricted access to the
underlying m_cs (CCriticalSection) object, which necessarily is shared
by all of the threads. And since the m_cs object is shared by all the
threads, its state can be changed by any one of them at any time,
making the IsLocked() function irrelevant and reliance on it a source
of program logic errors.
..BUT.. unless I am missing something - programmers should be aware o=
f
this nasty gotcha !
Mutexes in windows are recursive; MFC Lock types are not and will asse=
rt in
debug mode if you attempt to lock a lock that is already locked. It sh=
ould
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.
It's true that I misunderstood your focus on the sameness of the
CSingleLock object as opposed to that of the CCriticalSection object.
For purposes of clarity, it might be better if you did not refer to
locking of "lock types" since at least I thought that you were
referring to CCriticalSection::Lock() as opposed to
CSingleLock::Lock().
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.
Yes, you're correct: I misunderstood your point as explained above.
Also, the normal way to use CSingleLock is not to call Lock explicitly, b=
ut
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 no=
t
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 mo=
st
error-ridden in all of MFC.)
No attempt at the quiz?
Hehe, I didn't want to spoil the fun, so it's TRUE that I didn't
answer your quiz. The answer is hopefully clear to others from a
comparison of the code you posted versus the code I posted.
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- Hide quoted text -
- Show quoted text -- Hide quoted text -
- Show quoted text -