Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@httpd.apache.org
Message-ID: <4ABA8C03.7070406@apache.org>
Date: Wed, 23 Sep 2009 22:58:43 +0200
From: Ruediger Pluem <rpluem@apache.org>
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US;
 rv:1.8.1.23) Gecko/20090823 SeaMonkey/1.1.18
MIME-Version: 1.0
To: dev@httpd.apache.org
Subject: Re: Memory usage, core output filter, and apr_brigade_destroy
References: <Pine.LNX.4.64.0909131234570.28922@eru.sfritsch.de>
 <4AACEA2A.2040402@apache.org> <200909131909.38031.sf@sfritsch.de>
 <200909222119.20991.sf@sfritsch.de>
In-Reply-To: <200909222119.20991.sf@sfritsch.de>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 8bit


On 09/22/2009 09:19 PM, Stefan Fritsch wrote:
> On Sunday 13 September 2009, Stefan Fritsch wrote:
>> On Sunday 13 September 2009, Ruediger Pluem wrote:
>>> On 09/13/2009 01:11 PM, Stefan Fritsch wrote:
>>>> http://httpd.apache.org/docs/trunk/developer/output-filters.htm
>>>> l recommends to reuse bucket brigades and to not use
>>>> apr_brigade_destroy. However, both in 2.2 and in trunk, the
>>>> core output filter sometimes calls apr_brigade_destroy on
>>>> brigades that it has received down the chain from earlier
>>>> output filters. Is this not bound to cause problems since the
>>>> brigade's pool cleanup is then removed but the brigade is still
>>>> reused later on?
>>> It could be. But the questions is if it is really reused later
>>> on.
>> Since this is the recommended design for output filters, I am sure
>>  it can happen.
> 
> I have attached two patches:
> 
> In the first, I change (hopefully) all places to not 
> apr_brigade_destroy brigades that have been passed down the filter 
> chain. Especially the core_output_filter change needs some review.
> 
> In the second, I have changed all occurences of apr_brigade_split to 
> apr_brigade_split_ex and I have made some more changes where bucket 
> brigades can be reused.
> 
> The test suite shows no regressions.


Index: server/protocol.c
===================================================================
--- server/protocol.c   (Revision 818232)
+++ server/protocol.c   (Arbeitskopie)
@@ -1239,6 +1239,7 @@
                      * least one bucket on to the next output filter
                      * for this request
                      */
+    apr_bucket_brigade *tmpbb;
 };

 /* This filter computes the content length, but it also computes the number
@@ -1259,6 +1260,8 @@
     if (!ctx) {
         f->ctx = ctx = apr_palloc(r->pool, sizeof(*ctx));
         ctx->data_sent = 0;
+        ctx->tmpbb = apr_brigade_create(r->connection->pool,

This should be r->pool instead. The lifetime of ctx itself is limited to r->pool

+                                        r->connection->bucket_alloc);
     }


@@ -1433,24 +1431,38 @@
     if (f == NULL) {
         /* our filter hasn't been added yet */
         ctx = apr_pcalloc(r->pool, sizeof(*ctx));
+        ctx->tmpbb = apr_brigade_create(r->pool, r->connection->bucket_alloc);
+
         ap_add_output_filter("OLD_WRITE", ctx, r, r->connection);
         f = r->output_filters;
     }

+    return f;
+}
+
+static apr_status_t buffer_output(request_rec *r,
+                                  const char *str, apr_size_t len)
+{
+    conn_rec *c = r->connection;
+    ap_filter_t *f;
+    old_write_filter_ctx *ctx;
+
+    if (len == 0)
+        return APR_SUCCESS;
+
+    f = insert_old_write_filter(r);
+    ctx = f->ctx;
+
     /* if the first filter is not our buffering filter, then we have to
      * deliver the content through the normal filter chain
      */
     if (f != r->output_filters) {
-        apr_bucket_brigade *bb = apr_brigade_create(r->pool, c->bucket_alloc);
         apr_bucket *b = apr_bucket_transient_create(str, len, c->bucket_alloc);
-        APR_BRIGADE_INSERT_TAIL(bb, b);
+        APR_BRIGADE_INSERT_TAIL(ctx->tmpbb, b);

-        return ap_pass_brigade(r->output_filters, bb);
+        return ap_pass_brigade(r->output_filters, ctx->tmpbb);

To be on the save side I think an apr_brigade_cleanup(ctx->tmpbb) is due
before doing the return, just in case a filter did not consume the buckets
properly.

     }
@@ -1605,13 +1617,17 @@
 AP_DECLARE(int) ap_rflush(request_rec *r)
 {
     conn_rec *c = r->connection;
-    apr_bucket_brigade *bb;
     apr_bucket *b;
+    ap_filter_t *f;
+    old_write_filter_ctx *ctx;

-    bb = apr_brigade_create(r->pool, c->bucket_alloc);
+    f = insert_old_write_filter(r);
+    ctx = f->ctx;
+
     b = apr_bucket_flush_create(c->bucket_alloc);
-    APR_BRIGADE_INSERT_TAIL(bb, b);
-    if (ap_pass_brigade(r->output_filters, bb) != APR_SUCCESS)
+    APR_BRIGADE_INSERT_TAIL(ctx->tmpbb, b);
+
+    if (ap_pass_brigade(r->output_filters, ctx->tmpbb) != APR_SUCCESS)

Same as above: We should call apr_brigade_cleanup(ctx->tmpbb) to be on the save side.

         return -1;

     return 0;


--- modules/http/chunk_filter.c (Revision 818232)
+++ modules/http/chunk_filter.c (Arbeitskopie)
@@ -49,11 +49,11 @@
 #define ASCII_CRLF  "\015\012"
 #define ASCII_ZERO  "\060"
     conn_rec *c = f->r->connection;
-    apr_bucket_brigade *more;
+    apr_bucket_brigade *more, *tmp;
     apr_bucket *e;
     apr_status_t rv;

-    for (more = NULL; b; b = more, more = NULL) {
+    for (more = tmp = NULL; b; tmp = b, b = more, more = NULL) {
         apr_off_t bytes = 0;
         apr_bucket *eos = NULL;
         apr_bucket *flush = NULL;
@@ -85,7 +85,8 @@
             if (APR_BUCKET_IS_FLUSH(e)) {
                 flush = e;
                 if (e != APR_BRIGADE_LAST(b)) {
-                    more = apr_brigade_split(b, APR_BUCKET_NEXT(e));
+                    more = apr_brigade_split_ex(b, APR_BUCKET_NEXT(e), tmp);
+                    tmp = NULL;
                 }
                 break;
             }
@@ -105,7 +106,8 @@
                      * block so we pass down what we have so far.
                      */
                     bytes += len;
-                    more = apr_brigade_split(b, APR_BUCKET_NEXT(e));
+                    more = apr_brigade_split_ex(b, APR_BUCKET_NEXT(e), tmp);
+                    tmp = NULL;
                     break;
                 }
                 else {

What is the point here? tmp is always NULL when passed to apr_brigade_split_ex
so apr_brigade_split_ex == apr_brigade_split

--- modules/filters/mod_sed.c   (Revision 818232)
+++ modules/filters/mod_sed.c   (Arbeitskopie)
@@ -435,11 +440,9 @@
      * the question is where to add it?
      */
     while (APR_BRIGADE_EMPTY(ctx->bb)) {
-        apr_bucket_brigade *bbinp;
         apr_bucket *b;

         /* read the bytes from next level filter */
-        bbinp = apr_brigade_create(f->r->pool, f->c->bucket_alloc);
         status = ap_get_brigade(f->next, bbinp, mode, block, readbytes);
         if (status != APR_SUCCESS) {
             return status;
@@ -470,19 +473,16 @@
             }
         }
         apr_brigade_cleanup(bbinp);

This one should be moved *before* the ap_get_brigade. It ensures that we always
call ap_get_brigade with an empty brigade.

-        apr_brigade_destroy(bbinp);
     }


Regards

R�diger