[asterisk-dev] [Code Review] Fix FRACK message caused by chan_iax2

[Tilghman Lesher]

> On 2010-08-13 13:03:57, David Vossel wrote:
> > /branches/1.8/channels/chan_iax2.c, lines 1709-1715
> > <https://reviewboard.asterisk.org/r/861/diff/2/?file=12260#file12260line1709>
> >
> >     I'm not sure the DONT_RESHEDULE define is necessary.  I understand what it achieves, but the same thing could be achieved in the scheduler callback by simply checking if the schedule id is -1 or not.  If the id is already -1 when it enters the sched callback, then it was (or was attempted) to be deleted from the scheduler. That should indicate that it should not be rescheduled and should not even be queued to the next available thread.
> >     
> >     So I'm thinking something like this.
> >     
> >     send_lagrq() {
> >     
> >     lock
> >     if (iaxs[callno])
> >        if iaxs[callno].lagid == -1
> >            unlock
> >            return 0;
> >        else
> >            iaxs[callno].lagid = -1;
> >     unlock
> >     
> >     __send_lagrq();
> >     
> >     return 0
> >     
> >     }

Well, not really.  -1 is just our universal ID to mean "no job is presently scheduled".  Your method does not work, because the rescheduling occurs not in the scheduled task, but in the thread that is signalled from the scheduled task.  You still need to communicate to the actual task "don't reschedule this task after it's complete".

Since all negative job IDs are invalid specifiers, I simply reappropriated one that isn't typically used (-2) to signal that the job isn't to be rescheduled.


- Tilghman


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


On 2010-08-13 12:35:52, Tilghman Lesher wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/861/
> -----------------------------------------------------------
> 
> (Updated 2010-08-13 12:35:52)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> The FRACK message out of the scheduler indicates that we attempted to cancel an ID that does not exist.  This usually indicates a race condition.
> 
> What I've found is that due to the way that we schedule multiple threads in chan_iax2 to run scheduled tasks, there can be an extended period of time that a scheduled task has been successfully run by the scheduler, yet the task has not actually run, and therefore, has not altered the stored scheduler ID.  When teardown attempts to cancel all scheduled tasks, the scheduler complains and shortly thereafter, the task complains that the callno does not exist.
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/channels/chan_iax2.c 282013 
>   /branches/1.8/include/asterisk/sched.h 282013 
> 
> Diff: https://reviewboard.asterisk.org/r/861/diff
> 
> 
> Testing
> -------
> 
> Since this is a race condition, it's difficult to test.  However, I have run with this patch for extended periods (i.e. overnight) that would have previously caused the FRACK message to appear several times, and the FRACK message did not appear.
> 
> 
> Thanks,
> 
> Tilghman
> 
>