[Home]

Summary:ASTERISK-11857: join empty categories are not the same as leavewhenempty ones, joinempty probably wrong
Reporter:David Woolley (davidw)Labels:
Date Opened:2008-04-15 11:14:39Date Closed:2011-05-05 08:22:13
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Applications/app_queue
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:
Description:Once one accounts for joinempty and leavewhenempty sort of having opposite logical senses, one cannot match the effects of the values of these two variables one for one.

joinempty = no matches leavewhenempty = yes

joinempty = yes matches leavewhenempty = no

However, joinempty = strict drops only for QUEUE_NO_REACHABLE_MEMBERS, but

leavewhenempty = strict drops for both QUEUE_NO_MEMBERS and QUEUE_NO_REACHABLE_MEMBERS.


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

This is based on code reading, but arose out of an observation that queue entries were accepted for all agents logged out but not for all agents paused.  That immediate problem is probably going to be fixed by setting joinempty = yes, and it was encountered by a colleague on an older version of 1.4, so I may not follow that one through any futher (we didn't want entries to be dropped just for a pause).

I tend to feel that the leavewhenempty behaviour is more likely to be what was intended, but it is not 100% clear what was intended.
Comments:By: Tilghman Lesher (tilghman) 2008-04-15 21:23:26

We're not really concerned about 'in theory, maybe wrong', we're more concerned with actual behavior.  Is there unintended or buggy behavior in the queue settings?  If so, what specifically is the problem?
By: David Woolley (davidw) 2008-04-16 02:18:15

The key point is that they behave differently.  That violates the principle of least astonishment, and makes it difficult to learn what the options actually mean.

As a more concrete example.  If you have joinempty = strict and all the memebers are paused, the call will still be queued, but if you have leavewhenempty = strict and all the members are paused, the call will drop out of the queue..  Both will reject calls when all agents are not logged in (as long as they aren't also paused! - and assuming that all members are agents.)

(A colleague says that 1.4.16 can have a member paused with the agent logged out, but I haven't re-verified this for the current SVN version.)

If it is really intended that join and leave not have equivalent capabilities, the very least that is needed is to document the intended usages.

The behaviour that I think we would really like is both joining to fail and leaving to be forced if there were no agents logged in, but not if all the agents were paused (and logged in).  Given that we are not (yet) using penalties and we don't intend to have members in "invalid" states,, the current join = strict behaviour might actually be a step in the right direction (the precedence of paused over logged out is still a problem, unless there has been a recent change that forcibly resets paused on a logout, and the fact that one logged out agent with all the other paused will cause an overall status of unavailable, is also a problem).  However we wouldn't have a corresponding leave option that dumped the queue when the last agent logged out, which wouldn't also dump it when all agents were paused.

Fully characterising these options does require that one accounts invalid members and members that are blocked by penalty.


By: David Woolley (davidw) 2008-04-16 04:26:27

What specifically makes me feel that the join implemenation of strict is wrong is that, if you have a mixture of paused and unavailable members, the unavailable member takes precedence.

(But note, that whilst I think that the leave behaviour is more likely what was intended for both, its not the best behaviour for us, as we would like pause not to dump entries from the queue, but logout to dump them.)


By: David Woolley (davidw) 2008-05-20 11:45:25

I've had a look at queues.conf again and the documentation for joinempty disagrees with both my reading of the code and the code seems to actually work the way I read it.

I have just done a real test (1.4.20-rc3), to confirm this.  Pausing the (last) agent, which causes the join to fail with the default joinempty=no, doesn't compromise a join when strict is used, but strict does cause join to fail if the agent is logged out.  Given that a paused agent is treated like one that is not configured at all, the documentation would imply that both these conditions would cause the join to fail.

I've also just tried this with there really being no members in the queue, and one can still join the queue with strict, which is definitely in violation of the queues.conf documentation.

Note that our application uses pause in normal operation, so the current de facto behaviour of joinempty is more useful.  Our problem is that one cannot configure a leavewhenempty that doesn't risk calls be dropped prematurely because all agents are paused.  Also note that I got our best compromise wrong in the additional information; I should have said "strict" rather than "yes".

Either the code or the documentation is definitely wrong.
By: Digium Subversion (svnbot) 2008-05-30 16:11:24

Repository: asterisk
Revision: 119404

U   branches/1.4/apps/app_queue.c

------------------------------------------------------------------------
r119404 | tilghman | 2008-05-30 16:11:19 -0500 (Fri, 30 May 2008) | 6 lines

When joinempty=strict, it only failed on join if there were busy members.  If
all members were logged out OR paused, then it (incorrectly) let callers join
the queue.
(closes issue ASTERISK-11857)
Reported by: davidw

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

http://svn.digium.com/view/asterisk?view=rev&revision=119404
By: Digium Subversion (svnbot) 2008-05-30 16:16:51

Repository: asterisk
Revision: 119419

_U  trunk/
U   trunk/apps/app_queue.c

------------------------------------------------------------------------
r119419 | tilghman | 2008-05-30 16:16:49 -0500 (Fri, 30 May 2008) | 14 lines

Merged revisions 119404 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.4

........
r119404 | tilghman | 2008-05-30 16:17:45 -0500 (Fri, 30 May 2008) | 6 lines

When joinempty=strict, it only failed on join if there were busy members.  If
all members were logged out OR paused, then it (incorrectly) let callers join
the queue.
(closes issue ASTERISK-11857)
Reported by: davidw

........

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

http://svn.digium.com/view/asterisk?view=rev&revision=119419
By: Digium Subversion (svnbot) 2008-05-30 16:17:37

Repository: asterisk
Revision: 119420

_U  branches/1.6.0/
U   branches/1.6.0/apps/app_queue.c

------------------------------------------------------------------------
r119420 | tilghman | 2008-05-30 16:17:35 -0500 (Fri, 30 May 2008) | 22 lines

Merged revisions 119419 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

................
r119419 | tilghman | 2008-05-30 16:23:14 -0500 (Fri, 30 May 2008) | 14 lines

Merged revisions 119404 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.4

........
r119404 | tilghman | 2008-05-30 16:17:45 -0500 (Fri, 30 May 2008) | 6 lines

When joinempty=strict, it only failed on join if there were busy members.  If
all members were logged out OR paused, then it (incorrectly) let callers join
the queue.
(closes issue ASTERISK-11857)
Reported by: davidw

........

................

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

http://svn.digium.com/view/asterisk?view=rev&revision=119420
By: David Woolley (davidw) 2008-05-30 16:24:26

The change is, I believe, the change you intended, but the change history is wrong.  Firstly, paused is treated like what you call busy (but is actually none configured), not like logged out.  Secondly, the original behaviour was to only fail to join if *all* members were *logged out*.


By: Tilghman Lesher (tilghman) 2008-05-30 17:00:18

No, if you look at the code, paused is, in fact, treated like logged out.  And secondly, the original behavior was to fail to join if all members were logged out AND joinempty=no.  joinempty=strict followed a slightly different scheme.


By: David Woolley (davidw) 2008-05-30 17:27:01

We explicitly tested this with the old code and it did not cause the queuing to be rejected.  In fact, this whole issue first arose because one of our people tried to do an unnecessary workaround for queuing being rejected on join when joinempty was default and all queues were paused.

It's 23:26 here, so I'm not going to trace the code now to make a detailed case, but I seem to remember that paused members are treated as though they didn't exist  at all, rather than that they existed but were unavailable.

Actually, busy is wrong as well.  Busy members are OK.  The choice is between not existent, which includes paused, and unavailable, which includes logged out.  I will fix my original re-opening comment.
By: Digium Subversion (svnbot) 2008-05-30 18:32:38

Repository: asterisk
Revision: 119462

U   team/seanbright/resolve-shadow-warnings/apps/app_queue.c
U   team/seanbright/resolve-shadow-warnings/main/utils.c

------------------------------------------------------------------------
r119462 | seanbright | 2008-05-30 18:32:37 -0500 (Fri, 30 May 2008) | 28 lines

Merged revisions 119419,119423 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

................
r119419 | tilghman | 2008-05-30 17:23:14 -0400 (Fri, 30 May 2008) | 14 lines

Merged revisions 119404 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.4

........
r119404 | tilghman | 2008-05-30 16:17:45 -0500 (Fri, 30 May 2008) | 6 lines

When joinempty=strict, it only failed on join if there were busy members.  If
all members were logged out OR paused, then it (incorrectly) let callers join
the queue.
(closes issue ASTERISK-11857)
Reported by: davidw

........

................
r119423 | russell | 2008-05-30 17:51:17 -0400 (Fri, 30 May 2008) | 3 lines

Fix a minor merge issue that caused a function to not get compiled in with
DEBUG_THREADS like it was supposed to

................

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

http://svn.digium.com/view/asterisk?view=rev&revision=119462
By: Digium Subversion (svnbot) 2008-05-30 19:25:02

Repository: asterisk
Revision: 119466

_U  team/seanbright/resolve-shadow-warnings/

------------------------------------------------------------------------
r119466 | seanbright | 2008-05-30 19:25:00 -0500 (Fri, 30 May 2008) | 28 lines

Merged revisions 119419,119423 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

................
r119419 | tilghman | 2008-05-30 17:23:14 -0400 (Fri, 30 May 2008) | 14 lines

Merged revisions 119404 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.4

........
r119404 | tilghman | 2008-05-30 16:17:45 -0500 (Fri, 30 May 2008) | 6 lines

When joinempty=strict, it only failed on join if there were busy members.  If
all members were logged out OR paused, then it (incorrectly) let callers join
the queue.
(closes issue ASTERISK-11857)
Reported by: davidw

........

................
r119423 | russell | 2008-05-30 17:51:17 -0400 (Fri, 30 May 2008) | 3 lines

Fix a minor merge issue that caused a function to not get compiled in with
DEBUG_THREADS like it was supposed to

................

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

http://svn.digium.com/view/asterisk?view=rev&revision=119466
By: Sean Bright (seanbright) 2008-05-30 19:26:42

(This keeps getting reassigned to me and closed when I merge a developer branch, sorry for the mantis spam.)
By: David Woolley (davidw) 2008-06-02 08:30:10

Using 1.4.20.1, for reference, the categorisation of the queue member states is done by get_member_status, which was not modified by the current patch.

This starts by assuming QUEUE_NO_MEMBERS.

It then scans the members, skipping any that are paused, or fail the max_penalty condition. It also ignores any that are in an INVALID state, although the test is done slightly differently.

If it finds an entry which is UNAVAILABLE, it changes its assumption to QUEUE_NO_REACHABLE_MEMBERS.  UNAVAILABLE is how a logged out agent presents.

If it finds a member in any other state, including busy, it returns immediately with a result of QUEUE_NORMAL.  Note that doing anything else for busy would mean that you never built up a queue!

If it runs out of members, it returns the current assumed state, which is NO_MEMBERS unless it found an UNAVAILABLE entry.  Substituting logged out for unavailable (although there are other causes of unavailable).  Finding a logged out member will result in a NO_REACHABLE_MEMBERS result, but just finding paused memebers will be treated as though there were no members at all and result in a NO_MEMBERS result.  The presence of UNAVAILABLE members doesn't change the fact that the paused ones are ignored, but the NO_REACHABLE_MEMBERS state will then take precedence.

There is a slight anomaly if paused is combined with logged out, as paused causes the member to be skipped and the logged out state is never detected.  Thus, if all members are paused and logged out, you will get NO_MEMBERS, which is enough to trigger the joinempty=no condition.

Busy is always an accept condition, because you would never build a queue if you rejected just because the most available state was busy.
By: Tilghman Lesher (tilghman) 2008-06-02 10:54:17

I don't necessarily disagree with your analysis, but what specifically is wrong with the way that it works now in SVN (NOT 1.4.20.1, that is now older)?
By: David Woolley (davidw) 2008-06-02 12:09:36

The problem is the change history, not the change.  Someone reading the change log is going to get confused, as that is the only real documentation of the code.

(I don't like that paused can hide logged out, but I'm not sure that I can argue that that is not what was intended.  I think the whole thing needs much finer grained control, but that would be a feature request and off charter here.)
By: Tilghman Lesher (tilghman) 2008-06-02 12:29:55

Since there is nothing wrong with the code, I am closing this out as fixed.