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

[rmudgett at digium.com]

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


My two cents. FWIW.

Other than my concern below it looks good.


/branches/1.6.0/apps/app_queue.c
<https://reviewboard.asterisk.org/r/651/#comment4213>

    Is it safe to just initialize this one field in the struct call_queue tmpq instance?  The previous code had everything else zeroed in the declaration initialization.
    
    This is done in several other places in the patch.
    
    Another thing to be aware of is the fact that struct call_queue is a sizable entity for a stack variable.  (This patch makes it even larger.)


- rmudgett


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
> 
>