Summary:	ASTERISK-15630: [patch] [regression] autofill=no always IGNORED.
Reporter:	Matt King, M.A. Oxon. (kebl0155)	Labels:
Date Opened:	2010-02-16 05:28:58.000-0600	Date Closed:	2011-06-27 12:46:03
Priority:	Major	Regression?	Yes
Status:	Closed/Complete	Components:	Applications/app_queue
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) app_queue_no_autofill.v1.patch ( 1) app_queue_no_autofill.v2.patch
Description:	Autofill=yes allows calls to be placed to agents from callers who are not at the front of the queue. While this may be desirable in some cases, it causes the following problems in others: - Callers may get stuck in the queue (particularly with small teams of agents) - Wait times and position announcements may be upset if the queue is not first-come-first-served. The default behaviour is therefore autofill=no.  However, a bug was introduced in Asterisk 1.4.25 which overrides this setting. ****** ADDITIONAL INFORMATION ****** The bug occurred with the introduction of the num_available_members() function in 1.4.25.  The bug now infests all streams of asterisk that support the autofill parameter (1.4, 1.6, 1.6.2 and trunk), which all allow calls to come through from members further back in the queue. The attached patch fixes the bug 1.6.2.x, and is applicable to the other streams too. Regarding the patch, I'm not sure whether any locks need to be obtained before checking for autofill or the caller's position - perhaps someone can advise. This patch has been in place at one of our customers all morning without issue, and is likely to solve bugs 16823 and 16775. This is my first Asterisk patch so I hope I did it right...
Comments:	By: Mark Michelson (mmichelson) 2010-02-17 10:16:38.000-0600 I had written up a really nice long note here, and then accidentally navigated off the page and completely lost it :( The gist of the note though is that it's not actually the num_available_members() function that's causing the problem, but the combination of it plus the "pending" status of queue_ent structures. With autofill disabled, num_available_members correctly reports that only 1 member is available, even if multiple actually are. The problem is that when determining if the queue_ent is close enough to the front of the line to try calling a queue member, we do not increment the idx field if the current queue_ent we are iterating over is in the "pending" state. This state is entered when the caller is attempting to call a queue member but has not yet actually been connected. As such, callers beyond the front of the queue may attempt to call queue members if all callers ahead in the queue are pending. The conclusion, based on reading the commit and issue that caused the pending status to be introduced, is that the pending status should only be honored when autofill is turned on. As such, there are multiple ways to fix this. I had uploaded a fix of my own, but when making the comparison to your patch, mine really isn't any better. All of this is to say your patch looks fine by me. I just rambled on here so that it's clear what is *really* the problem here, since num_available_members is doing exactly what it is supposed to ;) Anyway, expect this to be committed soon. By: Digium Subversion (svnbot) 2010-02-17 10:39:06.000-0600 Repository: asterisk Revision: 247168 U   branches/1.4/apps/app_queue.c ------------------------------------------------------------------------ r247168 | mmichelson | 2010-02-17 10:24:17 -0600 (Wed, 17 Feb 2010) | 7 lines Make sure that when autofill is disabled that callers not in the front of the queue cannot place calls. (closes issue ASTERISK-15630) Reported by: kebl0155 Patches:      app_queue_no_autofill.v1.patch uploaded by kebl0155 (license 356) ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=247168 By: Matt King, M.A. Oxon. (kebl0155) 2010-02-17 11:58:11.000-0600 Hi there, Yes sorry - I wasn't trying to say that num_available_members was the source of the problem, just that this issue was introduced at the same time as this function. My bad! Also I had a look at the 1.2.X code and it uses qe==qe->parent->head as the check for validity - is this a better check than qe->pos==1 (which is what's in the patch)? Also do you think this is going to be applied to all the streams?  SVNBot just says 1.4... By: Matt King, M.A. Oxon. (kebl0155) 2010-03-03 10:18:01.000-0600 I checked the code and I think checking for object equivalence is more robust than looking at the queue position, so I have amended the patch (see attached). This will needs to be applied to the 1.4, 1.6 and 1.6.2 streams. By: Jonathan Rose (jrose) 2011-06-27 12:44:15.106-0500 Since the only difference between this patch and the one already committed by mmichelson has no impact on behavior or performance, we are just going to leave this as is.  There's been talk forever about completely redoing the whole queue system anyway, though no one has yet taken up the torch. By: Jonathan Rose (jrose) 2011-06-27 12:46:03.862-0500 Issue Closed, the problem is already fixed and the patch in question doesn't change anything in a consequential fashion.