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:30	Date Closed:	2011-06-07 14:02:43
Priority:	Major	Regression?	No
Status:	Closed/Complete	Components:	Applications/NewFeature
Versions:		Frequency of Occurrence
Related Issues:
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.. ****** ADDITIONAL INFORMATION ****** 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() optimized.. 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. Thoughts? 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.