[Home]

Summary:ASTERISK-12053: [patch] Queue timeout terminates call attempt, causing partial ring
Reporter:Atis Lezdins (atis)Labels:
Date Opened:2008-05-20 11:25:32Date Closed:2008-08-18 15:14:19
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Applications/app_queue
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) 12690.diff
( 1) full_2.log
( 2) full.log
( 3) queue_timeoutpriority.patch
( 4) queue_timeouts.c
( 5) queue_timeouts.patch
( 6) ring_time_calc.patch
Description:If queue timeout occurs while queue is trying to ring member, call attempt is terminated, not executing any instructions further, and causing partial ring, which can sometimes be even one second.

After discussion with putnopvut on irc, we came to conclusion that this is bug.



****** ADDITIONAL INFORMATION ******

Queue 22902 has one member - Local/21168@default_queue

Sample dialplan here:

...
Queue(22902|t|||14)
...

context default_queue {
_X. => {
 Dial(SIP/${EXTEN}|15|gt);
 PauseQueueMember(|Local/${EXTEN}@default_queue);
}

Comments:By: Mark Michelson (mmichelson) 2008-05-21 09:33:29

I've uploaded 12690.diff. This effectively will prevent the behavior your seeing.

The only problem I have is that I'm very hesitant about just adding this into 1.4 since there could be people who depend on (somewhat) exact timeouts occurring in their queues. I think that instead, I will make this an option that can be set in queues.conf and add the feature to trunk and 1.6
By: Atis Lezdins (atis) 2008-05-21 11:07:07

Nope, still doesn't work :(
By: Mark Michelson (mmichelson) 2008-05-21 12:57:16

What do you have set for "timeout" in your queue definition in queues.conf (or in your realtime queue table) ?
By: Atis Lezdins (atis) 2008-05-22 07:00:49

For me timeout in queues config is 0, as i use timeout in Dial from local channel (otherwise it would be impossible to handle NO ANSWER there)

After looking to Your patch, i started debugging that myself, and came to conclusion that this is what I would want. However, I'm not really sure how it will affect timeoutrestart (purpose of which i don't understand)

Here's comparison of both timeout calculations:
<pre>
qe->expire | qe->expire - now | qe->parent->timeout | to (current) | to (proposed)
===========|==================|=====================|==============|===============
  T+20    |       20         |       10            |     10000    |   10000
  T+10    |       10         |       10            |     10000    |   10000
   T+5    |        5         |       10            |     5000     |   10000
    T     |        0         |       10            |       0      |     0
  T+20    |       20         |        0            |     20000    |    -1
  T+10    |       10         |        0            |     10000    |    -1
   T+5    |        5         |        0            |     5000     |    -1
    T     |        0         |        0            |       0      |     0
    0     |       -T         |       10            |     10000    |   10000
    0     |       -T         |        5            |     5000     |   5000
    0     |       -T         |        0            |      -1      |    -1
</pre>


By: Mark Michelson (mmichelson) 2008-05-23 11:57:50

Very nice chart! The problem I have with the proposed timeouts is all the cases where qe->expire is set to some time but the qe->parent->timeout is 0. In those cases, we would ring the member indefinitely and completely ignore the desired Queue() timeout value. I think a better approach would be to use qe->parent->timeout if it is defined. Otherwise, use qe->expire - now. Thus, the table would be edited like so:
<pre>
qe->expire | qe->expire - now | qe->parent->timeout | to (current) |      to (proposed)
===========|==================|=====================|==============|===============
  T+20    |       20         |       10            |     10000    |   10000
  T+10    |       10         |       10            |     10000    |   10000
   T+5    |        5         |       10            |      5000    |   10000
    T     |        0         |       10            |         0    |       0
  T+20    |       20         |        0            |     20000    |   20000
  T+10    |       10         |        0            |     10000    |   10000
   T+5    |        5         |        0            |      5000    |    5000
    T     |        0         |        0            |         0    |       0
    0     |       -T         |       10            |     10000    |   10000
    0     |       -T         |        5            |      5000    |    5000
    0     |       -T         |        0            |        -1    |      -1
</pre>

By the way, you asked about the timeout restart option. The purpose of this is so that the timer will be re-initialized if a second member is attempted during the calling time. So for instance, if you have the timeout in queues.conf set to 20, then when your turn comes, you have 20 seconds in which you will attempt to ring members. If the first member you try to reach waits 5 seconds and then presses the reject button on his phone, then you still have 15 seconds in which to attempt to reach a member before the timer expires. However, if you have timeoutrestart turned on, then the timer will be reset to 20 seconds after the first member rejects the call. timeoutrestart does nothing if the queue uses the "ringall" strategy.
By: Atis Lezdins (atis) 2008-05-23 12:42:44

Well, the thing with queues.conf timeout is, that if set to 0, it means infinite, right? In my case, i really want it infinite (as i set my own timeout in Dial app), and want some post-processing after that, even if Queue app timeout approaches end.

I think this should be as simple as "queue member ring timeout" should have higher priority than "queue wait timeout" or opposite.
If you could explain why would somebody want to have infinite ring timeout terminated by queue timeout?

Btw, very nice explanation of timeoutrestart. It should be documented better, as for now there's nothing really explaining :)
By: Mark Michelson (mmichelson) 2008-05-27 12:32:25

My thinking is that if someone truly wanted infinite ring time to a queue member, then they would set the timeout=0 in queues.conf and they would not specify a timeout parameter when calling Queue().

For your situation, it makes sense to do what you are doing since you specify a timeout to Dial for the queue member. The problem is that we can't make those sorts of assumptions in app_queue since there are lots of people not using Local channels for their queue members. For them, if they want to ring a single queue member for a specified time and then exit the queue if that time expires, they have two potential ways of doing it.

1. Set the timeout in queues.conf to be a certain time and then pass the 'n' option to Queue().

2. Set the timeout in queues.conf to 0 and then pass a timeout parameter to Queue().

The difference in the two is that you can vary the timeout if you use method 2. You can't do that if you use method 1.



What I would suggest is that the behavior of timeouts is already a bit ambiguous and people may have different intentions by setting the various timeouts different ways, we create an option in queues.conf which will tell app_queue if it should take the timeout passed to Queue() into consideration when calculating the amount of time to ring a member. The option could be something like

allowpartialring = yes|no

Yes is the default, which will behave the way app_queue does right now. If set to no, then it would cause the timeout specified in queues.conf to always be used when calculating the amount of time to ring a member's phone. The only way the timeout passed to Queue() would be used in this situation is between attempts to ring members.

In the situation you reported initially, if you set allowpartialring=no, then app_queue would think that it should ring the queue member for an infinite period of time (assuming you have timeout=0 in queues.conf). This way, the 14 second timeout will not be taken into account when dialing the queue member. Instead, the 15 second timeout provided to Dial() will instead take effect.
By: Mark Michelson (mmichelson) 2008-06-26 17:31:41

atis: I have come up with a patch to close this issue, but I recall that you had a better suggestion for the name of the option in #asterisk-dev. I can't remember the name of the option you suggested, though.
By: Atis Lezdins (atis) 2008-06-27 14:54:46

Great news. One of things i wanted to finalize, but didn't had time..

I was suggesting that parameter would describe that it affects timeout priority.

Something like this:
timeoutpriority=queue|arg
timeoutpriorityarg=yes|no
timeoutpriorityqueueconf=yes|no
By: Digium Subversion (svnbot) 2008-07-03 09:27:11

Repository: asterisk
Revision: 127720

U   trunk/CHANGES
U   trunk/apps/app_queue.c
U   trunk/configs/queues.conf.sample

------------------------------------------------------------------------
r127720 | mmichelson | 2008-07-03 09:27:04 -0500 (Thu, 03 Jul 2008) | 7 lines

Added a new option, "timeoutpriority" to queues.conf. A detailed
explanation of the change may be found in configs/queues.conf.sample

(closes issue ASTERISK-12053)
Reported by: atis


------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=127720
By: Atis Lezdins (atis) 2008-07-18 13:31:30

It still doesn't give infinite ring timeout if config timeout is 0.

I'll post a patch shortly, which will calculate values in proposal2

<pre>
qe->expire |  qe->parent->timeout | timeoutpriority | to (old) | to (new) | to (proposal2)
------------|---------------------|-----------------|----------|----------|----------------
    T+20   |          10         |       CONF      |  10000   |  10000   |       10000
    T+10   |          10         |       CONF      |  10000   |  10000   |       10000
     T+5   |          10         |       CONF      |   5000   |  10000   |       10000
       T   |          10         |       CONF      |      0   |  10000   |           0
    T+20   |           0         |       CONF      |  20000   |  20000   |          -1
    T+10   |           0         |       CONF      |  10000   |  10000   |          -1
     T+5   |           0         |       CONF      |   5000   |   5000   |          -1
       T   |           0         |       CONF      |      0   |      0   |           0
       0   |          10         |       CONF      |  10000   |  10000   |       10000
       0   |           5         |       CONF      |   5000   |   5000   |        5000
       0   |           0         |       CONF      |     -1   |     -1   |          -1
    T+20   |          10         |        APP      |  10000   |  10000   |       10000
    T+10   |          10         |        APP      |  10000   |  10000   |       10000
     T+5   |          10         |        APP      |   5000   |   5000   |        5000
       T   |          10         |        APP      |      0   |      0   |           0
    T+20   |           0         |        APP      |  20000   |  20000   |       20000
    T+10   |           0         |        APP      |  10000   |  10000   |       10000
     T+5   |           0         |        APP      |   5000   |   5000   |        5000
       T   |           0         |        APP      |      0   |      0   |           0
       0   |          10         |        APP      |  10000   |  10000   |       10000
       0   |           5         |        APP      |   5000   |   5000   |        5000
       0   |           0         |        APP      |     -1   |     -1   |          -1
</pre>


By: Atis Lezdins (atis) 2008-07-18 14:51:00

Ok, the last patch updates code and documentation.
By: Atis Lezdins (atis) 2008-08-14 11:19:56

Last patch includes correct timing out when timeoutpriority=CONF
By: Digium Subversion (svnbot) 2008-08-18 15:14:17

Repository: asterisk
Revision: 138694

U   trunk/apps/app_queue.c
U   trunk/configs/queues.conf.sample

------------------------------------------------------------------------
r138694 | mmichelson | 2008-08-18 15:14:17 -0500 (Mon, 18 Aug 2008) | 10 lines

Change the queue timeout priority logic into less ugly
and confusing code pieces. Clarify the logic within
queues.conf.sample.

(closes issue ASTERISK-12053)
Reported by: atis
Patches:
     queue_timeoutpriority.patch uploaded by atis (license 242)


------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=138694