OSDir


[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Brigade memory lifetime & leaks


+1

> Am 05.06.2018 um 14:07 schrieb Plüm, Rüdiger, Vodafone Group <ruediger.pluem@xxxxxxxxxxxx>:
> 
> 
> 
>> -----Ursprüngliche Nachricht-----
>> Von: Stefan Eissing <stefan.eissing@xxxxxxxxxxxxx>
>> Gesendet: Dienstag, 5. Juni 2018 13:27
>> An: dev@xxxxxxxxxxxxxxxx
>> Cc: dev@xxxxxxxxxxxxxx
>> Betreff: Re: Brigade memory lifetime & leaks
>> 
>> 
>> 
>>> Am 05.06.2018 um 10:46 schrieb Joe Orton <jorton@xxxxxxxxxx>:
>>> 
>>> In 2.4's http_request.c there are two places doing:
>>> 
>>>   bb = apr_brigade_create(c->pool, c->bucket_alloc);
>>> 
>>> to handle sending a FLUSH between requests and an EOR bucket which
>> both
>>> can't be done off r->pool.  Because the brigade structure itself is
>>> allocated out of c->pool this means we leak (~64bytes) per request
>>> forever or up to request/conn limits.
>> 
>> Ok, not good. The first seems only to be triggered when request
>> data is still pending, but the second one always happens.
>> 
>>> We can thread a temp pool through to those functions and fix this,
>>> though because these core internal functions are in the public API (ha
>>> ha) this is the usual compat PITA, and I haven't worked out if that's
>>> more complex for the the async requesta processing call chain.
>>> As a PoC for non-async MPMs:
>>> http://people.apache.org/~jorton/2.4-use-temp-pool.patch
>>> 
>>> Another choice is to allocate the brigade structure using the bucket
>>> allocator and actually free it on _destroy().  Anybody around who can
>>> remember why we used a pool allocation for that structure from the
>>> beginning?
>> 
>> How about having the apr_bucket_brigade struct on the stack as an
>> option?
> 
> The usual approach in many locations is to store the created bucket brigade
> and reuse it. How about the following (untested):
> 
> Index: modules/http/http_request.c
> ===================================================================
> --- modules/http/http_request.c (revision 1832516)
> +++ modules/http/http_request.c (working copy)
> @@ -345,6 +345,16 @@
>     return rv;
> }
> 
> +#define RETRIEVE_BRIGADE_FROM_POOL(bb, key, pool, allocator) do { \
> +    apr_pool_userdata_get((void **)&bb, key, pool); \
> +    if (bb == NULL) { \
> +        bb = apr_brigade_create(pool, allocator); \
> +        apr_pool_userdata_setn((const void *)bb, key, NULL, pool); \
> +    } \
> +    else { \
> +        apr_brigade_cleanup(bb); \
> +    } \
> +} while(0)
> 
> AP_DECLARE(void) ap_process_request_after_handler(request_rec *r)
> {
> @@ -357,7 +367,8 @@
>      * this bucket is destroyed, the request will be logged and
>      * its pool will be freed
>      */
> -    bb = apr_brigade_create(c->pool, c->bucket_alloc);
> +    RETRIEVE_BRIGADE_FROM_POOL(bb, "ap_process_request_after_handler_brigade",
> +                               c->pool, c->bucket_alloc);
>     b = ap_bucket_eor_create(c->bucket_alloc, r);
>     APR_BRIGADE_INSERT_HEAD(bb, b);
> 
> @@ -477,7 +488,8 @@
>     ap_process_async_request(r);
> 
>     if (!c->data_in_input_filters) {
> -        bb = apr_brigade_create(c->pool, c->bucket_alloc);
> +        RETRIEVE_BRIGADE_FROM_POOL(bb, "ap_process_request_brigade", 
> +                                   c->pool, c->bucket_alloc);
>         b = apr_bucket_flush_create(c->bucket_alloc);
>         APR_BRIGADE_INSERT_HEAD(bb, b);
>         rv = ap_pass_brigade(c->output_filters, bb);
> 
> 
> Regards
> 
> Rüdiger