[asterisk-dev] [Code Review] Fix memory leaks and hogging in app_queue.c

[Russell Bryant]

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

Ship it!


- Russell


On 2010-05-07 10:38:24, Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/651/
> -----------------------------------------------------------
> 
> (Updated 2010-05-07 10:38:24)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This review request is for the patch on issue 17081.
> 
> A user reported that he saw increasing numbers of allocations stemming
> from app_queue.c when he would run the "queue show" CLI command. The
> user reported that he was using approximately 40 realtime queues and
> as he ran the CLI command more and more, the memory usage would shoot up.
> As it turns out, there was a memory leak and a separate usage of memory
> that, while not really a leak, was very irresponsible.
> 
> Both memory problems can be attributed to the function init_queue(). When
> the "queue show" command is run, all realtime queues have the init_queue()
> function called on the in-memory queue. The idea is to place the queue in
> its default state and then overwrite options specified in the realtime backend
> as we read them.
> 
> The first problem, the memory leak, had to do with the fact that the string
> field for the name of the first periodic announcement file was being re-created
> every time init_queue was called. This patch corrects the behavior by only
> calling ast_str_create if the memory has not already been allocated. 
> 
> The other problem is a bit more complicated. The majority of the strings
> in the call_queue structure were changed to use the ast_string_fields API
> for 1.6.0 and beyond. init_queue resets all string fields on the queue to
> their default values. Then, later in the realtime queue loading process,
> these string fields are set to their configured values.
> 
> For those unfamiliar with string fields, frequent resizing of a string like
> this is not what the string fields API is designed for. The result of this
> constant resizing is that as the queue gets loaded, eventually space for
> the string runs out and so a new memory pool, at twice the size of the
> previously allocated one, is created for the string fields. The reporter
> of issue 17081 wrote a script that ran the "queue show" CLI command 2100
> times. By the end, each of his 40 queues was taking about a megabyte of
> memory apiece just for their string fields.
> 
> My fix for this problem is to revert the call_queue structure from using
> string fields. In my patch here, I have moved the queue back to using
> fixed-sized buffers. I ran the script provided by the reporter of 17081
> and determined that I no longer saw the steadily-increasing memory usage
> that I had seen before applying the patch.
> 
> This patch is applicable to the 1.6.0, 1.6.1, and 1.6.2 branches of
> Asterisk. Asterisk trunk (to be 1.8 eventually) does not have this problem
> because the string fields API has some extra checks in place to prevent it
> from being as much of a memory hog in a situation like this.
> 
> For this review, I'm mainly asking that you make sure that I did not make
> any silly mistakes, like using the wrong variable name somewhere. Also,
> I'm open for other suggestions for solving this.
> 
> 
> This addresses bug 17081.
>     https://issues.asterisk.org/view.php?id=17081
> 
> 
> Diffs
> -----
> 
>   /branches/1.6.0/apps/app_queue.c 260567 
> 
> Diff: https://reviewboard.asterisk.org/r/651/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Mark
> 
>