[asterisk-dev] [Code Review] Fix deadlock when using originate from agent

[Russell Bryant]

> On 2008-11-11 16:19:17, Tilghman Lesher wrote:
> > branches/1.4/channels/chan_agent.c, line 944
> > <http://reviewboard.digium.com/r/35/diff/1/?file=1248#file1248line944>
> >
> >     I'm curious as to why you lock before signalling.  The manpage suggests that this is not strictly necessary.  Is it only to set p->app_lock_flag ?  See my other comment, as it appears that this variable is useless.
> 
>  wrote:
>     You're right - I locked in a bunch of unnecessary places I believe so I'm going to remove them.

I have not had a chance to review these changes from an architectural viewpoint yet, but I did want to comment on this one point.

While locking around a condition signal is not _required_ necessarily, it is almost always the right thing to do if you want to get predictable results.  Otherwise, you lack proper synchronization between threads waiting on a signal, and those delivering the signal.  You end up with a race condition where you could deliver a signal _right_ before a thread goes to sleep to wait for it.


- Russell


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/35/#review119
-----------------------------------------------------------


On 2008-11-12 17:58:12, Jeff Peeler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/35/
> -----------------------------------------------------------
> 
> (Updated 2008-11-12 17:58:12)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This changes the behavior of __login_exec to use a pthread condition to wait rather than just waiting for a lock. Using a pthread condition frees the restraint of requiring the same thread to let execution continue (by unlocking the lock), which is important as originate uses multiple threads.
> 
> 
> This addresses bug 12269.
>     http://bugs.digium.com/view.php?id=12269
> 
> 
> Diffs
> -----
> 
>   branches/1.4/channels/chan_agent.c 156442 
> 
> Diff: http://reviewboard.digium.com/r/35/diff
> 
> 
> Testing
> -------
> 
> Testing done both here and by reporters show the problem is resolved, but I wanted to make sure I'm not missing anything as changing locking can be tricky.
> 
> 
> Thanks,
> 
> Jeff
> 
>