/branches/1.8/res/res_rtp_asterisk.c (Diff revision 1)

Why are we checking schedid == -1 here.   This implies that the thread removing the scheduled item is not the same as the thread executing the scheduled event.  If this is the case, how are we guaranteed schedid won't get set to -1 after this check occurs while still in the callback?   Would this cause a ref leak or anything?

1. Russell Bryant Sept. 19, 2011, 2:38 a.m. (Sept. 19, 2011, 2:38 a.m.)

   Excellent observations.  You are correct that it is not guaranteed that the thread canceling the scheduled item is not the same one executing the scheduler.  Specifically, the SIP monitor thread is executing the scheduler and it is usually a channel thread canceling the scheduled item.  (sip_hangup -> stop_media_flows -> ast_rtp_instance_stop).
   
   There will not be a ref leak.  The reference is only released when either the scheduled item is successfully canceled or if the scheduler callback is returning 0 indicating that it is not going to run again.
   
   The other question, how are we guaranteed schedid won't get set to -1 after the check occurs, is more complicated.  The direct answer is we're not guaranteed.  The next question would be, is it harmful that there is no synchronization here?  I went back and forth with myself a bit on whether or not locking should be added here.  I obviously didn't want to add it unless it was absolutely necessary and ended up going without it.
   
   If schedid gets set to -1 (because the RTP instance got stopped) while this function is running, it's just going to do its thing like usual and transmit one last RTCP packet.  That doesn't seem harmful.  It's just going to catch the -1 the next time it runs and then bail out and unref the RTP instance.  So, it all works out in the end.  It's a little less pretty, but at least no more locks get added.
   
   As I wrote this out, the one down side I thought of is that if this edge case gets hit, it would delay the destruction of the RTP instance until the next RTCP packet is scheduled to be sent.  By default, this is every 5 seconds, but it's configurable.  I'm ok with that personally ...
   
   Now, for the lulz, here is some code in the handling of the rtcpinterval configuraiton option in res_rtp_asterisk.c:
   
                           if (rtcpinterval == 0)
                                   rtcpinterval = 0; /* Just so we're clear... it's zero */
   

/branches/1.8/res/res_rtp_asterisk.c (Diff revision 1)

There are macros for handling the ref counting of stuff associated with a scheduled event.

1. Russell Bryant Sept. 19, 2011, 2:24 a.m. (Sept. 19, 2011, 2:24 a.m.)

   Yeah ... I avoided it because AST_SCHED_DEL_UNREF() is even more evil than AST_SCHED_DEL().  It does the "loop 10 times" thing and also makes some assumptions about how the sched ID variable is handled and when it gets set to -1.  I would rather either leave it this way or start writing a new set of helper macros that are less evil.

2. Tilghman Lesher Sept. 19, 2011, 6:21 a.m. (Sept. 19, 2011, 6:21 a.m.)

   Best way to make it less evil is to change the assumption in the scheduler code that when a callback is running, it's removed from the list.  It's a basic assumption that has led to the number of scheduler macros that work on a principle of "scheduler jobs shouldn't take more than 1 second to run, so sometime in this sequence, it should eventually cancel the task".  It's probably past time to break that assumption in the scheduler
   code and get rid of the race condition between a running task that always reschedules itself and another thread trying to cancel that task.

A 5 second delayed destruction isn't that big of a deal.  I'm fine with this and completely agree that introducing any sort of synchronization here would just make this more complicated than is necessary.  Great Work!