Re: boost::mutex::scoped_lock conventions

From: Mark Sizer (yg-boost-users_at_[hidden])
Date: 2003-05-14 11:06:52

Thanks. I like your tight scoping. It does what I want 
(maintenance-wise), looks tidy, and avoids the redundant unlock. I've 
adopted it.

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

- Mark

P.S. You must work either with very new code or with much better than 
average programmers if you haven't run into maintenance nightmares 
revolving around people changing what they don't understand or writing 
6000 line methods.

William E. Kempf wrote:

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