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

Matt Jordan reviewboard at asterisk.org
Mon Oct 1 15:04:10 CDT 2012 


> On Oct. 1, 2012, 11:03 a.m., Mark Michelson wrote:
> > /trunk/main/event.c, line 181
> > <https://reviewboard.asterisk.org/r/2137/diff/1/?file=31565#file31565line181>
> >
> >     This shouldn't be necessary since
> >     
> >     1) globals automatically are zeroed.
> >     2) struct initializers automatically zero unspecified fields.
> >     
> >     This goes for the other assignments.

Removed


> On Oct. 1, 2012, 11:03 a.m., Mark Michelson wrote:
> > /trunk/main/event.c, lines 1833-1836
> > <https://reviewboard.asterisk.org/r/2137/diff/1/?file=31565#file31565line1833>
> >
> >     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.

Agreed, changed.


> On Oct. 1, 2012, 11:03 a.m., Mark Michelson wrote:
> > /trunk/main/format.c, line 1084
> > <https://reviewboard.asterisk.org/r/2137/diff/1/?file=31567#file31567line1084>
> >
> >     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.

Changed.


> On Oct. 1, 2012, 11:03 a.m., Mark Michelson wrote:
> > /trunk/main/manager.c, line 7234
> > <https://reviewboard.asterisk.org/r/2137/diff/1/?file=31570#file31570line7234>
> >
> >     Comment should probably say "Unregister" instead of "Register"

Changed.


> On Oct. 1, 2012, 11:03 a.m., Mark Michelson wrote:
> > /trunk/main/message.c, line 1351
> > <https://reviewboard.asterisk.org/r/2137/diff/1/?file=31571#file31571line1351>
> >
> >     See comment in asterisk.c about this.

Changed.


> On Oct. 1, 2012, 11:03 a.m., Mark Michelson wrote:
> > /trunk/main/asterisk.c, line 1716
> > <https://reviewboard.asterisk.org/r/2137/diff/1/?file=31557#file31557line1716>
> >
> >     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.

Nuts.

I'll break this up into two pieces then:
1) The destruction of the taskprocessor queue, which will occur at this location.
2) The reclaimation of the rest of the resources message uses, which will continue to use the atexit handler.

Where possible, I'd prefer to use the atexit handlers, as it preserves the initialization/destruction order of the various components.

Because this wasn't painfully obvious that this is why there was a special exception for message, I'll rename and/or put a comment explaining why here.


- Matt


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


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/d968c727/attachment-0001.htm>

More information about the asterisk-dev mailing list