From 67a7c967700189124772b7c8a4f608a80c81441d Mon Sep 17 00:00:00 2001 From: Richard Mudgett Date: Tue, 8 Jan 2013 22:55:43 +0000 Subject: [PATCH] app_queue: Fix multiple calls to a queue member that is in only one queue. When ringinuse=no queue members can receive more than one call if these calls happen at nearly the same time. * Fix so a queue member does not receive more than one call from a queue. NOTE: This fix does not prevent multiple calls to a member if the member is in more than one queue. * Did some refactoring to eliminate some code redundancy. (issue ASTERISK-16115) Reported by: nik600 Patches: jira_asterisk_16115_single_q_v1.8.patch (license #5621) patch uploaded by rmudgett Modified * Revert the -r341580 and -r341599 changes adding the queues.conf check_state_unknown option as it was added in an attempt to fix this problem. The fix did not need to be optional. The fix should not have tried to explicitly set the device state. Setting the device state by something other than the device introduces a race condition. I also could not see how the change would be effective other than delaying the app_queue code long enough for the device state to propagate to app_queue. ........ Merged revisions 378663 from http://svn.asterisk.org/svn/asterisk/branches/1.8 git-svn-id: https://origsvn.digium.com/svn/asterisk/branches/10@378683 65c4cc65-6c06-0410-ace0-fbb531ad65f3 --- CHANGES | 2 - UPGRADE.txt | 3 + apps/app_queue.c | 190 ++++++++++++++++++++++++------------- configs/queues.conf.sample | 7 -- 4 files changed, 128 insertions(+), 74 deletions(-) diff --git a/CHANGES b/CHANGES index 5ddbd66148..d20eb5b4a1 100644 --- a/CHANGES +++ b/CHANGES @@ -287,8 +287,6 @@ Queue changes * Added member option ignorebusy this when set and ringinuse is not will allow per member control of multiple calls as ringinuse does for the Queue. - * Added global option check_state_unknown to enforce checking of device state - when the device state is unknown app_queue will see unknown as available. Applications ------------ diff --git a/UPGRADE.txt b/UPGRADE.txt index 8083b703a8..8b494a2afd 100644 --- a/UPGRADE.txt +++ b/UPGRADE.txt @@ -25,6 +25,9 @@ From 10.11 to 10.12: part of a character set in the dialplan extensions. The code now consistently ignores these characters when matching dialplan extensions. +* Removed the queues.conf check_state_unknown option. It is no longer + necessary. + From 10.9 to 10.10: * Queue strategy rrmemory now has a predictable order similar to strategy diff --git a/apps/app_queue.c b/apps/app_queue.c index f8f6012e87..c2b1bf3478 100644 --- a/apps/app_queue.c +++ b/apps/app_queue.c @@ -952,9 +952,6 @@ static int update_cdr = 0; /*! \brief queues.conf [general] option */ static int negative_penalty_invalid = 0; -/*! \brief queues.conf [general] option */ -static int check_state_unknown = 0; - enum queue_result { QUEUE_UNKNOWN = 0, QUEUE_TIMEOUT = 1, @@ -1067,6 +1064,7 @@ struct member { struct call_queue *lastqueue; /*!< Last queue we received a call */ unsigned int dead:1; /*!< Used to detect members deleted in realtime */ unsigned int delme:1; /*!< Flag to delete entry on reload */ + unsigned int call_pending:1; /*!< TRUE if the Q is attempting to place a call to the member. */ char rt_uniqueid[80]; /*!< Unique id of realtime member entry */ unsigned int ignorebusy:1; /*!< Flag to ignore member if the status is not available */ }; @@ -3134,6 +3132,113 @@ static char *vars2manager(struct ast_channel *chan, char *vars, size_t len) return vars; } +/*! + * \internal + * \brief Check if the member status is available. + * + * \param status Member status to check if available. + * + * \retval non-zero if the member status is available. + */ +static int member_status_available(int status) +{ + return status == AST_DEVICE_NOT_INUSE || status == AST_DEVICE_UNKNOWN; +} + +/*! + * \internal + * \brief Clear the member call pending flag. + * + * \param mem Queue member. + * + * \return Nothing + */ +static void member_call_pending_clear(struct member *mem) +{ + ao2_lock(mem); + mem->call_pending = 0; + ao2_unlock(mem); +} + +/*! + * \internal + * \brief Set the member call pending flag. + * + * \param mem Queue member. + * + * \retval non-zero if call pending flag was already set. + */ +static int member_call_pending_set(struct member *mem) +{ + int old_pending; + + ao2_lock(mem); + old_pending = mem->call_pending; + mem->call_pending = 1; + ao2_unlock(mem); + + return old_pending; +} + +/*! + * \internal + * \brief Determine if can ring a queue entry. + * + * \param qe Queue entry to check. + * \param call Member call attempt. + * + * \retval non-zero if an entry can be called. + */ +static int can_ring_entry(struct queue_ent *qe, struct callattempt *call) +{ + if (call->member->paused) { + ast_debug(1, "%s paused, can't receive call\n", call->interface); + return 0; + } + + if ((!qe->parent->ringinuse || !call->member->ignorebusy) + && !member_status_available(call->member->status)) { + ast_debug(1, "%s not available, can't receive call\n", call->interface); + return 0; + } + + if ((call->lastqueue && call->lastqueue->wrapuptime && (time(NULL) - call->lastcall < call->lastqueue->wrapuptime)) + || (!call->lastqueue && qe->parent->wrapuptime && (time(NULL) - call->lastcall < qe->parent->wrapuptime))) { + ast_debug(1, "Wrapuptime not yet expired on queue %s for %s\n", + (call->lastqueue ? call->lastqueue->name : qe->parent->name), + call->interface); + return 0; + } + + if (use_weight && compare_weight(qe->parent, call->member)) { + ast_debug(1, "Priority queue delaying call to %s:%s\n", + qe->parent->name, call->interface); + return 0; + } + + if (!qe->parent->ringinuse || !call->member->ignorebusy) { + if (member_call_pending_set(call->member)) { + ast_debug(1, "%s has another call pending, can't receive call\n", + call->interface); + return 0; + } + + /* + * The queue member is available. Get current status to be sure + * because the device state and extension state callbacks may + * not have updated the status yet. + */ + if (!member_status_available(get_queue_member_status(call->member))) { + ast_debug(1, "%s actually not available, can't receive call\n", + call->interface); + member_call_pending_clear(call->member); + return 0; + } + } + + return 1; +} + /*! * \brief Part 2 of ring_one * @@ -3155,60 +3260,17 @@ static int ring_entry(struct queue_ent *qe, struct callattempt *tmp, int *busies char tech[256]; char *location; const char *macrocontext, *macroexten; - enum ast_device_state newstate; /* on entry here, we know that tmp->chan == NULL */ - if (tmp->member->paused) { - ast_debug(1, "%s paused, can't receive call\n", tmp->interface); + if (!can_ring_entry(qe, tmp)) { if (qe->chan->cdr) { ast_cdr_busy(qe->chan->cdr); } tmp->stillgoing = 0; - (*busies)++; - return 0; - } - - if ((tmp->lastqueue && tmp->lastqueue->wrapuptime && (time(NULL) - tmp->lastcall < tmp->lastqueue->wrapuptime)) || - (!tmp->lastqueue && qe->parent->wrapuptime && (time(NULL) - tmp->lastcall < qe->parent->wrapuptime))) { - ast_debug(1, "Wrapuptime not yet expired on queue %s for %s\n", - (tmp->lastqueue ? tmp->lastqueue->name : qe->parent->name), tmp->interface); - if (qe->chan->cdr) { - ast_cdr_busy(qe->chan->cdr); - } - tmp->stillgoing = 0; - (*busies)++; - return 0; - } - - if (!qe->parent->ringinuse || !tmp->member->ignorebusy) { - if (check_state_unknown && (tmp->member->status == AST_DEVICE_UNKNOWN)) { - newstate = ast_device_state(tmp->member->interface); - if (newstate != tmp->member->status) { - ast_log(LOG_WARNING, "Found a channel matching iterface %s while status was %s changed to %s\n", - tmp->member->interface, ast_devstate2str(tmp->member->status), ast_devstate2str(newstate)); - ast_devstate_changed_literal(newstate, AST_DEVSTATE_CACHABLE, tmp->member->interface); - } - } - if ((tmp->member->status != AST_DEVICE_NOT_INUSE) && (tmp->member->status != AST_DEVICE_UNKNOWN)) { - ast_debug(1, "%s in use, can't receive call\n", tmp->interface); - if (qe->chan->cdr) { - ast_cdr_busy(qe->chan->cdr); - } - tmp->stillgoing = 0; - (*busies)++; - return 0; - } - } - - if (use_weight && compare_weight(qe->parent,tmp->member)) { - ast_debug(1, "Priority queue delaying call to %s:%s\n", qe->parent->name, tmp->interface); - if (qe->chan->cdr) { - ast_cdr_busy(qe->chan->cdr); - } - tmp->stillgoing = 0; - (*busies)++; + ++*busies; return 0; } + ast_assert(qe->parent->ringinuse || tmp->member->call_pending); ast_copy_string(tech, tmp->interface, sizeof(tech)); if ((location = strchr(tech, '/'))) @@ -3219,18 +3281,18 @@ static int ring_entry(struct queue_ent *qe, struct callattempt *tmp, int *busies /* Request the peer */ tmp->chan = ast_request(tech, qe->chan->nativeformats, qe->chan, location, &status); if (!tmp->chan) { /* If we can't, just go on to the next call */ - if (qe->chan->cdr) { - ast_cdr_busy(qe->chan->cdr); - } - tmp->stillgoing = 0; - ao2_lock(qe->parent); - update_status(qe->parent, tmp->member, get_queue_member_status(tmp->member)); qe->parent->rrpos++; qe->linpos++; ao2_unlock(qe->parent); - (*busies)++; + member_call_pending_clear(tmp->member); + + if (qe->chan->cdr) { + ast_cdr_busy(qe->chan->cdr); + } + tmp->stillgoing = 0; + ++*busies; return 0; } @@ -3305,10 +3367,12 @@ static int ring_entry(struct queue_ent *qe, struct callattempt *tmp, int *busies /* Again, keep going even if there's an error */ ast_verb(3, "Couldn't call %s\n", tmp->interface); do_hang(tmp); - (*busies)++; - update_status(qe->parent, tmp->member, get_queue_member_status(tmp->member)); + member_call_pending_clear(tmp->member); + ++*busies; return 0; - } else if (qe->parent->eventwhencalled) { + } + + if (qe->parent->eventwhencalled) { char vars[2048]; ast_channel_lock_both(tmp->chan, qe->chan); @@ -3342,7 +3406,7 @@ static int ring_entry(struct queue_ent *qe, struct callattempt *tmp, int *busies ast_verb(3, "Called %s\n", tmp->interface); } - update_status(qe->parent, tmp->member, get_queue_member_status(tmp->member)); + member_call_pending_clear(tmp->member); return 1; } @@ -6991,10 +7055,6 @@ static void queue_set_global_params(struct ast_config *cfg) if ((general_val = ast_variable_retrieve(cfg, "general", "negative_penalty_invalid"))) { negative_penalty_invalid = ast_true(general_val); } - check_state_unknown = 0; - if ((general_val = ast_variable_retrieve(cfg, "general", "check_state_unknown"))) { - check_state_unknown = ast_true(general_val); - } } /*! \brief reload information pertaining to a single member @@ -7731,7 +7791,7 @@ static int manager_queues_summary(struct mansession *s, const struct message *m) while ((mem = ao2_iterator_next(&mem_iter))) { if ((mem->status != AST_DEVICE_UNAVAILABLE) && (mem->status != AST_DEVICE_INVALID)) { ++qmemcount; - if (((mem->status == AST_DEVICE_NOT_INUSE) || (mem->status == AST_DEVICE_UNKNOWN)) && !(mem->paused)) { + if (member_status_available(mem->status) && !mem->paused) { ++qmemavail; } } diff --git a/configs/queues.conf.sample b/configs/queues.conf.sample index b4d50931c2..ec4f6267eb 100644 --- a/configs/queues.conf.sample +++ b/configs/queues.conf.sample @@ -65,13 +65,6 @@ monitor-type = MixMonitor ; ;negative_penalty_invalid = no ; -; app_queue allows calls to members in a "Unknown" state to be treated as available -; setting check_state_unknown = yes will cause app_queue to query the channel driver -; to better determine the state this only applies to queues with ringinuse or ignorebusy -; set appropriately. -; -;check_state_unknown = no -; ;[markq] ; ; A sample call queue