[Home]

Summary:ASTERISK-08671: [patch] deadlock occurs when spy leaves session using ChanSpy due to inverse locking order
Reporter:Guillermo Winkler (guillecabeza)Labels:
Date Opened:2007-01-27 16:31:34.000-0600Date Closed:2007-04-02 14:30:02
Priority:BlockerRegression?No
Status:Closed/CompleteComponents:Resources/res_features
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) app_chanspy.c.patch
Description:CHANSPY
ast_mutex_lock(&csth.spy.lock);
if (csth.spy.chan) {
  csth.spy.status = CHANSPY_DONE;
  ast_mutex_lock(&csth.spy.chan->lock);

CORE:

***ast_read(struct ast_channel *chan)***
ast_mutex_lock(&chan->lock);
***queue_frame_to_spies***
AST_LIST_TRAVERSE(&chan->spies->list, spy, list) {
ast_mutex_lock(&spy->lock);

****** ADDITIONAL INFORMATION ******

The locking order in ChanSpy is necessary to avoid crashes when spied channel hangs up, but causes potential deadlock on frame queuing(we've seen it a lot when MOH is used and adds a little delay on frame processing)

There's a chance to avoid the deadlock retrying on queue_frame_to_spies

retries = 10;
lockstatus = ast_mutex_trylock(&spy->lock);
while ((lockstatus == EBUSY) && --retries) {
  usleep(1);
  lockstatus = ast_mutex_trylock(&spy->lock);
}
if ((lockstatus == EBUSY) || (spy->status == CHANSPY_DONE)) {
  spy invalid!
}

But it's not really "nice", maybe a new locking scheme would be best?

/*********************************************************
In order to reproduce the problem just add a usleep
**********************************************************/
ast_mutex_lock(&csth.spy.lock);
if (csth.spy.chan) {
   usleep(50000)
   csth.spy.status = CHANSPY_DONE;

Comments:By: Guillermo Winkler (guillecabeza) 2007-01-31 16:15:32.000-0600

Deadlock also happens when call ends (again due to inverse locking order). Modifying app_chanspy.c solves the problem.


Disclaimer on file.
By: BJ Weschke (bweschke) 2007-02-06 01:51:51.000-0600

assigning to file since he's done quite a bit of work in this area of the core.
By: Joshua C. Colp (jcolp) 2007-02-15 19:20:03.000-0600

Would it not be possible for the channel pointer to become NULL?
By: Serge Vecher (serge-v) 2007-02-26 10:45:43.000-0600

guillecabeza: please answer file's question
By: Guillermo Winkler (guillecabeza) 2007-03-02 07:58:09.000-0600

Yes, I think it does.

If hangup is waiting on spy.lock, chan will be completly destroyed in the moment(and of course spy.chan would be NULL).
By: Joshua C. Colp (jcolp) 2007-03-20 22:56:55

I've started a branch for a replacement of chanspy/etc, called audiohooks. It's at http://svn.digium.com/svn/asterisk/team/file/ah - can you give some input on it too perhaps?
By: Guillermo Winkler (guillecabeza) 2007-03-24 13:22:44

Just checked out the branch, what's the best way to interact?
By: Joshua C. Colp (jcolp) 2007-03-27 15:13:11

If you have comments about it feel free to email them to jcolp@digium.com - as for this issue... the patch isn't exactly right as mentioned as the channel pointer may become NULL underneath it... and I don't know of any other solutions :\
By: Joshua C. Colp (jcolp) 2007-04-02 14:30:01

I'm closing this out for now since the proposed solution has it's own issues and nobody else has provided any other solutions. As I'm rewriting the spy architecture hopefully it will solve the issue.