[asterisk-dev] app_queue: wrapuptime is intermittently disregarded

[Matthew Jordan]

On Wed, Dec 16, 2015 at 3:18 AM, Tomec Martin <tomec at ipex.cz> wrote:

> Hi all,
>
>
>
> I am solving issue ASTERISK-19820 and want to write patch, but I am not
> sure if I am on good way. Let me explain the issue:
>
> When agent in queue hangs up, his status is updated immediately, but the
> queue member lastcall time is updated after a while (after some logging
> etc.) in function update_queue. So when another caller wants to ring this
> agent, it gets new member status (not_inuse) and old queue member data
> (with old lastcall time). So wrapuptime is compared to wrong (old) lastcall
> time.
>
>
>
> My supposed solution is:
>
> Add new variable „is_in_call“ to struct member
>
> Set „is_in_call“ to true, when the call starts.
>
> Set „is_in_call“ to false on hangup callback (in function update_queue).
>
> Together with wrapuptime check also if „is_in_call“ is set to true.
>
>
>
> I don’t know how to determine call start and I am not sure if the variable
> should be named „is_in_call“. Any opinions are welcome.
>
>
>
> Thanks,
>
> Martin Tomec
>
>
>
Hi Martin:

Most of my thoughts on this come from Asterisk 13. Looking at the issue, it
appears as if the situation hasn't changed between when that issue was
filed and now, so hopefully this line of thinking is applicable to whatever
version you're looking at.

I can definitely see how this could be a bit racey. We essentially have two
things competing here:
 * The updating of the queue member status in update_status. This is,
interestingly enough, updated via a device state change or extension state
change. Even all the way back in 1.8 - and earlier - this would be handled
by some kind of asynchronous message mechanism, which would certainly be
happening on a different thread of execution than the channel threads
sitting in the queue.
 * The update of the 'queue' - which is really updating the member's state
in the queue, followed by updating various statistics of the queue -
happens in 13 when we detect one of three things happening:
  (1) A blind transfer away from the member
  (2) An attended transfer away from the member
  (3) A hangup of the member
  In all of these cases, these again are handled asynchronously off the
message bus. That wouldn't be the case in 11 and earlier - in those
versions, we'd be looking directly at either a masquerade occurring or soft
hangup flags on the member channel. Either way, it's certainly out of band
with the calls to update_status - and it is the same again in 13.

I've tried to think of another solution other than what you're proposing.
Essentially, we'd ideally like to rely on status to know if the member is
on a call or not; however, given the race condition occurring between the
changing of the member state (which should tell us that) and the "clean up
of the member in the queue" logic that is called via update_queue, I don't
think that's quite possible.

I don't think we can also easily combine update_queue with update_status.
update_status is called whenever a device/extension state changes, and
applies in situations other than just termination of a call. update_queue,
on the other hand, only happens on call end, and has certain logic that it
needs.

So: adding something to track the boolean state of whether or not a member
is on a call or not is probably the best approach.

As a cautionary point, I would be careful to test this logic here:

static int is_member_available(struct call_queue *q, struct member *mem)
{
    int available = 0;

    switch (mem->status) {
        case AST_DEVICE_INVALID:
        case AST_DEVICE_UNAVAILABLE:
            break;
        case AST_DEVICE_INUSE:
        case AST_DEVICE_BUSY:
        case AST_DEVICE_RINGING:
        case AST_DEVICE_RINGINUSE:
        case AST_DEVICE_ONHOLD:
            if (!mem->ringinuse) {
                break;
            }
            /* else fall through */
        case AST_DEVICE_NOT_INUSE:
        case AST_DEVICE_UNKNOWN:
            if (!mem->paused) {
                available = 1;
            }
            break;
    }

    /* Let wrapuptimes override device state availability */
    if (mem->lastcall && q->wrapuptime && (time(NULL) - q->wrapuptime <
mem->lastcall)) {
        available = 0;
    }
    return available;
}

As it would be prone to the race condition you're describing.

-- 
Matthew Jordan
Digium, Inc. | Director of Technology
445 Jan Davis Drive NW - Huntsville, AL 35806 - USA
Check us out at: http://digium.com & http://asterisk.org
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20151217/1edf1428/attachment.html>