CSingleLock - known behaviour?
neilsolent
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 !
![Doug Harrison [MVP]'s profile photo](http://lh3.googleusercontent.com/a-/ALV-UjVvzhOQGbT9K0Re6Qg-OU4VGuHq0tAdTu01YlmIjUGm6t_o8g=s40-c)
Doug Harrison [MVP]
<ne...@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.
>..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. The vast majority
of the time, the RAII usage should be all you need, e.g.
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.)
--
Doug Harrison
Visual C++ MVP
malach...@gmail.com
> On Wed, 25 Jun 2008 02:01:51 -0700 (PDT), neilsolent
>
> >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.
> >..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.
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();
...
}
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.
>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.
> 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.)
>
I agree that the design of the sync classes is poor and poorly
documented, but they are not particularly error-ridden.
Doug Harrison [MVP]
>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.
Joseph M. Newcomer
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
Joseph M. Newcomer [MVP]
email: newc...@flounder.com
Web: http://www.flounder.com
MVP Tips: http://www.flounder.com/mvp_tips.htm
Doug Harrison [MVP]
<newc...@flounder.com> 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.
malach...@gmail.com
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 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.
>
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.
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 -
Joseph M. Newcomer
On Wed, 25 Jun 2008 13:00:16 -0500, "Doug Harrison [MVP]" <d...@mvps.org> wrote:
>On Wed, 25 Jun 2008 13:47:04 -0400, Joseph M. Newcomer
><newc...@flounder.com> 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.
****
The Lock code for CCriticalSection has an ASSERT
ASSERT(dwTimeout == INFINITE);
Of course, in release mode, this will fail to detect an erroneous call.
****
>
>>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.
****
A named mutex used in two different processes; a named event used in two different
processes; a semaphore that is in a loop that needs to decrement the semaphore until it
blocks; that's just the start. Note that CMutex, CSemaphore, and CEvent all allow the
name to be supplied and used, but you can't actually use the primitives across processes.
And what is wrong with the non-recursive nature of the lock is that the OP appears to have
a legitimate reason to need recursive acquisition, and it doesn't work.
And no, it doesn't say anywhere in the discussion of CSingleLock class that it violates
the basic principles of recursive acquisition and cannot be used in that way, or that it
cannot be used when the object is shared between processes, and the documentation of
CSingleLock::Lock is deficient in the same way, nor does it say that the timeout must be
INFINITE for CCriticalSection. Furthermore, the discussion of the class does not mention
the existence, let alone the use, of CSingleLock::Lock, and the example is erroneous in
that it sets the lock and doesn't check the result, but then checks the IsLocked variable,
which sets up a potential race condition. Or that the destructor will unlock the
object, making it unusable when using CSemaphore. What can possibly be *right* about
these classes?
joe
****
Joseph M. Newcomer
CSemaphore queuesem(...);
and the code is
void Receiver()
{
CSingleLock lock(&queuesem);
while(TRUE)
{
lock.Lock();
...remove element from queue
}
}
which cannot be written as
void Receiver()
{
while(TRUE)
{
CSingleLock lock(&queuesem);
lock.Lock();
...remove element from queue
}
}
Note that in the first example, the lock cannot be acquired a second time, and in the
second example, each time you leave the body of the loop the semaphore is released, even
though that is the responsibility of the other thread.
There are symmetric examples for CEvent, where you want to wait for the event in a loop.
joe
On Wed, 25 Jun 2008 13:00:16 -0500, "Doug Harrison [MVP]" <d...@mvps.org> wrote:
Doug Harrison [MVP]
>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:
>>
>> 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.
>
>
>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.
Unfortunately, you can't believe half of what you read in MSDN, and that's
not from the better half. The function CSingleLock::IsLocked() absolutely
does not "Determine if the object associated with the CSingleLock object is
nonsignaled", which is what it claims to do. That's just silly, and it's
obvious why if you examine the source code:
_AFXMT_INLINE BOOL (::CSingleLock::IsLocked())
{ return m_bAcquired; }
Rather, the function only indicates whether the given CSingleLock object
holds the lock; that's a subtle difference, but crucial to understand.
>There
>is no such concept as the locked/signalled state of the CSingleLock
>object
Actually, that's pretty much the correct way of looking at it. :)
>So, it indeed is entirely possible to "mess with" the state of the
>underlying lock object, such as the underlying CCriticalSection.
There is no underlying "lock object"; the "lock object" is overt. It is the
CSingleLock object. The "underlying object" is the synchronization object,
in this case, the inaptly and awkwardly named "CCriticalSection". Of course
it is possible to mess with the state of the CCriticalSection; if there
weren't, there wouldn't be any point to CSingleLock!
>> 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.
That's because you incorrectly think IsLocked has something to do with the
mutex, and as explained earlier, it really doesn't. It indicates only
whether or not the given lock object has locked the mutex.
>> >> >..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.
>>
>
>
>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" referring to CCriticalSection::Lock() as opposed to
>CSingleLock::Lock().
I thought it was clear. If I meant to refer to CCriticalSection, I would
have used that word. My only use of "Lock types" was in this context:
<q>
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. The vast majority
of the time, the RAII usage should be all you need, e.g.
void f()
{
CSingleLock lk(x, true); // See below.
// NB: Since I'm not quoting the "below" part, I've corrected the
// example.
...
}
</q>
I drew a distinction between mutexes and "Lock types", I went on to talk
about "lock objects", and I gave an example using CSingleLock. Nowhere did
I use the word "CCriticalSection". I don't know how to make it any clearer.
>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.
We can only hope. Whenever the subject of the MFC lock types arises, I
always like to bring this up, because it's so surprising and wrong! I
expect there are numerous bugs in MT programs in the wild due to people
misunderstanding or (understandably) forgetting the necessary addition to
the initialization.
Doug Harrison [MVP]
<newc...@flounder.com> wrote:
>See below....
>On Wed, 25 Jun 2008 13:00:16 -0500, "Doug Harrison [MVP]" <d...@mvps.org> wrote:
>
>>The real mistake is that you can pass a dwTimeOut value != INFINITE when
>>locking a CCriticalSection.
>****
>The Lock code for CCriticalSection has an ASSERT
> ASSERT(dwTimeout == INFINITE);
>Of course, in release mode, this will fail to detect an erroneous call.
>****
Yeah, I know. As I recall, the CMultiLock class also asserts when passed a
CCriticalSection. It's just really bad design. And while this isn't
entirely MFC's fault, even the name "CCriticalSection" grates, because a
"critical section" is a region of code, not a data type, and the Windows
CRITICAL_SECTION object is really a type of lightweight mutex. In fact,
that's what I call it in my class library, "LwMutex". The MFC
synchronization classes have always bugged me for these and many other
reasons, literally from the moment they were introduced, I guess it was in
VC4. I continued to use and write my own synchronization classes rather
than use the MFC ones, which have always been rather hopeless.
>>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.
>****
>A named mutex used in two different processes; a named event used in two different
>processes; a semaphore that is in a loop that needs to decrement the semaphore until it
>blocks; that's just the start. Note that CMutex, CSemaphore, and CEvent all allow the
>name to be supplied and used, but you can't actually use the primitives across processes.
I don't see how any of that is relevant to the use of CSingleLock.
Seriously, try to write some code that exploits a recursive CSingleLock.
>And what is wrong with the non-recursive nature of the lock is that the OP appears to have
>a legitimate reason to need recursive acquisition, and it doesn't work.
I can't imagine good code that needs a recursive CSingleLock. Code example,
please?
>And no, it doesn't say anywhere in the discussion of CSingleLock class that it violates
>the basic principles of recursive acquisition and cannot be used in that way
Yeah, well, you're talking about MSDN, and you know what you think about
MSDN. :)
>or that it
>cannot be used when the object is shared between processes
If by "object" you mean (for example) CMutex, and thus kernel-based mutex,
I'm unaware of any such restriction.
>and the documentation of
>CSingleLock::Lock is deficient in the same way, nor does it say that the timeout must be
>INFINITE for CCriticalSection. Furthermore, the discussion of the class does not mention
>the existence, let alone the use, of CSingleLock::Lock, and the example is erroneous in
>that it sets the lock and doesn't check the result, but then checks the IsLocked variable,
>which sets up a potential race condition.
Are you talking about this example?
CSingleLock::IsLocked
http://msdn.microsoft.com/en-us/library/f36d0s0w(VS.80).aspx
<q>
CSingleLock singleLock(&m_Mutex);
// Attempt to lock the shared resource
singleLock.Lock(100); // Wait 100 ms...
// Has the resource been successfully locked?
if (singleLock.IsLocked())
{
// We were able to lock the resource;
// we may now work with the data associated with the mutex...
// Now that we are finished, unlock the resource for others.
singleLock.Unlock();
}
</q>
It doesn't need to check the result of Lock, because it calls IsLocked, and
there is no race, because CSingleLock::IsLocked is implemented as follows,
and the CSingleLock object is an auto (local stack) object, so every thread
running the code gets its own CSingleLock:
<q>
_AFXMT_INLINE BOOL (::CSingleLock::IsLocked())
{ return m_bAcquired; }
</q>
>Or that the destructor will unlock the
>object, making it unusable when using CSemaphore. What can possibly be *right* about
>these classes?
> joe
Not much. :)
neilsolent
As Doug says, because IsLocked() is telling you the state of your
CSingleLock object, and not the state of the underlying
synchronisation object per se, it is a useful function, and shouldn't
be dismissed (as someone claimed earlier)
I am using the following function instead of CSingleLock::Lock() now,
and everything seems to be fine:
void SafeLock(CSingleLock& lock)
{
if (! lock.IsLocked()) lock.Lock();
}
(Note: I don't care about timeouts - I am using critical sections that
don't support them anyway).
If I were the MFC developer, I would have made Lock() just return
without asserting if m_bAcquired == TRUE. But that's me!
This enables you to write neater code without having to worry about
whether you already have locked the object.
Sorry I didn't spot the ASSERT at the start of all this by the way -
was testing on a separate system without a debugger installed.
Doug Harrison [MVP]
<newc...@flounder.com> wrote:
>How about this one:
>
>CSemaphore queuesem(...);
>
>
>and the code is
>
>void Receiver()
> {
> CSingleLock lock(&queuesem);
> while(TRUE)
> {
> lock.Lock();
> ...remove element from queue
> }
>}
>
>which cannot be written as
>
>void Receiver()
> {
> while(TRUE)
> {
> CSingleLock lock(&queuesem);
> lock.Lock();
> ...remove element from queue
> }
> }
>
>Note that in the first example, the lock cannot be acquired a second time and in the
>second example, each time you leave the body of the loop the semaphore is released, even
>though that is the responsibility of the other thread.
>
>There are symmetric examples for CEvent, where you want to wait for the event in a loop.
> joe
I've always believed that the concept of "locking" doesn't really apply to
events, so it's just more bad design that you can use a CEvent with
CSingleLock, and that CEvent contains a function named "Unlock". Jeez, I
was saying that 10 years ago:
http://groups.google.com/group/microsoft.public.vc.language/msg/534cab3386d12348
After reviewing the implementation of CEvent::Unlock in VC 2008, I rest my
case:
BOOL CEvent::Unlock()
{
return TRUE;
}
Furthermore, although I've used events, I've never needed an RAII wrapper
for setting/resetting them, and indeed, my class library doesn't define
one.
Now that you mention it, the lock template class I last modified on
1/3/1999 does maintain a count. I guess I did this for the semaphore class
wrapper I wrote, and I forgot about it, as I haven't used a semaphore in
all that time. (The class library I wrote about three years ago doesn't
currently contain a semaphore class, just mutexes, events, condition
variables, and so forth.) So, I've been limiting my "lock thinking" to
mutexes, and CSingleLock clearly won't work for your semaphore example.
That said, it's trivial to write your own lock class that will work.
So, when I asked for a code example, I was really thinking about one that
would demonstrate the usefulness of recursive lock objects as applied to
mutexes.
Doug Harrison [MVP]
<ne...@solenttechnology.co.uk> wrote:
>Wow this has caused a stir !
>
>Good posts.
>
>My comments are:
>
>1. Someone raised the point "I think it would be very strange for you
>to logically not know whether or not a Lock object is locked or not. "
That was me.
>My answer: Of course I know whether a lock object is locked or not -
>or at least I could code in that way - but I was writing my code based
>on the assumption that it didn't matter whether the CSingleLock object
>had already had Lock() called on it previously - I made the assumption
>that the underlying MFC code would handle this intelligently. If you
>don't have to keep checking the state of your locks, you can write
>simpler, neater code that is easier to read and maintain.
I've used lock classes extensively for 10+ years, and I can't recall ever
needing to use a function such as CSingleLock::IsLocked, nor can I recall
coding myself into contortions to avoid it. Can you give me an example or
two that demonstrates why you need to do this?
>2. I think the CSingleLock::IsLocked() function tests whether Lock()
>has been called successfully - NOT whether the underlying
>synchronisation object has been locked by a different thread. From
>some of the posts it sounds like people disagree..
You are correct. Someone introduced the mistaken notion several messages
ago, and I corrected this misunderstanding in my reply.
>3. I am now using this function in place of CSingleLock::Lock()
>directly, and everything is working for me:
>
>void SafeLock(CSingleLock& lock)
>{
> if (! lock.IsLocked()) lock.Lock();
>}
Please don't take this the wrong way, but if you need to do that on
anything but an exceedingly rare basis, I'd have to suspect you're using
CSingleLock in less than an optimal fashion. It makes me wonder if you're
using a CSingleLock as a class member, or you're passing CSingleLock by
reference from function to function, etc, which would almost certainly
represent invalid use of this class for the "critical section" object you
mentioned in your first message.
Joseph M. Newcomer
On Wed, 25 Jun 2008 15:47:39 -0500, "Doug Harrison [MVP]" <d...@mvps.org> wrote:
>On Wed, 25 Jun 2008 15:39:07 -0400, Joseph M. Newcomer
><newc...@flounder.com> wrote:
>
>>See below....
>>On Wed, 25 Jun 2008 13:00:16 -0500, "Doug Harrison [MVP]" <d...@mvps.org> wrote:
>>
>>>The real mistake is that you can pass a dwTimeOut value != INFINITE when
>>>locking a CCriticalSection.
>>****
>>The Lock code for CCriticalSection has an ASSERT
>> ASSERT(dwTimeout == INFINITE);
>>Of course, in release mode, this will fail to detect an erroneous call.
>>****
>
>Yeah, I know. As I recall, the CMultiLock class also asserts when passed a
>CCriticalSection. It's just really bad design. And while this isn't
>entirely MFC's fault, even the name "CCriticalSection" grates, because a
>"critical section" is a region of code, not a data type, and the Windows
>CRITICAL_SECTION object is really a type of lightweight mutex. In fact,
>that's what I call it in my class library, "LwMutex". The MFC
>synchronization classes have always bugged me for these and many other
>reasons, literally from the moment they were introduced, I guess it was in
>VC4. I continued to use and write my own synchronization classes rather
>than use the MFC ones, which have always been rather hopeless.
****
Dijkstra first coined the term "critical section" to indicate a piece of code that needed
to be locked. Later, we decided that locking code was a Really Bad Idea (this was known
by at least 1972), so the purpose of the name was redirected to mean "code that cannot be
executed concurrently on a specific data object". The use of it to name a lock is not
only annoying, but it takes some time to educate new CS graduates that it doesn't mean
what they were taught.
****
>
>>>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.
>>****
>>A named mutex used in two different processes; a named event used in two different
>>processes; a semaphore that is in a loop that needs to decrement the semaphore until it
>>blocks; that's just the start. Note that CMutex, CSemaphore, and CEvent all allow the
>>name to be supplied and used, but you can't actually use the primitives across processes.
>
>I don't see how any of that is relevant to the use of CSingleLock.
>Seriously, try to write some code that exploits a recursive CSingleLock.
****
Circular list detection is the simplest case I can think of. I don't know what the OP was
doing. But sequential acquisition, e.g., my semaphore example, demonstrates that it also
won't work.
****
>
>>And what is wrong with the non-recursive nature of the lock is that the OP appears to have
>>a legitimate reason to need recursive acquisition, and it doesn't work.
>
>I can't imagine good code that needs a recursive CSingleLock. Code example,
>please?
****
Same reply as above. Circular list detection.
****
>
>>And no, it doesn't say anywhere in the discussion of CSingleLock class that it violates
>>the basic principles of recursive acquisition and cannot be used in that way
>
>Yeah, well, you're talking about MSDN, and you know what you think about
>MSDN. :)
****
And with justification!
****
>
>>or that it
>>cannot be used when the object is shared between processes
>
>If by "object" you mean (for example) CMutex, and thus kernel-based mutex,
>I'm unaware of any such restriction.
****
Try the following: a producer loop which locks on a named semaphore (poolsemaphore) to
obtain buffers, and uses a named semaphore (queuesemaphore) to notify consumers of a
queue, and a consumer loop that blocks on the queuesemaphore and releases the buffers back
using the poolsemaphore. Put them in different processes using, for example, shared
memory to hold the buffer pool and queue. Process-local handles for the semaphores. You
can't create a loop that waits on the semaphores because the code is fundamentally
incorrect. The local view of whether or not the semaphore is locked is always wrong, and
the fact that the destructor will ::ReleaseSemaphore is nonsensical.
****
****
This limitation is not documented, and therefore if the lock is passed by
reference/pointer to another thread, there is a race condition.
****
>
>>Or that the destructor will unlock the
>>object, making it unusable when using CSemaphore. What can possibly be *right* about
>>these classes?
>> joe
>
>Not much. :)
Joseph M. Newcomer
****
I knew it at about the same time; I was trying to write code that synchronized two
different processes using a CEvent (it also used a CMutex, but an event was the "natural"
implementation of a pause/proceed interprocess control mechanism) and it didn't work.
After single-stepping the code, I decided that the author of the code knew nothing about
synchronization, and in fact the classes add negative value to MFC. But here we are, ten
years later, with the same badly-implemented crap, no decent classes published by
Microsoft, and new programmers are discovering the same mistakes we have known about for a
decade.
****
>After reviewing the implementation of CEvent::Unlock in VC 2008, I rest my
>case:
>
>BOOL CEvent::Unlock()
>{
> return TRUE;
>}
****
So they finally fixed that one...
****
>
>Furthermore, although I've used events, I've never needed an RAII wrapper
>for setting/resetting them, and indeed, my class library doesn't define
>one.
*****
I just ignore all synchronization classes, which I have managed to do for a decade. But
everyone has to discover these failures for themselves, which should not be necessary.
*****
>
>Now that you mention it, the lock template class I last modified on
>1/3/1999 does maintain a count. I guess I did this for the semaphore class
>wrapper I wrote, and I forgot about it, as I haven't used a semaphore in
>all that time. (The class library I wrote about three years ago doesn't
>currently contain a semaphore class, just mutexes, events, condition
>variables, and so forth.) So, I've been limiting my "lock thinking" to
>mutexes, and CSingleLock clearly won't work for your semaphore example.
>That said, it's trivial to write your own lock class that will work.
>
>So, when I asked for a code example, I was really thinking about one that
>would demonstrate the usefulness of recursive lock objects as applied to
>mutexes.
*****
Circular lists is the one that comes to mind.
joe
*****
Joseph M. Newcomer
synchronization objects, which always do the right thing in the right way under all the
scenarios that are interesting. Or even possible (I exclude deadlock scenarios, in which
no implementation, even the raw API, can compensate for an incorrect locking strategy).
So why the fixation on a set of known-to-be-broken classes?
Don't say "the destructors handle the unlocking" because you can handle that quite simply.
Instead of writing
{
CSingleLock lock(&mutex);
lock.Lock();
....
} // implict Unlock here
you can write
__try {
::WaitForCriticalSection(mutexhandle, INFINITE); // error handling left EFTR
}
__finally
{
::ReleaseMutex(mutexhandle);
}
or, if you are using MFC, where _try/__finally are usually diagnosed incorrectly by the
compiler as Not Playing Well With Others
try {
::WaitForSingleObject(mutexhandle, INFINITE); // errors EFTR
DoSomething();
::ReleaseMutex(mutexhandle);
}
catch(CException * e)
{
::ReleaseMutex(mutexhandle);
throw;
}
or, if you either know there will be no exceptions thrown, or simply don't care because if
an exception is thrown you are already in such deep trouble that the mutex state isn't
going to matter, you can eliminate the try/catch block. Note that the *only* code between
the ::WFSO and ::ReleaseMutex is a single function call (perhaps with parameters) so no
matter *what* the code does (returns, etc.), you cannot bypass the unlock code.
I've used both these techniques in my code. It means I don't have to deal with someone
else's buggy code.
joe
*****
Doug Harrison [MVP]
<newc...@flounder.com> wrote:
>What is interesting is that this whole discussion goes away if you just use the raw
>synchronization objects, which always do the right thing in the right way under all the
>scenarios that are interesting.
But then you have problems with exception safety and early returns. You can
avoid those issues by using RAII lock classes. Otherwise, you might as well
program in C.
>Or even possible (I exclude deadlock scenarios, in which
>no implementation, even the raw API, can compensate for an incorrect locking strategy).
>
>So why the fixation on a set of known-to-be-broken classes?
I'm not fixated on them; as I've said many times, I've never used the MFC
sync classes.
>Don't say "the destructors handle the unlocking" because you can handle that quite simply.
>Instead of writing
> {
> CSingleLock lock(&mutex);
> lock.Lock();
If I were to use CSingleLock for the first time ever, I would write:
CSingleLock lock(&mutex, true);
and omit the Lock call.
> ....
> } // implict Unlock here
>
>you can write
> __try {
> ::WaitForCriticalSection(mutexhandle, INFINITE); // error handling left EFTR
> }
> __finally
> {
> ::ReleaseMutex(mutexhandle);
> }
>
>or, if you are using MFC, where _try/__finally are usually diagnosed incorrectly by the
>compiler as Not Playing Well With Others
>
> try {
> ::WaitForSingleObject(mutexhandle, INFINITE); // errors EFTR
> DoSomething();
> ::ReleaseMutex(mutexhandle);
> }
> catch(CException * e)
> {
> ::ReleaseMutex(mutexhandle);
> throw;
> }
Writing try/catch blocks clutters the code in a big way, and you have to
duplicate code as you've done above to handle the exception and
non-exception paths, which is error-prone. Q: Why doesn't C++ have finally?
A: Because it has RAII. See Stroustrup:
Why doesn't C++ provide a "finally" construct?
http://www.research.att.com/~bs/bs_faq2.html#finally
I've said many times that the degree to which you're using C++ exceptions
effectively is inversely proportional to the number of try/catch blocks you
write, and the reason for that is the use of classes with destructors
eliminates the need to write try/catch in most cases.
Doug Harrison [MVP]
<newc...@flounder.com> wrote:
>>Yeah, well, you're talking about MSDN, and you know what you think about
>>MSDN. :)
>****
>And with justification!
>****
I wasn't disagreeing. :)
>This limitation is not documented, and therefore if the lock is passed by
>reference/pointer to another thread, there is a race condition.
That would be a fatal misuse of CSingleLock. I don't know if it is
documented as such, but lock classes in general and CSingleLock in
particular are intended to be used to define auto variables for use only by
the thread that constructs them.
neilsolent
> I've used lock classes extensively for 10+ years, and I can't recall ever
> needing to use a function such as CSingleLock::IsLocked, nor can I recall
>coding myself into contortions to avoid it. Can you give me an example or
> two that demonstrates why you need to do this?
It is really very simple, I am surprised you can't imagine any example
of this, e.g:
void Function1()
{
CSingleLock mylock(&lock, FALSE);
if ( ...condition1 ..)
{
lock.Lock();
// do something with shared resource
}
if ( ... condition2 ..)
{
lock.Lock();
// do something else with shared resource
}
}
sometimes condition1 will be true, sometimes condition2, and sometimes
both.
> Please don't take this the wrong way, but if you need to do that on
> anything but an exceedingly rare basis, I'd have to suspect you're using
> CSingleLock in less than an optimal fashion. It makes me wonder if you're
> using a CSingleLock as a class member, or you're passing CSingleLock by
> reference from function to function, etc, which would almost certainly
> represent invalid use of this class for the "critical section" object you
> mentioned in your first message.
Nope, I only use CSingleLock on the stack, and only by one thread. I
wouldn't dream of doing any of these things you mention.
I use IsLocked() only in my SafeLock() function as a programming best
practice - to write neater code, where I don't have to keep checking
the state of CSingleLock.
My code is advanced in terms of intricate thread interactions.
Joseph M. Newcomer
void Function1()
{
CSingleLock mylock(&lock);
if(!condition1 && !condition2)
return;
if(condition1)
do stuff
if(condition2)
do stuff
}
neilsolent
Joseph M. Newcomer wrote:
> Well, one solution is
>
> void Function1()
> {
> CSingleLock mylock(&lock);
> if(!condition1 && !condition2)
> return;
> if(condition1)
> do stuff
> if(condition2)
> do stuff
> }
>
So why is that any better than using SafeLock() ?
What happens when there are 3,4,5,12 if statements - will the code be
nice to read and maintainable ?
Doug Harrison [MVP]
<ne...@solenttechnology.co.uk> wrote:
>It is really very simple, I am surprised you can't imagine any example
>of this, e.g:
>
>void Function1()
>{
> CSingleLock mylock(&lock, FALSE);
I would distinguish between the synchronization object "lock" and the lock
object "mylock". I like to use "mx" as a generic mutex name and "lk" as a
generic lock object name:
CSingleLock lk(&mx, FALSE);
> if ( ...condition1 ..)
> {
> lock.Lock();
See how confusing it is? You should be locking "mylock".
> // do something with shared resource
> }
>
> if ( ... condition2 ..)
> {
> lock.Lock();
>
> // do something else with shared resource
> }
>}
>
>sometimes condition1 will be true, sometimes condition2, and sometimes
>both.
My first question would be whether it's valid to test the conditions
without holding the lock. (Google "double checked locking pattern" to see
how subtle this can be to determine.) My second thought would be to
consider what it means for condition1 to be true. When condition1 is true,
the testing of condition2 and the (possible) execution of its block is
atomic WRT the execution of the code in the first block, and there is a
memory barrier before the testing of condition2 that is not present when
condition1 is false. This could hide bugs which may reveal themselves when
condition1 happens to be false. Depending on what condition2 does, it can
also make it harder to reason about deadlock scenarios, because the mutex
may or may not be held when condition2 is evaluated.
So as a general rule, I prefer to avoid situations in which a lock may or
may not be held, as is the case for the evaluation of your condition2,
because it increases complexity. I would lean to locking the whole
function, or if it is truly valid to test the conditions without holding
the lock, getting rid of the function-wide lock object and using local
locks inside the controlled blocks.
>Nope, I only use CSingleLock on the stack, and only by one thread. I
>wouldn't dream of doing any of these things you mention.
>I use IsLocked() only in my SafeLock() function as a programming best
>practice - to write neater code, where I don't have to keep checking
>the state of CSingleLock.
>My code is advanced in terms of intricate thread interactions.
Sure, but I'm wondering if it would be possible and reasonable to eliminate
checking the state, in the interest of simplifying and improving the
design, which helps a lot with correctness.
Joseph M. Newcomer
void Function1()
{
CSingleLock mylock(&lock);
mylock.Lock();
if(condition1)
...
if(condition2)
...
}
joe
neilsolent
Doug
While I appreciate your comments - remember you just asked me to
provide an example.
That is all this was! This is not my actual code.
neilsolent
Joseph M. Newcomer wrote:
> I would have done it as my equivalent of
>
> void Function1()
> {
> CSingleLock mylock(&lock);
> mylock.Lock();
> if(condition1)
> ...
> if(condition2)
> ...
But now you are locking the CS unnecessarily (if condition1 and
condition2 are both FALSE).
Doug Harrison [MVP]
This desire to avoid locking a mutex "unless necessary" is what led some
very smart people astray with the "double checked locking pattern" I
mentioned in another message. You need to specify what the "conditions" are
and how they're determined to reason about this in a meaningful way.
Doug Harrison [MVP]
Your pseudocode only raised the questions I gave in my last message.
Thinking about it some more, the following elaboration of your pseudocode
would be valid given a recursive lock class:
void my_free(void* p1, void* p2)
{
CSingleLock lk(&mx_heap, FALSE);
if (p1) // ( ...condition1 ..)
{
lk.Lock();
my_free_nolock(p1);
}
if (p2) // ( ...condition2 ..)
{
lk.Lock();
my_free_nolock(p2);
}
}
I don't think it's worthwhile to micromanage the locking in that way, but
the code is correct; the things I discussed in my last message do not apply
to it. However, this approach is still more verbose, complex, and fragile
under maintenance than locking the whole function, and any performance gain
is likely negligible, so by default, I would write:
void my_heap_free(void* p1, void* p2)
{
CSingleLock lk(&mx, true);
my_heap_free_nolock(p1); // This function also tests for NULL.
my_heap_free_nolock(p2);
malach...@gmail.com
Do it this way instead, which (I think) emphasizes the two points that
Doug Harrison has been trying to make: RAII is the better paradigm,
and don't call CSingleLock::Lock more than a single time:
void Function1()
{
if (condition1)
{
CSingleLock lk(&m_cs, TRUE);
/* do stuff for condition 1 */
}
if (condition2)
{
CSingleLock lk(&m_cs, TRUE);
/* do stuff for condition 2 */
}
/* etc for N times */
if (conditionN)
{
CSingleLock lk(&m_cs, TRUE);
/* do stuff for condition N */
}
}
Note that although there are N CSingleLock objects all named "lk", all
of them are different, and each is an automatic variable whose
lifetime is precisely the set of enclosing braces from the "if"
statement. Because of this limited lifetime, you get RAII. Because
each CSingleLock object is different, you call Lock() on each only
exactly once.
And, per your concern about locking the CS "unnecessarily", the above
code doesn't. However, I also agree with Doug that this might not be
a true concern. It's true in general that you should hold a lock on
shared data for as brief a period as possible, to promote efficient
multi-threading, but unless all of your conditions are extensive, it's
probably clearer to hold the lock for the entirety of "Function1", as
suggested by JMN. Stated another way, it's hard to see the benefit of
locking and unlocking frequently, at the expense of obscuring the
purpose of the code.
Joseph M. Newcomer
CRITICAL_SECTION will spin-lock prior to calling the kernel, so the performance impact is
unlikely to be measurable.
joe
neilsolent
> And, per your concern about locking the CS "unnecessarily", the above
> code doesn't. However, I also agree with Doug that this might not be
> a true concern. It's true in general that you should hold a lock on
> shared data for as brief a period as possible, to promote efficient
> multi-threading, but unless all of your conditions are extensive, it's
> probably clearer to hold the lock for the entirety of "Function1", as
> suggested by JMN. Stated another way, it's hard to see the benefit of
> locking and unlocking frequently, at the expense of obscuring the
> purpose of the code.
As I said, I just posted one example. In my real code, there are file
compress/decompress operations going on, which take time (a lot of
time in extreme cases) and multiple ciritical sections. It is not a
good idea to hold one lock open for a whole function "just in case".
With mutiple locks, deadlock is also a concern - the only way to avoid
deadlock is to always lock critical sections in the same order.
For example - any thread can lock critical sections A and B at any
time. If a thread needs to lock both of them, if must lock A first,
then B, to avoid deadlock.
If a thread already has B locked (and not A), then later needs to lock
A, it MUST unlock B first, then lock A, then lock B. The situation has
actually happened in my code.
The point I am trying to make is - threading and locks are complicated
enough - so a function like SafeLock() helps simplify thngs and
promotes more maintainable code. I agree - very simple examples there
is no need for it. I am talking about complicated examples.
Anyway, thanks for the input - the original question I had is answered
many posts ago!
neilsolent
Joseph M. Newcomer wrote:
> Yes, but for how long? Tens of nanseconds? Will it really matter? Note that
> CRITICAL_SECTION will spin-lock prior to calling the kernel, so the performance impact is
> unlikely to be measurable.
> joe
What if the function is being called thousands of times per second?
It matters to me! I try to write code as efficiently as possible.
Alexander Grigoriev
"neilsolent" <ne...@solenttechnology.co.uk> wrote in message
news:876f87da-419e-49ec...@d45g2000hsc.googlegroups.com...
neilsolent
Alexander Grigoriev wrote:
> Thousands times per second is NOTHING for a critical section.
>
OK - fine. What if the Critical Section is locked by another thread?
The thread calling the function will then block - completely un-
necessarily - using the "just in case" policy. And the delay could be
several seconds (in my code) if a large compress operation is being
done.
Joseph M. Newcomer
So what? You are confusing "efficiently as possible" with "efficiently as possible". One
refers to the cost of execution, one refers to the cost of development.
If you are having collisions in a critical section at the rate of thousands per second,
there is something deeply wrong with the design of your code. You should not be trying to
synchronize anything that has such a high collision potential. And how is locking it 20
times if all the locking conditions are met "more efficient" than locking it once if no
conditions are met? You are talking nonsense here, because you are talking about
preoptimzing in the absence of any measured performance data. If most of the conditions
are true most of the time, your "more efficient" implementation is in fact LESS efficient!
I always get suspicious of people who start talking about efficiency before they start
talking about measurement of performance or models of performance. In your case, the
worst-case scenario is not just a little bit less efficient, but potentially orders of
magnitude less efficient, than locking once. In addition, if your critical section is
locked thousands of times per second, you should start questioning the design.
Here is a good rule to remember: optimizing lines of code (your approach) will buy you
single-digit-percentage performance improvements. Optimizing architecture (such as not
locking in the inner part of a loop) will buy you orders of magnitude performance
improvement. I spent 15 years doing performance measurement, and there were no exceptions
to this rule (which I developed empirically by seeing what happened when people tried to
pre-optimize their code without gathering performance data or studying the program
architecture). You are pre-optimizing lines of code, and that is fundamentally wrong at
this point. You do not optimize lines of code until you know where the expensive lines of
code are.
joe
Joseph M. Newcomer
On Fri, 27 Jun 2008 22:23:59 -0700 (PDT), neilsolent <ne...@solenttechnology.co.uk> wrote:
>
>> And, per your concern about locking the CS "unnecessarily", the above
>> code doesn't. However, I also agree with Doug that this might not be
>> a true concern. It's true in general that you should hold a lock on
>> shared data for as brief a period as possible, to promote efficient
>> multi-threading, but unless all of your conditions are extensive, it's
>> probably clearer to hold the lock for the entirety of "Function1", as
>> suggested by JMN. Stated another way, it's hard to see the benefit of
>> locking and unlocking frequently, at the expense of obscuring the
>> purpose of the code.
>
>As I said, I just posted one example. In my real code, there are file
>compress/decompress operations going on, which take time (a lot of
>time in extreme cases)
****
First principle of locking performance: lock the smallest amount of data for the shortest
possible time. If you have operations that take a long time, they should not be executed
within a critical section. Find a way to work on unlocked data. This is an architecture
issue. You can't take naive algorithms, toss locks on them like pixie dust, and expect to
get either correctness or good performance out of them.
Second principle of locking performance: do not lock anything that has a high collision
potential unless you know that it absolutely, positively, MUST be shared. Collisions cost
you serious performance, even if the lock is very brief. If you have locking at this
level, you should consider redesign of the algorithms to reduce or eliminate locking.
Example: I want to have a reader and writer working on the same buffer. The reader is
reading the data and compressing it, the writer is adding new data to it.
Solution: have the compressor work on a large chunk. Ignore anything beyond the large
chunk. When the compressor has finished its chunk, that's it. It's compressed. If
anything has been added, compress it later.
Solution: pipeline the algorithm so that it works in stages, where each stage receives a
notification of the part of the buffer it is allowed to work on. While it is working,
there is never any change in that part of the buffer. The reader and writer each
pipeline.
Solution: double-buffer. One buffer is being compressed while the other buffer is being
filled.
Note that none of these require fine-grain locking at high rates. When a buffer is
compressed, and the resulting data displayed/written/otherwise-disposed-of, that buffer
gets added back to the pool of buffers. When a buffer is completely filled, it is
dispatched to the compressor and a new buffer is obtained. The locks are infrequent and
trivial, and deal with pointer manipulations.
Architecture is ALL that matters for concurrency. In general, taking a single-threaded
algorithm, or sequence of single-threaded algorithms, and the naive approach that they can
somehow be multiplexed together if there are only enough locks done, simply doesn't
survive. You either get lock failures, deadlocks, or performance bottlenecks.
You have to redesign the whole architecture from scratch, decompose the algorithms into
non-conflicting components, and proceed from there.
I write highly-multithreaded code working on complex data, and I usually do it without
using a single synchronization primivite explicitly. I do rely on the deep
synchronization of PostMessage/PostThreadMessage/PostQueuedCompletionStatus and their
complementary receiving functions, but I don't use critical sections if I can help it, and
I can almost always (recently, just always) avoid their use. When I get code from other
people that doesn't work, it is usually full of them (I recently turned down one project
because the client said "It's almost ready, but it has some synchronization problems. It
has about 20 CRITICAL_SECTIONs, and..." and that the point I realized that this was 20
CRITICAL_SECTIONs too many, and most likely the only way to salvage the project at all was
to completely rewrite it. I was not about to debug ANY piece of code with 20
CRITICAL_SECTIONs in it, because I knew it was almost certainly wrong, and not at a
superficial level that can be easily fixed, but at deep levels that would end up requiring
a major rewrite anyway.
A friend of mine just finished his PhD on analysis of concurrent code, and he gave me this
advice: "Everyone gets concurrency wrong all of the time. That's close enough to 100%
true that you can assume it any time you confront a piece of concurrent code".
I told him I avoid this by never writing code that accesses data concurrently. My code
comes up quickly, and runs correctly, and is easy to write, debug, and maintain. It is
highly parallel, but it uses patterns that do not require explicit locking.
joe
****
>and multiple ciritical sections. It is not a
>good idea to hold one lock open for a whole function "just in case".
>With mutiple locks, deadlock is also a concern - the only way to avoid
>deadlock is to always lock critical sections in the same order.
****
When possible, avoid multiple locking. If ANY part of the code is lengthy, then using a
lock even on that section is inappropriate. Note also that in your design, if you invoke
Lock in condition1, it WILL be held until condition 20, even if the remaining 19
conditions are false, so you haven't gained anything over my suggestion. If you simply
lock and unlock, that's fine, but then your fundamental issue goes away. Had you written
code of the form
if(condition1)
{
CSingleLock lock(&mutex);
...stuff
}
if(condition2)
{
CSingleLock lock(&mutex);
...stuff
}
then the problem you are concerned with would not have arisen, you would not be locking
for lengthy periods, etc.
Note that "same order" is more restrictive than the actual deadlock-avoidance algorithm,
"canonical order". Canonical order only requires that the lock that is locked have a
sequence number greater than all existing locks that have been locked along that path of
execution. That's quite a different requirement than locking all locks in the same order.
****
>
>For example - any thread can lock critical sections A and B at any
>time. If a thread needs to lock both of them, if must lock A first,
>then B, to avoid deadlock.
>
>If a thread already has B locked (and not A), then later needs to lock
>A, it MUST unlock B first, then lock A, then lock B. The situation has
>actually happened in my code.
>
>The point I am trying to make is - threading and locks are complicated
>enough - so a function like SafeLock() helps simplify thngs and
>promotes more maintainable code. I agree - very simple examples there
>is no need for it. I am talking about complicated examples.
****
Complicated examples are where you should not be doing locking at all.
I could talk about some of the non-locking algorithms I know about that are in use in the
kernel, but the details are all NDA. But the lesson was simple: locking hurts
performance, so don't lock if you don't need to. Redesigning the code to not require
locks is a better investment of time.
****
>
>Anyway, thanks for the input - the original question I had is answered
>many posts ago!
Doug Harrison [MVP]
<ne...@solenttechnology.co.uk> wrote:
>As I said, I just posted one example. In my real code, there are file
>compress/decompress operations going on, which take time (a lot of
>time in extreme cases) and multiple ciritical sections. It is not a
>good idea to hold one lock open for a whole function "just in case".
>With mutiple locks, deadlock is also a concern - the only way to avoid
>deadlock is to always lock critical sections in the same order.
>
>For example - any thread can lock critical sections A and B at any
>time. If a thread needs to lock both of them, if must lock A first,
>then B, to avoid deadlock.
>
>If a thread already has B locked (and not A), then later needs to lock
>A, it MUST unlock B first, then lock A, then lock B. The situation has
>actually happened in my code.
Coding as per your example would seem to increase the odds of that. Like I
said, it makes reasoning about deadlock scenarios that much harder.
>The point I am trying to make is - threading and locks are complicated
>enough - so a function like SafeLock() helps simplify thngs and
>promotes more maintainable code. I agree - very simple examples there
>is no need for it. I am talking about complicated examples.
I think SafeLock makes things more complicated, because it's optional, and
you can easily forget to use it when you need to. I'd suggest writing your
own RecursiveSingleLock class, which is quite trivial to do. It would be a
good opportunity to replace CSingleLock altogether with classes that avoid
its glaring design mistakes, such as the wrong choice for default argument
in its ctor, the ability to pass a timeout value for a CCriticalSection,
etc.
Doug Harrison [MVP]
<newc...@flounder.com> wrote:
>And how is locking it 20
>times if all the locking conditions are met "more efficient" than locking it once if no
>conditions are met?
He's not calling EnterCriticalSection 20 times in all those if's. By using
CSingleLock::IsLocked, he avoids redundant lock operations. The big
trade-off is the increase in complexity, because you never know which
if-statement is going to perform the lock, which creates tremendous
variability in the "conditions" that are tested and their controlled blocks
WRT atomicity, memory barriers, deadlock reasoning, etc. I agree with your
comments about performance vs correctness; this is not something I would do
unless its were proven to be a bottleneck, and then I'd first try to find a
design approach that makes it unnecessary.
neilsolent
Joseph M. Newcomer wrote:
> You are confusing correctness with performance.
Am I? I didn't know that. Thanks.
> If you are having collisions in a critical section at the rate of thousands per second,
> there is something deeply wrong with the design of your code.
NO I am not. I am gving you "what ifs". Write code that works under
all circumstances - it's easy to do.
You should not be trying to
> synchronize anything that has such a high collision potential. And how is locking it 20
> times if all the locking conditions are met "more efficient" than locking it once if no
> conditions are met?
Because the lock could be blocked by another thread, for a random
amount of time. If you don't need the resource, don't lock it.
You are talking nonsense here, because you are talking about
> preoptimzing in the absence of any measured performance data.
Am I? Sorry once again.
> If most of the conditions
> are true most of the time, your "more efficient" implementation is in fact LESS efficient!
> I always get suspicious of people who start talking about efficiency before they start
> talking about measurement of performance or models of performance.
Well your code will hang one thread unneccessarily for 18 months, if
the other blocks the resource for that long :-). Mine doesn't do this.
> In your case, the
> worst-case scenario is not just a little bit less efficient, but potentially orders of
> magnitude less efficient, than locking once. In addition, if your critical section is
> locked thousands of times per second, you should start questioning the design.
No it isn't, I was providing worst case scenarios.
> Here is a good rule to remember: optimizing lines of code (your approach) will buy you
> single-digit-percentage performance improvements. Optimizing architecture (such as not
> locking in the inner part of a loop) will buy you orders of magnitude performance
> improvement. I spent 15 years doing performance measurement, and there were no exceptions
> to this rule (which I developed empirically by seeing what happened when people tried to
> pre-optimize their code without gathering performance data or studying the program
> architecture). You are pre-optimizing lines of code, and that is fundamentally wrong at
> this point. You do not optimize lines of code until you know where the expensive lines of
> code are.
Sorry once again.
neilsolent
> >
> >If a thread already has B locked (and not A), then later needs to lock
> >A, it MUST unlock B first, then lock A, then lock B. The situation has
> >actually happened in my code.
>
> Coding as per your example would seem to increase the odds of that. Like I
> said, it makes reasoning about deadlock scenarios that much harder.
Sorry I don't agree. You have to carefully analyse your code anyway.
There is no magic recipe.
> >The point I am trying to make is - threading and locks are complicated
> >enough - so a function like SafeLock() helps simplify thngs and
> >promotes more maintainable code. I agree - very simple examples there
> >is no need for it. I am talking about complicated examples.
>
> I think SafeLock makes things more complicated, because it's optional, and
> you can easily forget to use it when you need to.
You can use the search function in VC++ to see where you have called
the Lock() function directlry throughout your code - takes about 3
seconds :-)
Joseph M. Newcomer
On Sat, 28 Jun 2008 11:36:42 -0700 (PDT), neilsolent <ne...@solenttechnology.co.uk> wrote:
>
>
>Joseph M. Newcomer wrote:
>
>> You are confusing correctness with performance.
>
>Am I? I didn't know that. Thanks.
****
It seems obvious.
****
>
>> If you are having collisions in a critical section at the rate of thousands per second,
>> there is something deeply wrong with the design of your code.
>
>NO I am not. I am gving you "what ifs". Write code that works under
>all circumstances - it's easy to do.
****
It is better to design code that doesn't require locking. That way you KNOW it is going
to work "under all circumstances" because there is no way to accidentally create
circumstances in the future under which it won't work (the most common failure mode of
synchronization with multiple locks is that it is fragile under maintenance)
****
>
>You should not be trying to
>> synchronize anything that has such a high collision potential. And how is locking it 20
>> times if all the locking conditions are met "more efficient" than locking it once if no
>> conditions are met?
>
>Because the lock could be blocked by another thread, for a random
>amount of time. If you don't need the resource, don't lock it.
****
If the time is more than a few microseconds, you shouldn't be locking at all. You should
be redesigning
****
>
>You are talking nonsense here, because you are talking about
>> preoptimzing in the absence of any measured performance data.
>
>Am I? Sorry once again.
****
Yep. Definitely. Building complex solutions based on hypotheticals and opinion, as
opposed to building simple solutions that rely on neither, is a losing development
strategy. There are three approaches
Don't build systems that require synchronization
Don't build systems that preoptimize locking strategies at the risk of correctness
Don't build systems that are complex unless you can prove the complexity is
justified
You are violating all of the above.
****
>
>> If most of the conditions
>> are true most of the time, your "more efficient" implementation is in fact LESS efficient!
>> I always get suspicious of people who start talking about efficiency before they start
>> talking about measurement of performance or models of performance.
>
>Well your code will hang one thread unneccessarily for 18 months, if
>the other blocks the resource for that long :-). Mine doesn't do this.
****
No, my code won't hang at all, because I wouldn't build code that required synchronization
at levels beyond a microsecond or so, and if I use it at all, my goal is < 100ns per lock,
and mostly I would try for < 20ns per lock. That's about enough time to link or unlink an
object from a shared list.
****
>
>> In your case, the
>> worst-case scenario is not just a little bit less efficient, but potentially orders of
>> magnitude less efficient, than locking once. In addition, if your critical section is
>> locked thousands of times per second, you should start questioning the design.
>
>No it isn't, I was providing worst case scenarios.
*****
Worst-case scenario: the first if-statement locks the lock and the remaining 20
if-statements are false. Same performance as locking once, far worse performance than
locking each time. If you care about performance, you will not only delay setting the
lock until it is needed, you will unlock it immediately. Therefore, there is no need to
lock it multiple times because each lock sequence is completely isolated from every other
locking sequence.
****
>
>> Here is a good rule to remember: optimizing lines of code (your approach) will buy you
>> single-digit-percentage performance improvements. Optimizing architecture (such as not
>> locking in the inner part of a loop) will buy you orders of magnitude performance
>> improvement. I spent 15 years doing performance measurement, and there were no exceptions
>> to this rule (which I developed empirically by seeing what happened when people tried to
>> pre-optimize their code without gathering performance data or studying the program
>> architecture). You are pre-optimizing lines of code, and that is fundamentally wrong at
>> this point. You do not optimize lines of code until you know where the expensive lines of
>> code are.
>
>Sorry once again.
****
I developed another principle doing performance analysis: "Ask a programmer where the
performance bottleneck is in their code, and you will get a wrong answer. Not just a
little bit wrong; wrong by orders of magnitude". This rule was 100% true, all the time,
in every situation. Including my own code, when I measured it. So I have learned to
ALWAYS distrust *opinions* of where there are performance problems, because I know they
are always wrong.
The classic example was always the programmer who completely redesigned their algorithm
for "efficiency". The resulting code was sometimes SLOWER than the original code, but
when it was faster, it was provably faster. We could see it in the numbers. Instead of
accounting for 0.05% of the total time used in the program, it now accounted for 0.025% of
the total time. There was usually some glaring design error (in retrospect) that
accounted for 30% of the time, Another that consumed 10% of the time, another that
consumed 5% of the time, and every once in a while, one that cost an order of magnitude
performance. None of these could be fixed by tweaking lines of code at the level you are
concerned about.
joe
****
Joseph M. Newcomer
representative of a bad design.
joe
Joseph M. Newcomer
****
Whoopie-doopie. So what? Can your search tell you where you *failed* to call Lock(),
where you failed to call Unlock, where you have locked something for longer than a few
microseconds, and can it establish that every execution path through the code obeys
canonical locking order? I don't think so.
I spent three months searching a huge multithreaded application to see where it was
deadlocking. I gave up, and ended up doing a Petri Net simulation to detect the cause. It
took many hours of mainframe time (although the mainframes were roughly the speed of a
286...). I could *find* all the locking sites in seconds with a search, but that told me
nothing about correctness.
What does the search ability of VC++ have to do with this problem at all?
By the way, have you noticed a pattern here? The highly-experienced multithreaded
programmers are telling you that you are not taking the right approach to the
problem...and you keep defending the approach as if it makes sense.
joe
****
neilsolent
Joseph,
To avoid deadlock with multiple critical sections, you simply need to
ensure that each thread obtains multiple locks in the same order. I
had this problem before, and I fixed it easily just using the search
capability in VC++.
I know a lot of argruments seem to have been put forward against my
approach, and I have considered them all. However I can honestly say I
do not believe that any of these arguments are really valid. I still
believe my approach makes sense. Some people just won't admit when
they are wrong - but I do (honest). I am wrong a lot of the time about
a lot of things :-)
Joseph M. Newcomer
****
It is a very unusual program where a search of the locks can tell you every execution path
that leads to them and demonstrate beyond any doubt that every such execution path
acquires locks in canonical order.
joe