Summary:	ASTERISK-08175: [patch] app_queue device state change race
Reporter:	Tim Ringenbach at Asteria Solutions Group (tim_ringenbach)	Labels:
Date Opened:	2006-11-22 16:44:34.000-0600	Date Closed:	2007-10-15 15:40:40
Priority:	Minor	Regression?	No
Status:	Closed/Complete	Components:	Applications/app_queue
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) 8407.patch ( 1) app_queue_devstate_threadfix-1.2.13.diff
Description:	Because device state changes are done in their own thread (one thread per state change), they happen in a random order. This is bad, if a device changes states quickly enough, the last change isn't always the last one recorded, so a device can be stuck in use forever. The patch fixes that by having a decided thread to do the updates. Diff made with svn diff against a checkout of 1.2.13's tag.
Comments:	By: Jason Parker (jparker) 2006-11-22 16:55:09.000-0600 I didn't look too deeply at this, but does do_device_change have any calls that could be blocking, ie; waiting on network traffic or something?  If not, I think this is a good change, and we just need some people to test it. By: Tim Ringenbach at Asteria Solutions Group (tim_ringenbach) 2006-11-23 19:49:27.000-0600 do_device_change is called from the decided thread, why does it matter if it blocks? But to answer your question, it aquires locks, and calls manager_event(). I'm not sure how manager_event() is implemented or if it blocks. Aquiring locks of course can block if someone else has the lock. It doesn't do any network io, however (except the aforementioned manager_event calls). changethread(), which calls do_device_change(), also blocks. It spends most of it's life blocking on ast_cond_wait(). By: Jason Parker (jparker) 2006-11-23 20:30:18.000-0600 If it blocks, then it blocks all device state changes (because it would all be done in one thread now). By: Tim Ringenbach at Asteria Solutions Group (tim_ringenbach) 2006-11-24 10:35:36.000-0600 Ok. Well it shouldn't do any unreasonable blocking. If it blocks aquiring a lock, it's a lock that would have been aquired by the seperate thread version anyway, and all the threads would block. The main point here is to process the events in order, because processing them out of order randomly produces wrong results. By: Christopher Howard (cchasteria) 2007-02-07 11:32:35.000-0600 Any updates on this?  Has this patch been abandoned?  Wrong device states on queues is a bad thing for queue monitoring applications. By: Serge Vecher (serge-v) 2007-02-07 16:01:28.000-0600 CCHAsteria, as noted by qwell, this patch needs testing. If you have, please report results. By: gianrico fichera (gianrico) 2007-05-30 06:47:00 I have the same issue with asterisk 1.4.4. I use dynami call members and after a numeber of calls the member stuck in "in use" state and the phone stop rings:   Members:      Local/4006@assistenza with penalty 10 (dynamic) (In use) has taken 4 calls (last was 1528 secs ago)      Local/4007@assistenza with penalty 10 (dynamic) (In use) has taken 1 calls (last was 1544 secs ago) I have to do a "reload", and all works well with "not in use" state. regards By: jmls (jmls) 2007-09-12 16:39:47 is this resolved by the large changes to app_queue in 1.4 svn ? By: Martin Vit (festr) 2007-09-22 12:39:52 hi, i'm heaving the same issue with the latest SVN-branch-1.4-r82676 as described by gianrico. My members are not dynamic and are Local channels. Members:      local/21@myexten (Not in use) has taken no calls yet By: Martin Vit (festr) 2007-10-08 03:27:00 is it possible to port this into 1.4? By: Martin Vit (festr) 2007-10-08 11:11:51 maybe usefull note, that queue has ringall strategy. By: Mark Michelson (mmichelson) 2007-10-15 10:03:07 I updated the patch so that it would apply cleanly to the latest 1.4 SVN. If people could please test this and report if there are any problems, it would be much appreciated. By: Russell Bryant (russell) 2007-10-15 15:06:07 putnopvut:  Keep in mind that this has already been done in trunk.  You may just want to copy the code from trunk back to 1.4 for this, so that it's the same.  Or, I can do it if you want. By: Digium Subversion (svnbot) 2007-10-15 15:39:26 Repository: asterisk Revision: 85717 U   branches/1.4/apps/app_queue.c ------------------------------------------------------------------------ r85717 | russell | 2007-10-15 15:39:25 -0500 (Mon, 15 Oct 2007) | 7 lines Previously, app_queue created a thread to handle every single device state change.  I changed this a while ago in trunk for performance reasons.  However, bug 8407 points out that it is actually a race condition, causing device state changes to get processed in random order.  So, I backported my changes from trunk to 1.4. (closes issue ASTERISK-8175, patch provided by tim_ringenbach, committed patch by me) ------------------------------------------------------------------------ By: Digium Subversion (svnbot) 2007-10-15 15:40:40 Repository: asterisk Revision: 85719 _U  trunk/ ------------------------------------------------------------------------ r85719 | russell | 2007-10-15 15:40:40 -0500 (Mon, 15 Oct 2007) | 14 lines Blocked revisions 85717 via svnmerge ........ r85717 | russell | 2007-10-15 15:59:27 -0500 (Mon, 15 Oct 2007) | 7 lines Previously, app_queue created a thread to handle every single device state change.  I changed this a while ago in trunk for performance reasons.  However, bug 8407 points out that it is actually a race condition, causing device state changes to get processed in random order.  So, I backported my changes from trunk to 1.4. (closes issue ASTERISK-8175, patch provided by tim_ringenbach, committed patch by me) ........ ------------------------------------------------------------------------