Re: [Boost-Users] Re: boost::mutex::scoped_lock conventions

From: William E. Kempf (wekempf_at_[hidden])
Date: 2003-05-14 09:16:46

Mark Sizer said:
> The hypothetical is easy enough:
>
> void myclass::doSomething()
> {
>    scoped_lock lockData( _mutexData );
>    <do something using the data>
> }
>
> After two years and three developers:
>
> void myclass::doSomething()
> {
>    scoped_lock lockData( _mutexData );
>    <do the original thing that needed the lock>
>
>    <do all sorts of additional stuff not needing lock>
> }

This doesn't illustrate it for me, for several reasons:

1) Doing more stuff that doesn't need to be locked doesn't necessarily
mean dire consequences.  We all know that holding a lock too long *can*
lead to problems, but generally it's only a problem of performance.

2) If you have a block of code in which things grow to the point that
you'd not be able to easily see the scoped lock, and thus not make this
mistake, then you've got more maitenance issues than explicit unlocking is
going to help you with.

3) Generally, most functions/blocks will be factored in such a way that
the mutex *would* have to be held the entire scope.  It's a rare case in
which you'd have a function like you illustrate above, and such cases are
generally self evident from the beginning and don't result from
maintenance changes.

> Of course we all know that no programmer would ever be so lazy as to
> modify code with which he was not completely familiar (I REALLY need
> thread IDs! - sound eerily familiar? [btw: TSS is working great.]). No
> one would ever make one method do several different things, either. All
> code is kept optimally factored over years of development <chortle>.

What do thread IDs have to do with this?

But yes, I generally do believe that properly designed code won't have any
of the characteristics you sarcastically seem to indicate does occur.  If
you work with code like that, you have much worse problems than holding
onto a lock longer than you should will help prevent.

Do you call reset() explicitly on all of your auto_ptr's?

I'm not going to tell anyone else that they shouldn't call unlock()
explicitly.  If you think it will help during maintenance, then by all
means, do so.  But I'm not convinced enough to recommend this to others.

> In an ideal world, unlocking is rarely necessary.

I hope you mean explicit unlocking!

> In the real world, I
> think it helps define the developer's intent. At the very least the next
>  programmer is presented with the obvious choice of putting the new code
>  before or after the "unlock". He can still do it wrong, but it has to
> be  concious choice.

In my own experience, it's been a concious choice to do so with implicit
unlocking as well.  I've never made the mistake you illustrate above. 
It's probably due to the fact that when dealing with synchronization, you
*HAVE* to fully understand the code being synchronized, lest you create
deadlocks or race conditions, so such areas of code are kept short,
explicit and well documented.

> Haven't you ever tracked down this bug (usually introduced by those who
> think indentation is for wimps):
> if ( <condition> )
> {
>    <do true>
> }
> else
>    <do false>
>
> that becomes:
>
> if ( <condition> )
> {
>    <do true>
> }
> else
>    <do false>
>    <do more false> // oops!
>
> Same pattern, different situation.

I don't see it as the same pattern at all.

> If I ever get around to creating a
> language, it will be indent sensitive. Screw the punctuation ('{',
> 'begin', '(', etc...).

Try Python.

> P.S. On the same note, does anyone indent inside locks (I don't)?

Yes:

void foo()
{
   { boost::mutex::scoped_lock lock(mutex);
      // code
   }
   // other code
}

Not quite what you meant, I know, but I think this illustrates the point
about RAII not necessarily leading to the problem you see.  Locks should
be held for as short of a period of time as possible, generally, which
means short blocks, even if artificial.  Short code blocks combined with
the need to carefully analyze synchronization leads to little chance of
making the mistake you illustrate.

-- 
William E. Kempf