[asterisk-bugs] [JIRA] (ASTERISK-28952) Queue wrapuptime sometimes not respected (based on stale lastcall time)
Walter Doekes (JIRA)
noreply at issues.asterisk.org
Mon Jun 15 06:22:25 CDT 2020
Walter Doekes created ASTERISK-28952:
----------------------------------------
Summary: 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
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