[asterisk-dev] [Code Review] 4498: res_pjsip: Enable unload of all modules at shutdown

[Kevin Harwell]

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



/branches/13/res/res_pjsip_outbound_registration.c
<https://reviewboard.asterisk.org/r/4498/#comment25410>

    I ran the res_pjsip_outbound_registration testsuite test locally and never received any fracks/crashes with this line added or removed.
    
    I *think* removing the unref causes a leak. The pjsip code holds onto a reference of the client state. Each time pjsip_regc_send gets called the ref on the client_state is incremented. The timer_cb removes this ref.


While testing res_pjsip_outbound_registration I wanted to see if a frack would occur while unloading during registration attempts, but instead Asterisk crashed after unloading.  I believe this is due to the module unloading, yet pjsip is still attempting to register to a non existent remote server. After a timeout it then attempts to call back into the module, which has now been unloaded.

A similar thing occurred with res_pjsip_outbound_publish and some code was put into place that makes the module wait to fully unload until all registration attempts are complete. Something similar will need to be done here for outbound registrations.

That being said if you feel that fixing this is outside the scope of the changes found here then an issue can be made so it can be fixed at a later time.

- Kevin Harwell


On March 20, 2015, 11:17 p.m., Corey Farrell wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4498/
> -----------------------------------------------------------
> 
> (Updated March 20, 2015, 11:17 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-24731
>     https://issues.asterisk.org/jira/browse/ASTERISK-24731
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This patch allows all PJSIP modules to shutdown cleanly, once approved we'll be able enable it in the Bamboo Reference Checks Job.
> 
> * Move most of res_pjsip:module_unload to unload_pjsip to resolve crashes caused by running PJSIP functions from non-PJSIP threads.
> * Remove call to pjsip_endpt_destroy(ast_pjsip_endpoint), it was causing crashes in some cases.  In theory pj_shutdown() should take care of this for us.
> * Mark res_pjsip_keepalive and res_pjsip_session as allowed to unload at shutdown.
> * Resolve leaked config global in res_pjsip_notify.
> * Unregister pubsub pjsip service module.
> * Implement cleanup for res_pjsip_session.
> * Fix a pre-existing FRACK in res_pjsip_outbound_registration.
> 
> More about res_pjsip_outbound_registration:
> tests/channels/pjsip/ami/show_registrations_outbound has an AO2 FRACK during shutdown.  I get this error with or without my patch.  I've removed what I believe is an extra unref, but this doesn't solve all the FRACK's.
> 
> 
> Diffs
> -----
> 
>   /branches/13/res/res_pjsip_session.c 433244 
>   /branches/13/res/res_pjsip_pubsub.c 433244 
>   /branches/13/res/res_pjsip_outbound_registration.c 433244 
>   /branches/13/res/res_pjsip_notify.c 433244 
>   /branches/13/res/res_pjsip_keepalive.c 433244 
>   /branches/13/res/res_pjsip.c 433244 
> 
> Diff: https://reviewboard.asterisk.org/r/4498/diff/
> 
> 
> Testing
> -------
> 
> All of tests/channels/pjsip.  Some tests still have reference leaks, but most tests do not.
> 
> Can someone retest tests/channels/pjsip/ami/show_registrations_outbound to confirm that I haven't made it worse?  Seems to make no difference on my system, but Bamboo doesn't seem to have a problem with this test.
> 
> 
> Thanks,
> 
> Corey Farrell
> 
>

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