[asterisk-dev] [Code Review] A whole mess of ref counting cleanups

[Mark Michelson]

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



/trunk/main/asterisk.c
<https://reviewboard.asterisk.org/r/2137/#comment13821>

    This change is going to be problematic because the atexit handler runs too late.
    
    The messaging core makes use of a special message queue channel. The only way for this channel to be unreffed is if the message core gets told to shut down. By doing that shutdown operation here, we ensure that the channel reference goes away before Asterisk goes into its wait loop for all channel references to be gone. With this change, a "core stop gracefully" can potentially block forever if messaging was used.



/trunk/main/event.c
<https://reviewboard.asterisk.org/r/2137/#comment13822>

    This shouldn't be necessary since
    
    1) globals automatically are zeroed.
    2) struct initializers automatically zero unspecified fields.
    
    This goes for the other assignments.



/trunk/main/event.c
<https://reviewboard.asterisk.org/r/2137/#comment13823>

    This certainly works, but a more typical pattern used when deleting items in a linked list is:
    
    while ((item = AST_RWDLLIST_REMOVE_HEAD(...))) {
        delete_item(item);
    }
    
    It's certainly simpler.



/trunk/main/format.c
<https://reviewboard.asterisk.org/r/2137/#comment13824>

    Consider changing these debug strings to be something like "unref format_list container on shutdown". The reason is that while we have all intention of destroying them here, there may arise some sort of refcount bug at some point where we actually don't end up destroying these here. While it wouldn't be smart of someone, I could easily see someone looking at a ref log, seeing the word "destroy" in the debug message and automatically assuming that the container got destroyed properly.



/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/2137/#comment13825>

    Comment should probably say "Unregister" instead of "Register"



/trunk/main/message.c
<https://reviewboard.asterisk.org/r/2137/#comment13826>

    See comment in asterisk.c about this.


- Mark


On Sept. 30, 2012, 8:37 p.m., Matt Jordan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2137/
> -----------------------------------------------------------
> 
> (Updated Sept. 30, 2012, 8:37 p.m.)
> 
> 
> Review request for Asterisk Developers, Mark Michelson, rmudgett, and jcolp.
> 
> 
> Summary
> -------
> 
> While hunting for a channel ref leak, I found that after running the refcounter utility, there were a whole host of ao2 object ref leaks.  Wondering if that was just my test causing that, I started Asterisk and immediately stopped it.
> 
> Nope, lots of ref leaks still.
> 
> This patch cleans up a fair number of them - so that at least with my admittedly limited configuration (which had a number of modules excluded), there were no longer any ao2 ref leaks on Asterisk shutdown.  Most of these are simply Asterisk not properly cleaning up after itself on shutdown; however, a few could arguably result in slow growing memory leaks, particularly in systems that used frequent reloads.
> 
> Still haven't found the channel ref leak, but this does remove a lot of error messages.
> 
> Note that while this patch is for trunk, a version should be applied to 1.8, 10, and 11 as well.
> 
> For Richard:
> 
> I noticed that some of the internally allocated nodes in astobj2 would not be represented in a REF_DEBUG log, as they never used a 'debug' version of ao2_alloc or ao2_ref.  Unfortunately, a few code paths resulted in those same nodes showing up in the REF_DEBUG log, leading to warnings that nodes were appearing that were never allocated.  This patch manages to get all of the nodes represented in a REF_DEBUG log; however, I imagine that for the red/black tree work, this is going to be compounded.  If you have another approach you'd like to take other than what I have here, let me know.
> 
> 
> Diffs
> -----
> 
>   /trunk/channels/chan_agent.c 374095 
>   /trunk/include/asterisk/astobj2.h 374095 
>   /trunk/main/asterisk.c 374095 
>   /trunk/main/astobj2.c 374095 
>   /trunk/main/ccss.c 374095 
>   /trunk/main/cel.c 374095 
>   /trunk/main/channel.c 374095 
>   /trunk/main/config_options.c 374095 
>   /trunk/main/data.c 374095 
>   /trunk/main/db.c 374095 
>   /trunk/main/event.c 374095 
>   /trunk/main/features.c 374095 
>   /trunk/main/format.c 374095 
>   /trunk/main/format_pref.c 374095 
>   /trunk/main/indications.c 374095 
>   /trunk/main/manager.c 374095 
>   /trunk/main/message.c 374095 
>   /trunk/main/named_acl.c 374095 
>   /trunk/main/pbx.c 374095 
>   /trunk/main/taskprocessor.c 374095 
>   /trunk/main/udptl.c 374095 
>   /trunk/main/xmldoc.c 374095 
>   /trunk/res/res_musiconhold.c 374095 
>   /trunk/res/res_xmpp.c 374095 
> 
> Diff: https://reviewboard.asterisk.org/r/2137/diff
> 
> 
> Testing
> -------
> 
> A whole lot of loading/unloading, changing configuration, and generally peeking through REF_DEBUG logs.
> 
> 
> Thanks,
> 
> Matt
> 
>

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