Merged revisions 185072 via svnmerge from

https://origsvn.digium.com/svn/asterisk/trunk

................
  r185072 | mmichelson | 2009-03-30 11:26:48 -0500 (Mon, 30 Mar 2009) | 45 lines
  
  Merged revisions 185031 via svnmerge from 
  https://origsvn.digium.com/svn/asterisk/branches/1.4
  
  ........
    r185031 | mmichelson | 2009-03-30 11:17:35 -0500 (Mon, 30 Mar 2009) | 39 lines
    
    Fix queue weight behavior so that calls in low-weight queues are not inappropriately blocked.
    
    (This is copied and pasted from the review request I made for this patch)
    
    Asterisk has some odd behavior when queue weights are used. The current logic used when
    potentially calling a queue member is:
    
    If the member we are going to call is part of another queue and _that other queue has any 
    callers in it_ and has a higher weight than the queue we are calling from, then don't try 
    to contact that member. The issue here is what I have marked with underscores. If the 
    higher-weighted queue has any callers in it at all, then the queue member will be unreachable 
    from the lower-weighted queue. This has the potential to be really really bad if using a 
    queue strategy, such as leastrecent or fewestcalls, with the potential to call the same 
    member repeatedly.
    
    The fix proposed by garychen on issue 13220 is very simple and, as far as I can see, works 
    well for this situation. With this set of changes, the logic used becomes:
    
    If the member we are going to call is part of another queue, the other queue has a higher 
    weight than the queue we are calling from, and the higher weight queue has at least as many 
    callers as available members, then do not try to contact the queue member. If the higher 
    weighted queue has fewer callers than available members, then there is no reason to deny 
    the call to this member since the other queue can afford to spare a member.
    
    Since the fix involved writing a generic function for determining the number of available 
    members in the queue, I also modified the is_our_turn function to make use of the new 
    num_available_members function to determine if it is our turn to try calling a member. There 
    is one small behavior change. Before writing this patch, if you had autofill disabled, then 
    if you were the head caller in a queue, you would automatically be told that it was your 
    turn to try calling a member. This did not take into account whether there were actually any 
    queue members available to take the call. Now we actually make sure there is at least one 
    member available to take the call if autofill is disabled.
    
    (closes issue #13220)
    Reported by: garychen
    
    Review: http://reviewboard.digium.com/r/202/
  ........
................


git-svn-id: http://svn.digium.com/svn/asterisk/branches/1.6.2@185089 f38db490-d61c-443f-a65b-d21fe96a405b

1 files changed, 81 insertions, 70 deletions

diff --git a/apps/app_queue.c b/apps/app_queue.c
index 260745d65..1acf5bb43 100644
--- a/apps/app_queue.c
+++ b/apps/app_queue.c
@@ -2268,11 +2268,56 @@ static void hangupcalls(struct callattempt *outgoing, struct ast_channel *except
 	}
 }
 
-/*! 
- * \brief traverse all defined queues which have calls waiting and contain this member
- * \retval 0 if no other queue has precedence (higher weight) 
- * \retval 1 if found  
-*/
+/*!
+ * \brief Get the number of members available to accept a call.
+ *
+ * \note The queue passed in should be locked prior to this function call
+ *
+ * \param[in] q The queue for which we are couting the number of available members
+ * \return Return the number of available members in queue q
+ */
+static int num_available_members(struct call_queue *q)
+{
+	struct member *mem;
+	int avl = 0;
+	struct ao2_iterator mem_iter;
+
+	mem_iter = ao2_iterator_init(q->members, 0);
+	while ((mem = ao2_iterator_next(&mem_iter))) {
+		switch (mem->status) {
+		case AST_DEVICE_INUSE:
+			if (!q->ringinuse)
+				break;
+			/* else fall through */
+		case AST_DEVICE_NOT_INUSE:
+		case AST_DEVICE_UNKNOWN:
+			if (!mem->paused) {
+				avl++;
+			}
+			break;
+		}
+		ao2_ref(mem, -1);
+
+		/* If autofill is not enabled or if the queue's strategy is ringall, then
+		 * we really don't care about the number of available members so much as we
+		 * do that there is at least one available.
+		 *
+		 * In fact, we purposely will return from this function stating that only
+		 * one member is available if either of those conditions hold. That way,
+		 * functions which determine what action to take based on the number of available
+		 * members will operate properly. The reasoning is that even if multiple
+		 * members are available, only the head caller can actually be serviced.
+		 */
+		if ((!q->autofill || q->strategy == QUEUE_STRATEGY_RINGALL) && avl) {
+			break;
+		}
+	}
+
+	return avl;
+}
+
+/* traverse all defined queues which have calls waiting and contain this member
+   return 0 if no other queue has precedence (higher weight) or 1 if found  */
 static int compare_weight(struct call_queue *rq, struct member *member)
 {
 	struct call_queue *q;
@@ -2292,7 +2337,7 @@ static int compare_weight(struct call_queue *rq, struct member *member)
 		if (q->count && q->members) {
 			if ((mem = ao2_find(q->members, member, OBJ_POINTER))) {
 				ast_debug(1, "Found matching member %s in queue '%s'\n", mem->interface, q->name);
-				if (q->weight > rq->weight) {
+				if (q->weight > rq->weight && q->count >= num_available_members(q)) {
 					ast_debug(1, "Queue '%s' (weight %d, calls %d) is preferred over '%s' (weight %d, calls %d)\n", q->name, q->weight, q->count, rq->name, rq->weight, rq->count);
 					found = 1;
 				}
@@ -2981,78 +3026,44 @@ static struct callattempt *wait_for_answer(struct queue_ent *qe, struct callatte
 /*! 
  * \brief Check if we should start attempting to call queue members.
  *
- * The behavior of this function is dependent first on whether autofill is enabled
- * and second on whether the ring strategy is ringall. If autofill is not enabled,
- * then return true if we're the head of the queue. If autofill is enabled, then
- * we count the available members and see if the number of available members is enough
- * that given our position in the queue, we would theoretically be able to connect to
- * one of those available members
+ * A simple process, really. Count the number of members who are available
+ * to take our call and then see if we are in a position in the queue at
+ * which a member could accept our call.
+ *
+ * \param[in] qe The caller who wants to know if it is his turn
+ * \retval 0 It is not our turn
+ * \retval 1 It is our turn
  */
 static int is_our_turn(struct queue_ent *qe)
 {
 	struct queue_ent *ch;
-	struct member *cur;
-	int avl = 0;
-	int idx = 0;
 	int res;
+	int avl;
+	int idx = 0;
+	/* This needs a lock. How many members are available to be served? */
+	ao2_lock(qe->parent);
 
-	if (!qe->parent->autofill) {
-		/* Atomically read the parent head -- does not need a lock */
-		ch = qe->parent->head;
-		/* If we are now at the top of the head, break out */
-		if (ch == qe) {
-			ast_debug(1, "It's our turn (%s).\n", qe->chan->name);
-			res = 1;
-		} else {
-			ast_debug(1, "It's not our turn (%s).\n", qe->chan->name);
-			res = 0;
-		}	
+	avl = num_available_members(qe->parent);
 
-	} else {
-		/* This needs a lock. How many members are available to be served? */
-		ao2_lock(qe->parent);
-			
-		ch = qe->parent->head;
-	
-		if (qe->parent->strategy == QUEUE_STRATEGY_RINGALL) {
-			ast_debug(1, "Even though there may be multiple members available, the strategy is ringall so only the head call is allowed in\n");
-			avl = 1;
-		} else {
-			struct ao2_iterator mem_iter = ao2_iterator_init(qe->parent->members, 0);
-			while ((cur = ao2_iterator_next(&mem_iter))) {
-				switch (cur->status) {
-				case AST_DEVICE_INUSE:
-					if (!qe->parent->ringinuse)
-						break;
-					/* else fall through */
-				case AST_DEVICE_NOT_INUSE:
-				case AST_DEVICE_UNKNOWN:
-					if (!cur->paused)
-						avl++;
-					break;
-				}
-				ao2_ref(cur, -1);
-			}
-		}
+	ch = qe->parent->head;
 
-		ast_debug(1, "There are %d available members.\n", avl);
-	
-		while ((idx < avl) && (ch) && (ch != qe)) {
-			if (!ch->pending)
-				idx++;
-			ch = ch->next;			
-		}
-	
-		/* If the queue entry is within avl [the number of available members] calls from the top ... */
-		if (ch && idx < avl) {
-			ast_debug(1, "It's our turn (%s).\n", qe->chan->name);
-			res = 1;
-		} else {
-			ast_debug(1, "It's not our turn (%s).\n", qe->chan->name);
-			res = 0;
-		}
-		
-		ao2_unlock(qe->parent);
+	ast_debug(1, "There %s %d available %s.\n", avl != 1 ? "are" : "is", avl, avl != 1 ? "members" : "member");
+
+	while ((idx < avl) && (ch) && (ch != qe)) {
+		if (!ch->pending)
+			idx++;
+		ch = ch->next;			
+	}
+
+	ao2_unlock(qe->parent);
+
+	/* If the queue entry is within avl [the number of available members] calls from the top ... */
+	if (ch && idx < avl) {
+		ast_debug(1, "It's our turn (%s).\n", qe->chan->name);
+		res = 1;
+	} else {
+		ast_debug(1, "It's not our turn (%s).\n", qe->chan->name);
+		res = 0;
 	}
 
 	return res;