[asterisk-dev] [Code Review] 2657: Replace chan_agent with app_agent_pool.

Fri Jul 5 15:51:29 CDT 2013

> On July 4, 2013, 4:27 p.m., Matt Jordan wrote:
> > /trunk/apps/app_agent_pool.c, lines 78-83
> > <https://reviewboard.asterisk.org/r/2657/diff/1/?file=41120#file41120line78>
> >
> >     I can see needing two other statuses here:
> >     
> >     * LOGGED_OUT - agent was logged out of the pool
> >     * HUNGUP - agent hung up
> >     
> >     More on that when the login app exits.
> >     
> >

Potential local channel optimization on the agent channel prevents meaningful information once the agent is logged in.  The optimized local channel would return here while the agent was still logged in.  The HUNGUP value is potentially useful in hangup handlers, but then this should be obvious since you are in a hangup handler.

> On July 4, 2013, 4:27 p.m., Matt Jordan wrote:
> > /trunk/apps/app_agent_pool.c, lines 446-447
> > <https://reviewboard.asterisk.org/r/2657/diff/1/?file=41120#file41120line446>
> >
> >     Just curious, why a RB tree?

Mainly a sorted container is needed to deal with duplicates.  I could have used a hash container with sorted buckets here instead.  I made this container the same as the active agents container for consistency.  The active agents container is also a rbtree.  The agents container is better sorted because the CLI "agent show all" can thus output a sorted agent list.

> On July 4, 2013, 4:27 p.m., Matt Jordan wrote:
> > /trunk/apps/app_agent_pool.c, lines 613-615
> > <https://reviewboard.asterisk.org/r/2657/diff/1/?file=41120#file41120line613>
> >
> >     In this function you note that the locking order is logged in channel, then agent. Go ahead and promote that as a doxygen \note on this function declaration - that's useful information for people looking at how to log an agent.

Added the note.  Not sure how useful the note will be.

> On July 4, 2013, 4:27 p.m., Matt Jordan wrote:
> > /trunk/apps/app_agent_pool.c, lines 1422-1425
> > <https://reviewboard.asterisk.org/r/2657/diff/1/?file=41120#file41120line1422>
> >
> >     This kind of off nominal path could probably use an ERROR message (or at least a debug so we can figure out why none of the agents are working)

Added a debug message here.  If this happens, the user has done this to us by destroying the holding bridge.  The only thing we can do is to log out the agent.  When the agent logs back in the holding bridge is recreated.

> On July 4, 2013, 4:27 p.m., Matt Jordan wrote:
> > /trunk/apps/app_agent_pool.c, lines 1426-1436
> > <https://reviewboard.asterisk.org/r/2657/diff/1/?file=41120#file41120line1426>
> >
> >     It's a subtle point that the ast_bridge_join returns when the agent either:
> >     
> >     (a) Leaves the holding bridge because they've hung up or otherwise moved on
> >     
> >     (b) Was removed from the holding bridge, did a call with a caller, and the bridging system returned through the stack of both bridges to this point
> >     
> >     A comment or something that describes this somewhere might be appropriate.

Added some more comments when the loop is broken.

(b) This statement is true in general but wrong in details.  The channel is moved from the holding bridge to a caller's bridge for the call.  It does not return through both bridges since it is only in one bridge at a time.

> On July 4, 2013, 4:27 p.m., Matt Jordan wrote:
> > /trunk/apps/app_agent_pool.c, lines 1428-1429
> > <https://reviewboard.asterisk.org/r/2657/diff/1/?file=41120#file41120line1428>
> >
> >     You could probably just bail out here immediately, moving your logic from of the end of the function up to here.

I originally had that code here, but I moved it to the end because it could be true even if the loop broke from some other condition.

> On July 4, 2013, 4:27 p.m., Matt Jordan wrote:
> > /trunk/apps/app_agent_pool.c, lines 1767-1776
> > <https://reviewboard.asterisk.org/r/2657/diff/1/?file=41120#file41120line1767>
> >
> >     Should this failure path reset the agent->devstate as well? caller_abort_agent doesn't appear to do this and probably should.

No, this is the caller thread.  If the agent logged out the status reflecting logout is updated by the agent channel thread.

> On July 4, 2013, 4:27 p.m., Matt Jordan wrote:
> > /trunk/apps/app_agent_pool.c, lines 1793-1797
> > <https://reviewboard.asterisk.org/r/2657/diff/1/?file=41120#file41120line1793>
> >
> >     Should this hang up the calling channel on exit?
> >     
> >     I would think we'd return 0 here and let the dialplan decide what to do.

Local channel optimization on the caller channel prevents meaningful information once the caller joins the bridging system and the agent connects.  The normal use case is going to be using a local channel with AgentRequest().

There are a few cases where it would be nice to continue in the dialplan here with the AGENT_STATUS value set appropriately if the caller fails to connect with the agent.  They are just very hard to guarantee to be accurate.
* The safety timeout aborts the caller being stuck in the bridge if the agent is stuck for some reason.  This could be done by caller_safety_timeout().  Unfortunately there are locking and race conditions to deal with.
* The agent fails to ack the call in time and gets logged out.  You have to "reach across the bridge" to set the channel variable.  A bridge action to set the channel variable could be done here, but the pitfall is if the channel is not in the bridge at the time.
* The agent acks the call but fails to get moved to the caller's bridge.  The same bridge action could be done here with the same pitfalls.

It is just simpler to punt and hangup the channel.

> On July 4, 2013, 4:27 p.m., Matt Jordan wrote:
> > /trunk/apps/app_agent_pool.c, lines 1948-1949
> > <https://reviewboard.asterisk.org/r/2657/diff/1/?file=41120#file41120line1948>
> >
> >     As noted previously, I'd set a channel variable here that noted whether or not the channel was hung up or was logged off.
> >     
> >     Since a log off doesn't necessarily hangup the channel, shouldn't this return 0?

Local channel optimization gets in the way as earlier.

> On July 4, 2013, 4:27 p.m., Matt Jordan wrote:
> > /trunk/apps/app_agent_pool.c, lines 2351-2374
> > <https://reviewboard.asterisk.org/r/2657/diff/1/?file=41120#file41120line2351>
> >
> >     Document both of these events at the top of the file. Reference the events from the Agents action.

Also added the logged in channel uniqueid to the event.

> On July 4, 2013, 4:27 p.m., Matt Jordan wrote:
> > /trunk/apps/app_agent_pool.c, lines 1413-1416
> > <https://reviewboard.asterisk.org/r/2657/diff/1/?file=41120#file41120line1413>
> >
> >     Consider reducing indentation here by bailing early and/or jumping to a cleanup point if the bridge features cannot be initialized.

Added an agent_run_cleanup goto target.

- rmudgett

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/2657/#review9074
-----------------------------------------------------------

On July 4, 2013, 1:14 a.m., rmudgett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2657/
> -----------------------------------------------------------
> 
> (Updated July 4, 2013, 1:14 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-21554
>     https://issues.asterisk.org/jira/browse/ASTERISK-21554
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> The ill conceived chan_agent is no more.  It is now replaced by app_agent_pool.
> 
> Agents login using the AgentLogin() application as before.  The AgentLogin() application no longer does any authentication.  Authentication is now the responsibility of the dialplan.  (Besides, the authentication done by chan_agent did not match what the voice prompts asked for.)
> 
> Sample extensions.conf
> [login]
> ; Sample agent 1001 login
> ; Set COLP for in between calls so the agent does not see the last caller COLP.
> exten => 1001,1,Set(CONNECTEDLINE(all)="Agent Waiting" <1001>)
> ; Give the agent DTMF transfer and disconnect features when connected to a caller.
> same => n,Set(CHANNEL(dtmf-features)=TX)
> same => n,AgentLogin(1001)
> same => n,NoOp(AGENT_STATUS is ${AGENT_STATUS})
> same => n,Hangup()
> 
> [caller]
> ; Sample caller direct connect to agent 1001
> exten => 800,1,AgentRequest(1001)
> same => n,NoOp(AGENT_STATUS is ${AGENT_STATUS})
> same => n,Hangup()
> 
> ; Sample caller going through a Queue to agent 1001
> exten => 900,1,Queue(agent_q)
> same => n,Hangup()
> 
> Sample queues.conf
> [agent_q]
> member => Local/800 at caller,,SuperAgent,Agent:1001
> 
> Under the hood operation overview:
> 1) Logged in agents wait for callers in an agents holding bridge.
> 2) Caller requests an agent using AgentRequest()
> 3) A basic bridge is created, the agent is notified, and caller joins the basic bridge to wait for the agent.
> 4) The agent is either automatically connected to the caller or must ack the call to connect.
> 5) The agent is moved from the agents holding bridge to the basic bridge.
> 6) The agent and caller talk.
> 7) The connection is ended by either party.
> 8) The agent goes back to the agents holding bridge.
> 
> To avoid some locking issues with the agent holding bridge, I needed to make some changes to the after bridge callback support.  The after bridge callback is now a list of requested callbacks with the last to be added the only active callback.  The after bridge callback for failed callbacks will always happen in the channel thread when the channel leaves the bridging system or is destroyed.
> 
> 
> Diffs
> -----
> 
>   /trunk/CHANGES 393634 
>   /trunk/UPGRADE.txt 393634 
>   /trunk/apps/app_agent_pool.c PRE-CREATION 
>   /trunk/channels/chan_agent.c 393634 
>   /trunk/configs/agents.conf.sample 393634 
>   /trunk/configs/queues.conf.sample 393634 
>   /trunk/include/asterisk/bridging.h 393634 
>   /trunk/include/asterisk/config_options.h 393634 
>   /trunk/include/asterisk/stasis_channels.h 393634 
>   /trunk/main/bridging.c 393634 
>   /trunk/main/stasis_channels.c 393634 
> 
> Diff: https://reviewboard.asterisk.org/r/2657/diff/
> 
> 
> Testing
> -------
> 
> Tested the features of agents and they work as expected.
> 1) Agents login and get MOH and any COLP set by the dialplan before AgentLogin().
> 2) Agents logging in with a local channel chain wait for the local channels to optimize out.
> 3) Caller channels directly running AgentRequest() are able to connect to the agent.
> 4) Callers going through Queue() can connect to agents via local channels.  The local channels can optimize themselves out.
> 5) Tested recording agent calls.  Note: Agent calls cannot be recorded currently if the caller came in on an optimizing local channel because MixMonitor audio hooks are not being handled by the optimization.
> 6) Caller COLP is shown to the agent before the agent accepts the call.
> 7) The original COLP when the agent logged in is restored while the agent is between calls.
> 8) Tested agent wrapup time.
> 9) Tested agent not acknowledging the call before autologoff time.
> 
> Many more.
> 
> 
> Thanks,
> 
> rmudgett
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130705/374c0e83/attachment-0001.htm>