Summary:ASTERISK-10408: [patch] preventing parallel logins from the same line or by the same agent
Reporter:Alexandre Snarskii (snar)Labels:
Date Opened:2007-09-28 11:44:30Date Closed:2011-06-07 14:02:43
Versions:Frequency of
Environment:Attachments:( 0) app_queue-parallel.patch
( 1) app-queue-parallel-rev2.patch
Description:This feature allows asterisk to deal with next scenarios:

Assume, agent A logs in (with AddQueueMember) from line SIP/NNN to queue queueA.
Then his/her shift ends, and he/she leaves call-center without logging off.
On the next day, agent B logs in from the same SIP/NNN line to queueB,
but he start receiving calls to queueA too, because asterisk supposes
that's there is still agent A (subscribed to queueA) on this line.

Another scenario: Assume, agent A logged in from SIP/NNA.
Then, does not matter why, he decides to log on from SIP/NNB.
Current queue logic does not notices that, so, calls from queue gets
ringing both SIP/NNA and SIP/NNB, and when strategy is not ringall one
this can introduce unnecessary delays in call processing.

Well, both scenarios can be dealt with enforcing local policy, but
in our non-ideal world with non-ideal agents I suppose that adding some
smart behaviour to asterisk is just better idea..


Well, this patch is not ideal.

a) It relies on queue member MemberName to distinct agents,
and if membername not set - all checks skipped, because there are
no other distinction.
b) It can't 'logoff' agents whose status is not 'not in use' or
'Unknown'. While testing patch, I found that at least removing
from queue member in state Ringing leads to call to be dropped..
Comments:By: Paul W (kwakwaversal) 2007-09-28 17:50:03

Maybe it's just me but if agents are logging in with AddQueueMember I assume something is manipulating the AMI interface to add them to a queue?  Some kind of script or daemon?  There are a number of ways to find out if an interface is logged into a queue but wouldn't it be just as easy to run `RemoveQueueMember` for each queue before you AddQueueMember to make sure that it's logged out of every queue?

I'm probably being biased towards using AMI to handle dynamic member subscriptions, this may be a very useful feature.  I just can't help but think there are a million and one different scenario's that might crop up with regards to dynamic members so there may need to be a different approach?

By: Alexandre Snarskii (snar) 2007-09-30 10:24:59

No, AMI is not necessarily used with AddQueueMember. In my context
AddQueueMember is just a dialplan application which should be used
to add dynamic member to queue from dialplan in abscence of AgentCallbackLogin
in 1.6. (example: doc/queues-with-callback-members.txt in 1.4 sources).

Well, there is also AMI action 'QueueAdd', which is mostly identical
to AddQueueMember, and which is also handled by proposed patch.

And, of course, there is also a variant of writing separate daemon,
which will handle QueueMemberAdded/QueueMemberRemoved
events and preventing any kind of parallel logins using AMI, but:
- writing/debugging such a daemon is a non-trivial task, much harder than
provide 100-line patch to asterisk. Remember, that daemon must be non-blocking
and run at least on Linux/FreeBSD/Solaris.
- system administrators all over the world will have to install
additional software to get those little features to work (and, for
starters, they should know that they need to install that daemon).
- this daemon (at least for the first time) will be application-oriented.
I know functions and requirements of my call-center, you know functions
and requirements of yours - but, at least I'm not sure that I will ever
know functions and requirements of "call-center in general".
- and, for the final - there are possibilities of 'daemon conflicts',
when, assume, your daemon issues QueueAdd, my daemon thinks that in
result (QueueMemberAdded) we got policy violation, issues QueueRemove,
your daemon see QueueMemberRemoved, but, as he wants agent to be logged,
issues QueueAdd and so on and on...

By: Eliel Sardanons (eliel) 2007-10-04 16:58:07

I don't know if this patch should be commited but:
                    You should check members interface in a case insensitive manner, use strcasecmp instead of strcmp because sip/eliel is the same as SIP/eliel.

By: Mark Michelson (mmichelson) 2007-10-05 11:44:11

I took a long look at this, wondering if this was really something that will be needed by people using queues, and my conclusion is that it really couldn't hurt to have it.

I haven't reviewed the patch yet though, so I'll give an update after I've looked it over.

By: Mark Michelson (mmichelson) 2007-10-05 12:19:32

I just took a look at the patch, and it made me feel a lot better about this option. I had misunderstood the concept at first, but after looking through the patch, it really does seem to make the queue application "smarter" in detecting possible problems.

That being said, I can think of several things that could make this patch better.

First and foremost, there is some unnecessary code duplication. For instance, a call to remove_from_queue could be used instead of manually doing the same thing in the check_queue_members function. Also, the calls to check_queue_members could be moved to add_to_queue, so that that check is done all in one place as well.

Second, at the beginning of check_queue_members, instead of checking for the possibility of the strings being NULL, it's better to use the ast_strlen_zero function, which checks both for a NULL string, or a zero-length string. Also, it's not necessary to check if interface is NULL, since that is checked previously in all cases.

Third, because you don't have to worry about interface being NULL, you can save some computation by using ao2_find to find a specific member. Since astobj2 uses a hash table for storing members, it is quicker to do a find than to iterate through the whole table. If you search for ao2_find in app_queue, you can see how to use it.

There are a couple of nitpicks, too, but I feel I should point them out anyway. First, it's not necessary to put semicolons after curly braces. Second, regarding your additions to the queues.conf.sample file, I would recommend replacing all instances of the word "agent" with "member" to prevent confusion, since conceivably, this does not have to be used with only agent channels.

Thanks for the patch, and I look forward to seeing an update.

By: Alexandre Snarskii (snar) 2007-10-05 14:09:30

Concerning your note on remove_from_queue: not duplicating code is really
good idea, but remove_from_queue locks queue from which interface is being removed. And, as this queue is always locked in check_queue_members, deadlock condition occurs :( Or have I misunderstood something ?
What do you think I should do better: add additional parameter meaning
'do not lock queue while remove interface' to remove_queue_member, or leave
that part intact ?

Note on 'misplacement' of calls to check_queue_members: Intended behaviour
of those options is that last logged in member is always 'winner'. If I move
call to add_queue_member - that will add this call to 'reload persistent
members from database' cycle, and there is a possibility that last logged
in becomes loser and some phantom located in database after him wins.
(Well, that can happen only when one of options changed in runtime, but,
anyway, better safe than sorry..)

Note on !=NULL vs. ast_strlen_zero: fixed.

Note on ao2_find: fixed. However, to prevent the same member to be logged from
different interfaces (when denyparallellogin set to on), I still need to
iterate queue members.

Note on queues.conf: agents renamed to members, option onlyagentperline
renamed to onlymemberperline

Patch is to be uploaded...

PS: also, eliel note on interface case insensitiveness fixed,
added logging to queue_log and placement of calls to dump_queue_members()

By: David Brillert (aragon) 2007-11-09 23:21:52.000-0600

snar is your patch working for you?
In production?
No segfaults etc...?

By: Joel Vandal (jvandal) 2007-11-12 09:25:54.000-0600

IMHO this can be done in dialplan directly on your Login/Logout context.

I use AstDB key to check if an agent is already login and save login status (which queueues, etc). With Dialplan you can customize destination (ex. Hey, you are already logged from 1234, press 1 to logoff previous agents, 2 to hangup, etc).

By: Jason Parker (jparker) 2008-04-02 12:46:12

I pretty much agree with jvandal here.  It would be fairly easy to do from dialplan.


By: Mark Michelson (mmichelson) 2008-04-02 17:42:03

In response to the last few notes here as well as discussions I've had, this functionality can be taken care of in the dialplan, and so I am going to close this issue.

I'm sorry that this issue remained open for so long only to be closed without the patch being merged, and I hope this doesn't discourage you from contributing to Asterisk in the future.