[asterisk-dev] [Code Review] Backport of state_interface for app_queue in Asterisk 1.4

[Mark Michelson]

> On 2009-01-29 11:38:42, atis wrote:
> > /branches/1.4/apps/app_queue.c, line 908
> > <http://reviewboard.digium.com/r/116/diff/1/?file=2283#file2283line908>
> >
> >     It seems that http://svn.digium.com/view/asterisk/trunk/apps/app_queue.c?r1=140489&r2=140975 has not been applied here. It will probably create deadlocks rarely
> >

I gave a look at that patch and I think it doesn't really apply to this backport. In trunk, the removal of locks of the queue container (since the ao2 interface will do most of the necessary locking for you) was what caused this deadlock to surface. Looking at 1.4's code, remove_from_interfaces is always called with either no locks held or with both the queue container lock and the queue's lock held. In trunk, the reason this patch was needed was because there were times where only the queue's lock was held and the queue container lock was not. This created an inconsistent lock order that was fixed through the patch you referenced.


> On 2009-01-29 11:38:42, atis wrote:
> > /branches/1.4/apps/app_queue.c, line 4437
> > <http://reviewboard.digium.com/r/116/diff/1/?file=2283#file2283line4437>
> >
> >     This comment is really not necessary anymore :)

I've removed this comment. Thanks :)


- Mark


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/116/#review308
-----------------------------------------------------------


On 2009-01-05 17:09:32, Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/116/
> -----------------------------------------------------------
> 
> (Updated 2009-01-05 17:09:32)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This is a backport of functionality that is already in all releases of Asterisk 1.6.
> 
> The functionality being added here is the ability to associate an interface with a queue member which is used to determine that member's device state.
> A common usage for such functionality is to accurately determine the device state of a queue member who is called via the Agent or Local channel 
> technologies. By specifying a more "concrete" interface to watch for device state, the queue application can more accurately decide the availability
> of the queue member.
> 
> The policy for Asterisk 1.4 is to not introduce new functionality, and only make bug fixes. This case is one where the line has been blurred significantly
> and so community feedback would be helpful. From a development standpoint, this violates the policy since most of what is being added is a new option for
> queue members. From many users' perspectives, however, this is a means of solving a deficiency in the basic operations of queues. Please take a look at the
> diff attached and comment on whether this would be problematic. It is implemented in such a way that any old dialplans, CLI commands, manager commands, etc.
> will not need to be updated unless you are making use of this new functionality.
> 
> In internal discussions, the only point that was brought up was a possible problem that could aris if using persistent members. If you upgraded to a version 
> that had the state interface functionality in it and then downgraded to a version which did not have state interface functionality, then the queue members' 
> names loaded from the database would be wrong (they would have a semicolon and the state interface appended). Since the members' names are only used for display 
> purposes, since the error may be easily corrected by reloading the queue configuration,and since the number of people likely to be affected by this is very 
> small, I feel it's a risk worth taking.
> 
> Another potential problem I have thought of is that Asterisk 1.4 does not contain func_devstate. This means that this solution will only work if
> you know for sure what endpoint will be dialed when a call is placed from a queue. To give an example, if you know that calling Local/2000 will always 
> end up dialing SIP/2000, then this functionality will work great for you. If calling Local/2000 may end up dialing SIP/2000 or potentially SIP/3000 based 
> on some dialplan logic, then this new functionality will not help you since you cannot use custom device states in the dialplan.
> 
> 
> Diffs
> -----
> 
>   /branches/1.4/apps/app_queue.c 166949 
> 
> Diff: http://reviewboard.digium.com/r/116/diff
> 
> 
> Testing
> -------
> 
> I used the scenario of issue 12970 as a basis for creating a test.
> 
> For the test, I created an Agent channel, Agent/1000, and had this line in the
> dialplan:
> 
> exten => 1006,1,AgentCallbackLogin(1000,,1000 at internal)
> 
> In the context "internal," extension 1000 is as follows:
> 
> exten => 1000,1,Dial(SIP/1000)
> 
> If it is not clear, All calls to Agent/1000 will eventually go to SIP/1000. I added
> a line to my queues.conf file for my queue:
> 
> member => Agent/1000,,,SIP/1000
> 
> When I place an outbound call from SIP/1000, issuing the "queue show" command shows that
> Agent/1000 is "in use." Attempting to place a call to the queue will not cause SIP/1000 to
> ring since I have ringinuse=no for my queue. When SIP/1000 hangs up, the status returns to 
> "not in use."
> 
> 
> As another test, I tried implementing a common problem. In my queues.conf file, I added this
> to my queue:
> 
> member => Local/1000,,,SIP/1000
> 
> As you may be able to guess, Local/1000 always dials SIP/1000. I placed a call to my queue
> and answered with SIP/1000. At this point, issuing a "queue show" command showed that the
> member Local/1000 was "in use." Then, from SIP/1000 I transferred the call to another endpoint.
> Issuing a "queue show" command then showed Local/1000 as being "not in use."
> 
> 
> Thanks,
> 
> Mark
> 
>