[asterisk-dev] Locking, coding guidelines addition

[Russell Bryant]

On Jun 27, 2008, at 5:14 PM, Tzafrir Cohen wrote:
>>      unlock(pvt);
>>      lock(ast_channel);
>>      lock(pvt);
>
> Putting on my Captain Obvious mask:
>
> (The resulting code is not fundementally different from the code here)

You're right.  In fact, many of the places where the trylock loop is  
used (the ones where we loop forever until successful), this would be  
equivalent and actually more efficient.

> If we can just give up the lock, why was it held in the first place?


That's a good question.  It's tough to answer this in a general way.

It's quite often that processing is being done right before and right  
after this construct that requires the lock being held.

Another thing to note is that locks in Asterisk are used in ...  
interesting ways.  In a perfect world, locks would only be used for  
critical sections.  Unfortunately, that's not the case.  Locks are  
often (ab)used to ensure that for some given code path, an object does  
not get destroyed out from under you.  We have made vast improvements  
in object handling in recent times (using reference counted objects  
instead of lock abuse), but the usage I refer to still exists in quite  
a few places, including the current handling of ast_channel.

Quite frankly, it makes for messy code that sometimes does unexpected  
things.  There are code paths that unlock and re-lock a lock out from  
under you.  There are places all over chan_iax2 where this is done and  
the result is often that even though you called a function with a pvt  
lock held, the pvt structure may get destroyed by another thread in  
the midst of this "deadlock avoidance" unlock/lock code.  So, after  
these functions return, you have to re-check to see if the pvt still  
exists.  It's a pain and makes it such that you have to be _very_  
careful when modifying that code.  In other places, it means that an  
object may get changed even though you had it locked, because the lock  
was temporarily released internally to some function call.

--
Russell Bryant
Senior Software Engineer
Open Source Team Lead
Digium, Inc.