[asterisk-dev] [Code Review] 3362: func_periodic_hook: New function for periodic hooks.

Mon Mar 31 07:06:05 CDT 2014

> On March 30, 2014, 12:12 a.m., Corey Farrell wrote:
> > Do you have any ideas on how this will effect CPU load?
> > 
> > I was thinking of this as an example:
> > A system with 1 inbound call per second, each call lasting 5 minutes.  Every inbound channel gets a 60 second timer for beep Playback (assume the timer runs 5 times).   The inbound channel then dials outbound.  You would reach about 600 concurrent "real" channels (300 in, 300 out), 2 real channels created and destroyed every second.
> > 
> > Of the 300 timers on inbound channels, 5 will fire every second.  This means 10 local channels are created and destroyed every second.  This gives us a total of 610 concurrent channels, 12 channels created and destroyed every second.  I know channel creation/destruction isn't a lightweight operation, but I don't know if it will have a big impact overall.

I don't have any data.  Compared to the cost of all of the other things involved in call processing, it seems relatively low impact to me.  Usage of this feature implies Asterisk is processing the media for that call, which is going to be a lot more expensive than this, presumably.  (potentially lots of DB hits during setup/teardown, transcoding, disk I/O, ...)

> On March 30, 2014, 12:12 a.m., Corey Farrell wrote:
> > /trunk/funcs/func_periodic_hook.c, lines 54-56
> > <https://reviewboard.asterisk.org/r/3362/diff/4/?file=56930#file56930line54>
> >
> >     Need to document and enforce a minimum interval.  Also need to document that interval is whole seconds only.

Updated to enforce that the interval is non-zero.

> On March 30, 2014, 12:12 a.m., Corey Farrell wrote:
> > /trunk/funcs/func_periodic_hook.c, line 133
> > <https://reviewboard.asterisk.org/r/3362/diff/4/?file=56930#file56930line133>
> >
> >     state->interval could be added to hook_thread_arg so we can use it for dial timeout.

I don't think the timeout is important here.  The extension called is something this module defines.

> On March 30, 2014, 12:12 a.m., Corey Farrell wrote:
> > /trunk/funcs/func_periodic_hook.c, line 206
> > <https://reviewboard.asterisk.org/r/3362/diff/4/?file=56930#file56930line206>
> >
> >     state->last_hook is never pre-initialized to ast_tvnow(), so hooks would always trigger on first check after being created or re-enabled.  Unless this behaviour is documented I would expect the first run after create or resume to be delayed state->interval seconds.

Fair.  I had originally intended for it to run immediately, but I think it makes more sense to delay.

> On March 30, 2014, 12:12 a.m., Corey Farrell wrote:
> > /trunk/funcs/func_periodic_hook.c, line 273
> > <https://reviewboard.asterisk.org/r/3362/diff/4/?file=56930#file56930line273>
> >
> >     Need checks for !ast_strlen_zero on all args.  Maybe also a verify the exten at context exists?

I think this is all handled.  For interval, sscanf() will just fail on an empty string.  For exten and context, defaults of "s" and "default" are applied.

Also, I don't think it should check for the exten to exist.  If it doesn't, errors should show up when it tries to run.  Extensions can be added/removed dynamically, so I think we should just assume the best here.

> On March 30, 2014, 12:12 a.m., Corey Farrell wrote:
> > /trunk/funcs/func_periodic_hook.c, line 299
> > <https://reviewboard.asterisk.org/r/3362/diff/4/?file=56930#file56930line299>
> >
> >     Can we detach the audio hook here?  I know we escape the hook callback very early but it still feels wasteful to keep it enabled.
> >     
> >     Possibly remove state->disabled variable?

As you said, the callback does just about nothing, so I don't think it really hurts to keep it.

But honestly, I would have done it this way if the audiohooks API was built for dynamic adding/removing.  It really isn't.  No other code does that.  It all relies on channel destruction to tear it down.

> On March 30, 2014, 12:12 a.m., Corey Farrell wrote:
> > /trunk/funcs/func_periodic_hook.c, lines 345-352
> > <https://reviewboard.asterisk.org/r/3362/diff/4/?file=56930#file56930line345>
> >
> >     Should we have an option to delete a hook?  Something to free all memory from a periodic_hook that will not be used again.

It's a pretty tiny amount of memory, I think.

Also, the hook can't really be deleted without work to the audiohooks API from my testing.

> On March 30, 2014, 12:12 a.m., Corey Farrell wrote:
> > /trunk/funcs/func_periodic_hook.c, lines 394-396
> > <https://reviewboard.asterisk.org/r/3362/diff/4/?file=56930#file56930line394>
> >
> >     Is this due to a technical limitation of ChanSpy?  If not we should incorporate hook_id into the group variable so we limit 1 channel per hook_id.
> >     
> >     Maybe add to documentation that a timer interval will be skipped if the channel from the previous interval hasn't ended.

Good call on adding the ID here.  The intention is to prevent a hook from running if it's already running.  It was done before I made it so you could have more than one hook at a time.

It's not a technical limitation of ChanSpy, more just built in sanity checking / rate limiting.

> On March 30, 2014, 12:12 a.m., Corey Farrell wrote:
> > /trunk/funcs/func_periodic_hook.c, lines 68-71
> > <https://reviewboard.asterisk.org/r/3362/diff/4/?file=56930#file56930line68>
> >
> >     Can we get a documented variable for the original channel name or uniqueid?  I'd prefer something defined by this module (so not SPY_CHANNEL).

Oops, this shouldn't have been using SPY_CHANNEL, as it's totally unrelated to what ChanSpy uses SPY_CHANNEL for.  I changed that.

As for setting a variable for the original channel in the hook, that's a good idea.  I suppose this should be reworked to exec GoSub with arguments instead of run an extension directly.  That's the only way I can see to pass variables to the hook.

- Russell

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

On March 29, 2014, 8:27 p.m., Russell Bryant wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3362/
> -----------------------------------------------------------
> 
> (Updated March 29, 2014, 8:27 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
>     This commit introduces a new dialplan function, PERIODIC_HOOK().
>     It allows you run to a dialplan hook on a channel periodically.  The
>     original use case that inspired this was the ability to play a beep
>     periodically into a call being recorded.  The implementation is much
>     more generic though and could be used for many other things.
>     
>     The implementation makes heavy use of existing Asterisk components.
>     It uses a combination of Local channels and ChanSpy() to run some
>     custom dialplan and inject any audio it generates into an active call.
>     
>     The other important bit of the implementation is how it figures out
>     when to trigger the beep playback.  This implementation uses the
>     audiohook API, even though it's not actually touching the audio in any
>     way.  It's a convenient way to get a callback and check if it's time
>     to kick off another beep.  It would be nice if this was timer event
>     based instead of polling based, but unfortunately I don't see a way to
>     do it that won't interfere with other things.
> 
> 
> Diffs
> -----
> 
>   /trunk/funcs/func_periodic_hook.c PRE-CREATION 
>   /trunk/CHANGES 411572 
> 
> Diff: https://reviewboard.asterisk.org/r/3362/diff/
> 
> 
> Testing
> -------
> 
> Called the following extension (100 at test), both letting it run all the way through, as well as hanging up at various points in the middle.
> 
> [hooks]
> 
> exten => beep,1,Answer()
>     same => n,Playback(beep)
> 
> [test]
> 
> exten => 100,1,Answer()
>     same => n,Set(BEEP_ID=${PERIODIC_HOOK(hooks,beep,5)})
>     same => n,Wait(20)
>     same => n,Set(PERIODIC_HOOK(${BEEP_ID})=off)
>     same => n,Wait(20)
>     same => n,Set(PERIODIC_HOOK(${BEEP_ID})=on)
>     same => n,Wait(20)
>     same => n,Hangup()
> 
> 
> Thanks,
> 
> Russell Bryant
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140331/e613f99a/attachment-0001.html>