[asterisk-dev] [Code Review]: Fix timeout antipattern throughout the code

[Mark Michelson]

> On Oct. 1, 2012, 6:48 p.m., rmudgett wrote:
> > Enough for now will look more tomorrow.

Thanks a bunch. I'll make the suggested changes and put up a new diff as soon as I can.


> On Oct. 1, 2012, 6:48 p.m., rmudgett wrote:
> > /branches/1.8/apps/app_queue.c, line 3492
> > <https://reviewboard.asterisk.org/r/2135/diff/1/?file=31503#file31503line3492>
> >
> >     Change to:
> >     while ((*to = ast_remaining_ms(start_time_tv, orig)) && !peer) {
> >     
> >     Then many changes can be reverted in this function.

Same as for app_dial.


> On Oct. 1, 2012, 6:48 p.m., rmudgett wrote:
> > /branches/1.8/apps/app_queue.c, line 3905
> > <https://reviewboard.asterisk.org/r/2135/diff/1/?file=31503#file31503line3905>
> >
> >     Revertable
> >     
> >     This also changed behavior if the timeout was infinite.

I'll need to both revert this and move it outside the while loop.


> On Oct. 1, 2012, 6:48 p.m., rmudgett wrote:
> > /branches/1.8/apps/app_dial.c, line 1099
> > <https://reviewboard.asterisk.org/r/2135/diff/1/?file=31500#file31500line1099>
> >
> >     Just change to:
> >     while ((*to = ast_remaining_ms(start, orig)) && !peer) {
> >     
> >     Then the following changes in this function can be reverted.

That's a great idea. I need to move the checks on lines 1631 and 1633 (lines 1629 and 1631 in the original) to outside the while loop.


> On Oct. 1, 2012, 6:48 p.m., rmudgett wrote:
> > /branches/1.8/apps/app_record.c, line 335
> > <https://reviewboard.asterisk.org/r/2135/diff/1/?file=31504#file31504line335>
> >
> >     How about
> >     while (ms = ast_remaining_ms(start, maxduration)) {
> >     
> >     This suggestion could be done many other places throughout this patch.

Sure, I'll scan for more places this can change as well.


- Mark


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


On Sept. 28, 2012, 5:15 p.m., Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2135/
> -----------------------------------------------------------
> 
> (Updated Sept. 28, 2012, 5:15 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This is an interesting one. I filed issue ASTERISK-20375 a while back and David Lee fixed it. The problem was that in my setup, a call to ast_waitfor_nandfds() returned in less than a millisecond. The result was that the input-output value passed into ast_waitfor_nandfds() would never decrement the value properly. Thus a 3000 ms timeout would take *days* to actually time out.
> 
> David discovered that a similar pattern for determining the passage of time had been used throughout the code. This patch is an attempt to fix all of those. I've created a function in time.h called ast_remaining_ms() that will tell the amount of time remaining given a starting timestamp and the duration that the original timeout was. This is useful for determining the timeout to pass to an ast_waitfor() or related function.
> 
> Most of the changes here were straightforward, but there were some tricky bits. Focus strongly on the following changes:
> 
> 1) wait_for_answer() in app_dial.c
> 2) wait_for_answer() in app_queue.c
> 3) generic_fax_exec() in res_fax.c
> 4) places where I have started setting "res" instead of "ms" or something similar in ast_waitfor() calls. Ensure that I have not tainted a return value. I found a few of these places while testing and fixed what I found on my own, but I could have missed some places.
> 5) places where a timeout is supposed to restart. I've tried to find these places, but I may have missed some.
> 
> In addition, there has been a change made to the return of ast_waitfor(). Prior to this patch, if a negative timeout were passed to ast_waitfor(), it would be impossible to return a negative value. This meant that it was impossible to detect if an error had occurred. I've changed ast_waitfor() to be able to return a negative value if a negative timeout is passed in, but only if the underlying call to ast_waitfor_nandfds() returns NULL.
> 
> I did this because I found many places in the code with this construct:
> 
> if (ast_waitfor(chan, -1) < 0) {
>     ast_log(LOG_ERROR, "ERMAHGERD!\n");
>     return HORRIBLE_ERROR_CONDITION;
> }
> 
> The problem is that the error return could never actually be reached. It should be possible with my change. Please let me know if my change could cause problems though.
> 
> 
> This addresses bug ASTERISK-20414.
>     https://issues.asterisk.org/jira/browse/ASTERISK-20414
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/apps/app_dial.c 373847 
>   /branches/1.8/apps/app_jack.c 373847 
>   /branches/1.8/apps/app_meetme.c 373847 
>   /branches/1.8/apps/app_queue.c 373847 
>   /branches/1.8/apps/app_record.c 373847 
>   /branches/1.8/apps/app_waitforring.c 373847 
>   /branches/1.8/channels/chan_agent.c 373847 
>   /branches/1.8/channels/chan_dahdi.c 373847 
>   /branches/1.8/channels/chan_iax2.c 373847 
>   /branches/1.8/channels/sig_analog.c 373847 
>   /branches/1.8/channels/sig_pri.c 373847 
>   /branches/1.8/include/asterisk/time.h 373847 
>   /branches/1.8/main/channel.c 373847 
>   /branches/1.8/main/pbx.c 373847 
>   /branches/1.8/main/rtp_engine.c 373847 
>   /branches/1.8/main/utils.c 373847 
>   /branches/1.8/res/res_fax.c 373847 
> 
> Diff: https://reviewboard.asterisk.org/r/2135/diff
> 
> 
> Testing
> -------
> 
> I've placed many test calls through items that used timeouts like Dial and Queue with a timeout. I've also ensured that other apps that use ast_waitfor() such as Answer() and Echo() still function properly.
> 
> 
> Thanks,
> 
> Mark
> 
>

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