[Home]

Summary:ASTERISK-27216: app_queue: does its check-makeannouncement-logic twice each head-caller-loop
Reporter:Stefan Engström (StefanEng86)Labels:
Date Opened:2017-08-24 10:38:02Date Closed:2017-09-26 06:24:31
Priority:MinorRegression?
Status:Closed/CompleteComponents:Applications/app_queue
Versions:13.15.0 Frequency of
Occurrence
Related
Issues:
Environment:Attachments:
Description:I think the addition of the second check via the commit in
https://reviewboard.asterisk.org/r/2263/
is unnecessary,

By putting a check there, it allows for the possibility for the agent's phones to be ringing while announcement information is being played out to the head caller. Announcements should only be played between ringing cycles?

All right, let me provide a little more context

1. Create a queue with a ringtime of 20 seconds and periodic announcement audio message (of playlength ~5 sconds) to be played every 20 seconds
2. Create a single SIP device B to be the only queue member
3. Have person A call the queue
4. B should attempt to answer the call while the periodic announcement message is being played out to the header caller (the only caller) (A). This should happen around 21 seconds after A enters queue

The behavior I expect: B's sip device should stop ringing after ~20 seconds after A enters queue , and the whole periodic announcement should be played out to A before the second ringing cycle to B starts.
Actual behavior:
1) B sip device does ring as A is hearing announcement info, and
2) when B attempts to answer the queue call is not actually answered.

I'm not too concerned with 2) because 2) is a consequence of 1) and I think 1) is already unexpected behavior... It's even possible my particular configuration causes 2), but 1) is the issue I'm reporting. Should it really be possible for the head caller to hear "We will soon take your call, thank you for calling B...." then instantly get connected and hear "Hello you are talking to Doris"?

I'm not sure I follow your comment, what does *they* refer to in 'they are checks for two different types of announcements. '
There are two checks for two different types yes, but each of them are checked twice:
looking at app_queue.c

you have at line 8073 in queue_exec
{noformat}
               if (makeannouncement) {
                       /* Make a position announcement, if enabled */
                       if (qe.parent->announcefrequency)
                               if ((res = say_position(&qe,ringing)))
                                       goto stop;
               }
               makeannouncement = 1;

               /* Make a periodic announcement, if enabled */
               if (qe.parent->periodicannouncefrequency) {
                       if ((res = say_periodic_announcement(&qe,ringing))) {
                               goto stop;
                       }
               }
{noformat}
then at line 5247 a very similar code block starts
{noformat}
       /* Make a position announcement, if enabled */
       if (qe->parent->announcefrequency && qe->parent->announce_to_first_user) {
               say_position(qe, ringing);
       }

       /* Make a periodic announcement, if enabled */
       if (qe->parent->periodicannouncefrequency && qe->parent->announce_to_first_user) {
               say_periodic_announcement(qe, ringing);
       }
{noformat}
The queue_exec loop goes through both of these blocks each loop.
Comments:By: Asterisk Team (asteriskteam) 2017-08-24 10:38:04.003-0500

Thanks for creating a report! The issue has entered the triage process. That means the issue will wait in this status until a Bug Marshal has an opportunity to review the issue. Once the issue has been reviewed you will receive comments regarding the next steps towards resolution.

A good first step is for you to review the [Asterisk Issue Guidelines|https://wiki.asterisk.org/wiki/display/AST/Asterisk+Issue+Guidelines] if you haven't already. The guidelines detail what is expected from an Asterisk issue report.

Then, if you are submitting a patch, please review the [Patch Contribution Process|https://wiki.asterisk.org/wiki/display/AST/Patch+Contribution+Process].
By: Benjamin Keith Ford (bford) 2017-08-30 10:32:48.729-0500

Hey Stefan,
If you look at the queues.conf sample file and search "announce-frequency" and "periodic-announce", you can find these descriptions:
{quote}
; How often to announce queue position and/or estimated
; holdtime to caller (0=off)
; Note that this value is ignored if the caller's queue
; position has changed (see min-announce-frequency)
;
;announce-frequency = 90

;periodic-announce = queue-periodic-announce
;
; A set of periodic announcements can be defined by separating
; periodic announcements to reproduce by commas. For example:
;periodic-announce = queue-periodic-announce,your-call-is-important,please-wait
;
; The announcements will be played in the order in which they are defined. After
; playing the last announcement, the announcements begin again from the beginning.
{quote}
This seems to be inline with the source code, since they are checks for two different types of announcements. Is there a bug you've run into? If so, please post it along with the steps you took so we can reproduce it. Thanks!
By: Stefan Engström (StefanEng86) 2017-08-31 02:09:03.193-0500

All right, let me provide a little more context

1. Create a queue with a ringtime of 20 seconds and periodic announcement audio message (of playlength ~5 sconds) to be played every 20 seconds
2. Create a single SIP device B to be the only queue member
3. Have person A call the queue
4. B should attempt to answer the call while the periodic announcement message is being played out to the header caller (the only caller) (A). This should happen around 21 seconds after A enters queue

The behavior I expect: B's sip device should stop ringing after ~20 seconds after A enters queue , and the whole periodic announcement should be played out to A before the second ringing cycle to B starts.
Actual behavior:
1) B sip device does ring as A is hearing announcement info, and
2) when B attempts to answer the queue call is not actually answered.

I'm not too concerned with 2) because 2) is a consequence of 1) and I think 1) is already unexpected behavior... It's even possible my particular configuration causes 2), but 1) is the issue I'm reporting. Should it really be possible for the head caller to hear "We will soon take your call, thank you for calling B...." then instantly get connected and hear "Hello you are talking to Doris"?

I'm not sure I follow your comment, what does *they* refer to in 'they are checks for two different types of announcements. '
There are two checks for two different types yes, but each of them are checked twice:
looking at app_queue.c

you have at line 8073 in queue_exec
{noformat}
               if (makeannouncement) {
                       /* Make a position announcement, if enabled */
                       if (qe.parent->announcefrequency)
                               if ((res = say_position(&qe,ringing)))
                                       goto stop;
               }
               makeannouncement = 1;

               /* Make a periodic announcement, if enabled */
               if (qe.parent->periodicannouncefrequency) {
                       if ((res = say_periodic_announcement(&qe,ringing))) {
                               goto stop;
                       }
               }
{noformat}
then at line 5247 a very similar code block starts
{noformat}
       /* Make a position announcement, if enabled */
       if (qe->parent->announcefrequency && qe->parent->announce_to_first_user) {
               say_position(qe, ringing);
       }

       /* Make a periodic announcement, if enabled */
       if (qe->parent->periodicannouncefrequency && qe->parent->announce_to_first_user) {
               say_periodic_announcement(qe, ringing);
       }
{noformat}
The queue_exec loop goes through both of these blocks each loop.

By: Stefan Engström (StefanEng86) 2017-08-31 02:22:51.767-0500

The issue with say_position and say_periodic_announcement are very similar.

Also a key point I'm making (what I meant with 'uneccessary commit') is that reverting that old commit I link preserves the good behavior (the behaviour i expect asterisk to have had up til 2013) that all positioninfo and announcements to head callers are still played, but only between the ringing phases. But of course I invite everyone to review side effects of reverting that commit.

I noticed that the commit in question had a commit message saying something like "without this commit head caller will not hear announcements", which is not true anymore by my tests. (But maybe it was when the commit was made?)

By: Rusty Newton (rnewton) 2017-09-05 17:59:14.556-0500

Thanks, the additional context and information makes the issue more clear. You are welcome to submit a patch to Gerrit and see what comes of it with reviews. That is probably the fastest way to get the change made if you are invested in it.
By: Richard Mudgett (rmudgett) 2017-09-19 17:33:53.470-0500

Heh.  The patch for ASTERISK-21782 is now mostly dead code because it was added to stop the announcements that are being removed by this patch.
By: Stefan Engström (StefanEng86) 2017-09-21 06:21:45.546-0500

The patchset #3 of the gerrit has now been tested in production (or well an identical change to a asterisk 13.15 version); and no reported unexpected side effects yet, but there might be more of those as Richard found
By: Friendly Automation (friendly-automation) 2017-09-26 06:24:33.280-0500

Change 6506 merged by Jenkins2:
app_queue: Only do announcement logic between ringing cycles

[https://gerrit.asterisk.org/6506|https://gerrit.asterisk.org/6506]
By: Friendly Automation (friendly-automation) 2017-09-26 06:25:44.987-0500

Change 6580 merged by Jenkins2:
app_queue: Only do announcement logic between ringing cycles

[https://gerrit.asterisk.org/6580|https://gerrit.asterisk.org/6580]
By: Friendly Automation (friendly-automation) 2017-09-26 06:30:18.971-0500

Change 6581 merged by Jenkins2:
app_queue: Only do announcement logic between ringing cycles

[https://gerrit.asterisk.org/6581|https://gerrit.asterisk.org/6581]
By: Friendly Automation (friendly-automation) 2017-09-26 06:39:02.151-0500

Change 6582 merged by Joshua Colp:
app_queue: Only do announcement logic between ringing cycles

[https://gerrit.asterisk.org/6582|https://gerrit.asterisk.org/6582]
By: Friendly Automation (friendly-automation) 2017-10-04 06:35:20.959-0500

Change 6634 merged by Jenkins2:
app_queue.c: Fix announcements when announce-to-first-user not enabled.

[https://gerrit.asterisk.org/6634|https://gerrit.asterisk.org/6634]
By: Friendly Automation (friendly-automation) 2017-10-04 06:55:21.162-0500

Change 6635 merged by Joshua Colp:
app_queue.c: Fix announcements when announce-to-first-user not enabled.

[https://gerrit.asterisk.org/6635|https://gerrit.asterisk.org/6635]
By: Friendly Automation (friendly-automation) 2017-10-04 07:03:01.372-0500

Change 6636 merged by Jenkins2:
app_queue.c: Fix announcements when announce-to-first-user not enabled.

[https://gerrit.asterisk.org/6636|https://gerrit.asterisk.org/6636]
By: Friendly Automation (friendly-automation) 2017-10-04 07:09:43.662-0500

Change 6637 merged by Joshua Colp:
app_queue.c: Fix announcements when announce-to-first-user not enabled.

[https://gerrit.asterisk.org/6637|https://gerrit.asterisk.org/6637]