[asterisk-bugs] [JIRA] (ASTERISK-28952) Queue wrapuptime sometimes not respected (based on stale lastcall time)

George Joseph (JIRA) noreply at issues.asterisk.org
Mon Jun 15 09:58:26 CDT 2020


     [ https://issues.asterisk.org/jira/browse/ASTERISK-28952?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

George Joseph updated ASTERISK-28952:
-------------------------------------

    Assignee: Walter Doekes

> Queue wrapuptime sometimes not respected (based on stale lastcall time)
> -----------------------------------------------------------------------
>
>                 Key: ASTERISK-28952
>                 URL: https://issues.asterisk.org/jira/browse/ASTERISK-28952
>             Project: Asterisk
>          Issue Type: Bug
>      Security Level: None
>          Components: Applications/app_queue
>    Affects Versions: 13.34.0
>            Reporter: Walter Doekes
>            Assignee: Walter Doekes
>
> Possibly related to ASTERISK-22589, we've also been seeing issues with wrapuptime not being respected.
> In our case -- we're also using realtime -- the issue appears to be the following:
> - try_calling() creates a list of potential members (Agents)
> - it copies the current state from the members to the callattempt linked list
> - ring_one() is fed the list
> - if the strategy is _not_ ringall, the members are tried sequentially
> - ring_entry() dials a member
> - member does not pick up
> - ring_entry() dials another member
> - before dialing, can_ring_entry() checks whether member->lastcall is more recent than wrapuptime seconds ago
> - if wrapuptime has not been reached, it skips this member and moves onto the next
> In our case, the callattempt-list is populated with both in-use and not-in-use members. This is no problem, as can_ring_entry() checks both the inuse state and the wrapuptime.
> However: if the callattempt-list was created _before_ the inuse member ends the call, but the attempt is first made _after_ the hangup, can_ring_entry() checks _old_ data:
> - call1: MemberX has call (is inuse)
> - call2: NewCaller calls into queue, callattempt is made with currently available lastcall times
> - call2: MemberY is tried
> - call1: MemberX ends call, stores "lastcall" time in member (but not in callattempt)
> - call2: MemberX is tried (checks stale "lastcall" time from callattempt)
> That last one _should be_:
> - call2: MemberX is skipped, after checking "lastcall" on member struct
> h3. History
> struct callattempt has always had copies of the lastcall/lastqueue:
> {noformat}
> commit b140eef0aa186020868aaf4ab663ce2aa38e73c2
> Author: Mark Spencer <markster at digium.com>
> Date:   Sat Jun 26 16:26:39 2004 +0000
>     Implement queue wide wrapuptime (bug #1707)
> ...
> @@ -1019,6 +1028,7 @@ static int try_calling(struct queue_ent *qe, char *options, char *announceoverri
>                 tmp->member = cur;              /* Never directly dereference!  Could change on reload */
>                 strncpy(tmp->tech, cur->tech, sizeof(tmp->tech)-1);
>                 strncpy(tmp->numsubst, cur->loc, sizeof(tmp->numsubst)-1);
> +               tmp->lastcall = cur->lastcall;
>                 /* If we're dialing by extension, look at the extension to know what to dial */
>                 if ((newnum = strstr(tmp->numsubst, "BYEXTENSION"))) {
>                         strncpy(restofit, newnum + strlen("BYEXTENSION"), sizeof(restofit)-1);
> {noformat}
> We were not allowed to touch the member directly.
> This was changed:
> {noformat}
> commit e5582607460aa9fe8312bdf5b184160b92d90f7b
> Author: Tilghman Lesher <tilghman at meg.abyt.es>
> Date:   Mon Sep 10 18:32:59 2007 +0000
>     Merged revisions 82155 via svnmerge from
>     https://origsvn.digium.com/svn/asterisk/branches/1.4
>     
>     ........
>     r82155 | tilghman | 2007-09-10 13:02:02 -0500 (Mon, 10 Sep 2007) | 2 lines
>     
>     Convert struct member to use refcounts (closes issue #10199)
> ...
> @@ -2601,7 +2620,7 @@ static int try_calling(struct queue_ent *qe, const char *options, char *announce
>                         goto out;
>                 }
>                 tmp->stillgoing = -1;
> -               tmp->member = cur;              /* Never directly dereference!  Could change on reload */
> +               tmp->member = cur;
>                 tmp->oldstatus = cur->status;
>                 tmp->lastcall = cur->lastcall;
>                 ast_copy_string(tmp->interface, cur->interface, sizeof(tmp->interface));
> {noformat}
> And within 4 days that feature was already used:
> {noformat}
> commit 38460271f20c45350e428b5d3a7cc07989df1b54
> Author: Mark Michelson <mmichelson at digium.com>
> Date:   Fri Sep 14 18:34:53 2007 +0000
>     Merged revisions 82396 via svnmerge from
>     https://origsvn.digium.com/svn/asterisk/branches/1.4
>     
>     ........
>     r82396 | mmichelson | 2007-09-14 13:28:36 -0500 (Fri, 14 Sep 2007) | 5 lines
>     
>     Adding member name field to manager events where they were missing before
>     
>     (closes issue #10721, reported by snar)
> ...
> @@ -1901,7 +1902,7 @@ static int ring_entry(struct queue_ent *qe, struct callattempt *tmp, int *busies
>                                         "Extension: %s\r\n"
>                                         "Priority: %d\r\n"
>                                         "%s",
> -                                       qe->parent->name, tmp->interface, qe->chan->name, tmp->chan->name,
> +                                       qe->parent->name, tmp->interface, tmp->member->membername, qe->chan->name, tmp->chan->name,
>                                         tmp->chan->cid.cid_num ? tmp->chan->cid.cid_num : "unknown",
>                                         tmp->chan->cid.cid_name ? tmp->chan->cid.cid_name : "unknown",
>                                         qe->chan->context, qe->chan->exten, qe->chan->priority,
> {noformat}
> However, the original use of the copied field remained:
> {code}
>                 struct callattempt *tmp;
>                 struct member *cur;
> ...
>                 tmp->lastcall = cur->lastcall;
> {code}
> As far as I can tell, there is no reason to not query the member directly, and drop the obsolete (and sometimes stale) copies.
> h3. Fix
> Which would look somewhat like this:
> {noformat}
> diff --git a/apps/app_queue.c b/apps/app_queue.c
> index 3a572efc2d..5d2721bdfc 100644
> --- a/apps/app_queue.c
> +++ b/apps/app_queue.c
> @@ -1443,8 +1443,6 @@ struct callattempt {
>  	struct ast_channel *chan;
>  	char interface[256];			/*!< An Asterisk dial string (not a channel name) */
>  	int metric;
> -	time_t lastcall;
> -	struct call_queue *lastqueue;
>  	struct member *member;
>  	/*! Saved connected party info from an AST_CONTROL_CONNECTED_LINE. */
>  	struct ast_party_connected_line connected;
> @@ -4236,36 +4234,38 @@ static int member_status_available(int status)
>   */
>  static int can_ring_entry(struct queue_ent *qe, struct callattempt *call)
>  {
> -	if (call->member->paused) {
> +	struct member *memberp = call->member;
> +
> +	if (memberp->paused) {
>  		ast_debug(1, "%s paused, can't receive call\n", call->interface);
>  		return 0;
>  	}
>  
> -	if (!call->member->ringinuse && !member_status_available(call->member->status)) {
> +	if (!memberp->ringinuse && !member_status_available(memberp->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))) {
> +	if ((memberp->lastqueue && memberp->lastqueue->wrapuptime && (time(NULL) - memberp->lastcall < memberp->lastqueue->wrapuptime))
> +		|| (!memberp->lastqueue && qe->parent->wrapuptime && (time(NULL) - memberp->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),
> +			(memberp->lastqueue ? memberp->lastqueue->name : qe->parent->name),
>  			call->interface);
>  		return 0;
>  	}
>  
> -	if (use_weight && compare_weight(qe->parent, call->member)) {
> +	if (use_weight && compare_weight(qe->parent, memberp)) {
>  		ast_debug(1, "Priority queue delaying call to %s:%s\n",
>  			qe->parent->name, call->interface);
>  		return 0;
>  	}
>  
> -	if (!call->member->ringinuse) {
> +	if (!memberp->ringinuse) {
>  		struct member *mem;
>  
>  		ao2_lock(pending_members);
>  
> -		mem = ao2_find(pending_members, call->member,
> +		mem = ao2_find(pending_members, memberp,
>  				  OBJ_SEARCH_OBJECT | OBJ_NOLOCK);
>  		if (mem) {
>  			/*
> @@ -4283,8 +4283,8 @@ static int can_ring_entry(struct queue_ent *qe, struct callattempt *call)
>  		 * If not found add it to the container so another queue
>  		 * won't attempt to call this member at the same time.
>  		 */
> -		ast_debug(3, "Add %s to pending_members\n", call->member->membername);
> -		ao2_link(pending_members, call->member);
> +		ast_debug(3, "Add %s to pending_members\n", memberp->membername);
> +		ao2_link(pending_members, memberp);
>  		ao2_unlock(pending_members);
>  
>  		/*
> @@ -4292,10 +4292,10 @@ static int can_ring_entry(struct queue_ent *qe, struct callattempt *call)
>  		 * 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))) {
> +		if (!member_status_available(get_queue_member_status(memberp))) {
>  			ast_debug(1, "%s actually not available, can't receive call\n",
>  				call->interface);
> -			pending_members_remove(call->member);
> +			pending_members_remove(memberp);
>  			return 0;
>  		}
>  	}
> @@ -6638,9 +6638,7 @@ static int try_calling(struct queue_ent *qe, struct ast_flags opts, char **opt_a
>  
>  		tmp->block_connected_update = block_connected_line;
>  		tmp->stillgoing = 1;
> -		tmp->member = cur;/* Place the reference for cur into callattempt. */
> -		tmp->lastcall = cur->lastcall;
> -		tmp->lastqueue = cur->lastqueue;
> +		tmp->member = cur; /* Place the reference for cur into callattempt. */
>  		ast_copy_string(tmp->interface, cur->interface, sizeof(tmp->interface));
>  		/* Calculate the metric for the appropriate strategy. */
>  		if (!calc_metric(qe->parent, cur, x++, qe, tmp)) {
> {noformat}



--
This message was sent by Atlassian JIRA
(v6.2#6252)



More information about the asterisk-bugs mailing list