[asterisk-dev] [Code Review] fix using external MP3 player with res_timing_dahdi being used as timer causing MOH stream to stop

[mjordan]

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



/trunk/res/res_musiconhold.c
<https://reviewboard.asterisk.org/r/1578/#comment9206>

    I'm not 100% sure that we can lose the lock around the mohdata members here, since the members have a reference back to the class that can be accessed in moh_release.
    
    Theoretically, another thread could call ast_deactivate_generator on a channel at the same time that the moh class is unref'd to 0.  This is highly unlikely, since it looks like an accessible mohclass only get's ref'd to 0 when the module is unloaded, but its not impossible - particularly if someone unloaded the module from the CLI.
    
    You may want to move the removal of the mohmembers to the beginning of the destructor so that it's the first thing that happens when a class is destroyed, and, unless its causing some other issue, keep the lock.  The underlying ao2 object should still be okay at this point, and it may prevent some odd race condition in the scenario above.



/trunk/res/res_musiconhold.c
<https://reviewboard.asterisk.org/r/1578/#comment9207>

    I don't think you can bump the dependency level here to be the same as the channel drivers.  The channel drivers depend on MOH - having the MOH module load at the same time as the channel driver could introduce problems, as the order in which modules are loaded that have the same priority isn't specified.
    
    The problem isn't that MOH is at the wrong dependency level, but that the timing modules, which MOH depends on, apparently should be completely loaded before loading MOH.


- mjordan


On Nov. 14, 2011, 11:02 a.m., elguero wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1578/
> -----------------------------------------------------------
> 
> (Updated Nov. 14, 2011, 11:02 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> The attached patch does the following:
> 
> - Changes the load priority so that this module is loaded after the timing interfaces are.
> 
> At times, res_musichold.so would work with an external mp3 player.  Through debugging, I noticed that res_timing_pthread was being used at first.  If I only loaded res_timing_dahdi, then the external mp3 stream would start and then pause causing nothing to be heard on the channel. So, if res_timing_pthread was present at start, upon reload, since res_timing_dahdi takes priority as a timer, the timing changed to this timing interface and would just sit there, hence the need for the following change.
> 
> - Adds the POLLPRI event for ast_poll, otherwise ast_poll just sits there waiting when the timer being used is res_timing_dahdi.so
> 
> - Attempt to cleanup a few items
> 
> 
> This addresses bug ASTERISK-17474.
>     https://issues.asterisk.org/jira/browse/ASTERISK-17474
> 
> 
> Diffs
> -----
> 
>   /trunk/res/res_musiconhold.c 344329 
> 
> Diff: https://reviewboard.asterisk.org/r/1578/diff
> 
> 
> Testing
> -------
> 
> Local box running CentOS 5.7 and dahdi-trunk.
> 
> On JIRA, tested by:
> Thomas Arimont - 1.8.7
> Luke H - 1.8.8-rc3, CentOS 5.5 (32bit), DAHDI 2.5.0.2
> 
> 
> Thanks,
> 
> elguero
> 
>

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