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
