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.
Cheers,
Stefan
diff --git a/modules/http/byterange_filter.c b/modules/http/byterange_filter.c
index a79b7f7..92048ab 100644
--- a/modules/http/byterange_filter.c
+++ b/modules/http/byterange_filter.c
@@ -307,7 +307,7 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_byterange_filter(ap_filter_t *f,
APR_BRIGADE_INSERT_TAIL(bsend, e);
/* we're done with the original content - all of our data is in bsend. */
- apr_brigade_destroy(bb);
+ apr_brigade_cleanup(bb);
/* send our multipart output */
return ap_pass_brigade(f->next, bsend);
diff --git a/modules/http/http_filters.c b/modules/http/http_filters.c
index 3e96cb9..01ced24 100644
--- a/modules/http/http_filters.c
+++ b/modules/http/http_filters.c
@@ -1112,7 +1112,7 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_http_header_filter(ap_filter_t *f,
ctx = f->ctx = apr_pcalloc(r->pool, sizeof(header_filter_ctx));
}
else if (ctx->headers_sent) {
- apr_brigade_destroy(b);
+ apr_brigade_cleanup(b);
return OK;
}
}
@@ -1283,7 +1283,7 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_http_header_filter(ap_filter_t *f,
ap_pass_brigade(f->next, b2);
if (r->header_only) {
- apr_brigade_destroy(b);
+ apr_brigade_cleanup(b);
ctx->headers_sent = 1;
return OK;
}
diff --git a/server/core_filters.c b/server/core_filters.c
index f073765..eb206c1 100644
--- a/server/core_filters.c
+++ b/server/core_filters.c
@@ -316,7 +316,7 @@ int ap_core_input_filter(ap_filter_t *f, apr_bucket_brigade *b,
static void setaside_remaining_output(ap_filter_t *f,
core_output_filter_ctx_t *ctx,
apr_bucket_brigade *bb,
- int make_a_copy, conn_rec *c);
+ conn_rec *c);
static apr_status_t send_brigade_nonblocking(apr_socket_t *s,
apr_bucket_brigade *bb,
@@ -392,19 +392,21 @@ apr_status_t ap_core_output_filter(ap_filter_t *f, apr_bucket_brigade *new_bb)
}
}
+ if (new_bb != NULL) {
+ bb = new_bb;
+ }
+
if ((ctx->buffered_bb != NULL) &&
!APR_BRIGADE_EMPTY(ctx->buffered_bb)) {
- bb = ctx->buffered_bb;
- ctx->buffered_bb = NULL;
if (new_bb != NULL) {
- APR_BRIGADE_CONCAT(bb, new_bb);
+ APR_BRIGADE_PREPEND(bb, ctx->buffered_bb);
+ }
+ else {
+ bb = ctx->buffered_bb;
}
c->data_in_output_filters = 0;
}
- else if (new_bb != NULL) {
- bb = new_bb;
- }
- else {
+ else if (new_bb == NULL) {
return APR_SUCCESS;
}
@@ -444,7 +446,7 @@ apr_status_t ap_core_output_filter(ap_filter_t *f, apr_bucket_brigade *new_bb)
/* The client has aborted the connection */
c->aborted = 1;
}
- setaside_remaining_output(f, ctx, bb, 0, c);
+ setaside_remaining_output(f, ctx, bb, c);
return rv;
}
@@ -508,14 +519,14 @@ apr_status_t ap_core_output_filter(ap_filter_t *f, apr_bucket_brigade *new_bb)
}
}
- setaside_remaining_output(f, ctx, bb, 1, c);
+ setaside_remaining_output(f, ctx, bb, c);
return APR_SUCCESS;
}
static void setaside_remaining_output(ap_filter_t *f,
core_output_filter_ctx_t *ctx,
apr_bucket_brigade *bb,
- int make_a_copy, conn_rec *c)
+ conn_rec *c)
{
if (bb == NULL) {
return;
@@ -523,20 +534,14 @@ static void setaside_remaining_output(ap_filter_t *f,
remove_empty_buckets(bb);
if (!APR_BRIGADE_EMPTY(bb)) {
c->data_in_output_filters = 1;
- if (make_a_copy) {
+ if (bb != ctx->buffered_bb) {
/* XXX should this use a separate deferred write pool, like
* the original ap_core_output_filter?
*/
ap_save_brigade(f, &(ctx->buffered_bb), &bb, c->pool);
- apr_brigade_destroy(bb);
- }
- else {
- ctx->buffered_bb = bb;
+ apr_brigade_cleanup(bb);
}
}
- else {
- apr_brigade_destroy(bb);
- }
}
#ifndef APR_MAX_IOVEC_SIZE
diff --git a/modules/filters/mod_deflate.c b/modules/filters/mod_deflate.c
index 4451d77..2cbbf6b 100644
--- a/modules/filters/mod_deflate.c
+++ b/modules/filters/mod_deflate.c
@@ -1011,14 +1011,11 @@ static apr_status_t deflate_in_filter(ap_filter_t *f,
}
if (!APR_BRIGADE_EMPTY(ctx->proc_bb)) {
- apr_bucket_brigade *newbb;
-
/* May return APR_INCOMPLETE which is fine by us. */
apr_brigade_partition(ctx->proc_bb, readbytes, &bkt);
- newbb = apr_brigade_split(ctx->proc_bb, bkt);
APR_BRIGADE_CONCAT(bb, ctx->proc_bb);
- APR_BRIGADE_CONCAT(ctx->proc_bb, newbb);
+ apr_brigade_split_ex(bb, bkt, ctx->proc_bb);
}
return APR_SUCCESS;
diff --git a/modules/filters/mod_sed.c b/modules/filters/mod_sed.c
index 1fc72b0..9faf489 100644
--- a/modules/filters/mod_sed.c
+++ b/modules/filters/mod_sed.c
@@ -48,6 +48,7 @@ typedef struct sed_filter_ctxt
ap_filter_t *f;
request_rec *r;
apr_bucket_brigade *bb;
+ apr_bucket_brigade *bbinp;
char *outbuf;
char *curoutbuf;
int bufsize;
@@ -392,6 +393,7 @@ static apr_status_t sed_request_filter(ap_filter_t *f,
&sed_module);
sed_filter_ctxt *ctx = f->ctx;
apr_status_t status;
+ apr_bucket_brigade *bbinp;
sed_expr_config *sed_cfg = &cfg->input;
if (mode != AP_MODE_READBYTES) {
@@ -413,9 +415,12 @@ static apr_status_t sed_request_filter(ap_filter_t *f,
if (status != APR_SUCCESS)
return status;
ctx = f->ctx;
- ctx->bb = apr_brigade_create(f->r->pool, f->c->bucket_alloc);
+ ctx->bb = apr_brigade_create(f->r->pool, f->c->bucket_alloc);
+ ctx->bbinp = apr_brigade_create(f->r->pool, f->c->bucket_alloc);
}
+ bbinp = ctx->bbinp;
+
/* Here is the logic :
* Read the readbytes data from next level fiter into bbinp. Loop through
* the buckets in bbinp and read the data from buckets and invoke
@@ -435,11 +440,9 @@ static apr_status_t sed_request_filter(ap_filter_t *f,
* 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 @@ static apr_status_t sed_request_filter(ap_filter_t *f,
}
}
apr_brigade_cleanup(bbinp);
- apr_brigade_destroy(bbinp);
}
if (!APR_BRIGADE_EMPTY(ctx->bb)) {
- apr_bucket_brigade *newbb = NULL;
apr_bucket *b = NULL;
/* This may return APR_INCOMPLETE which should be fine */
apr_brigade_partition(ctx->bb, readbytes, &b);
- newbb = apr_brigade_split(ctx->bb, b);
APR_BRIGADE_CONCAT(bb, ctx->bb);
- APR_BRIGADE_CONCAT(ctx->bb, newbb);
+ apr_brigade_split_ex(bb, b, ctx->bb);
}
return APR_SUCCESS;
}
diff --git a/modules/http/chunk_filter.c b/modules/http/chunk_filter.c
index a8ce0d0..51544d4 100644
--- a/modules/http/chunk_filter.c
+++ b/modules/http/chunk_filter.c
@@ -49,11 +49,11 @@ apr_status_t ap_http_chunk_filter(ap_filter_t *f, apr_bucket_brigade *b)
#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 @@ apr_status_t ap_http_chunk_filter(ap_filter_t *f, apr_bucket_brigade *b)
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 @@ apr_status_t ap_http_chunk_filter(ap_filter_t *f, apr_bucket_brigade *b)
* 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 {
diff --git a/server/protocol.c b/server/protocol.c
index 3491bdd..705559c 100644
--- a/server/protocol.c
+++ b/server/protocol.c
@@ -1239,6 +1239,7 @@ struct content_length_ctx {
* 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 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_content_length_filter(
if (!ctx) {
f->ctx = ctx = apr_palloc(r->pool, sizeof(*ctx));
ctx->data_sent = 0;
+ ctx->tmpbb = apr_brigade_create(r->connection->pool,
+ r->connection->bucket_alloc);
}
/* Loop through this set of buckets to compute their length
@@ -1288,16 +1291,15 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_content_length_filter(
* do a blocking read on the next batch.
*/
if (e != APR_BRIGADE_FIRST(b)) {
- apr_bucket_brigade *split = apr_brigade_split(b, e);
+ apr_brigade_split_ex(b, e, ctx->tmpbb);
apr_bucket *flush = apr_bucket_flush_create(r->connection->bucket_alloc);
APR_BRIGADE_INSERT_TAIL(b, flush);
rv = ap_pass_brigade(f->next, b);
if (rv != APR_SUCCESS || f->c->aborted) {
- apr_brigade_destroy(split);
return rv;
}
- b = split;
+ APR_BRIGADE_CONCAT(b, ctx->tmpbb);
e = APR_BRIGADE_FIRST(b);
ctx->data_sent = 1;
@@ -1390,6 +1392,7 @@ AP_DECLARE(size_t) ap_send_mmap(apr_mmap_t *mm, request_rec *r, size_t offset,
typedef struct {
apr_bucket_brigade *bb;
+ apr_bucket_brigade *tmpbb;
} old_write_filter_ctx;
AP_CORE_DECLARE_NONSTD(apr_status_t) ap_old_write_filter(
@@ -1410,16 +1413,11 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_old_write_filter(
return ap_pass_brigade(f->next, bb);
}
-static apr_status_t buffer_output(request_rec *r,
- const char *str, apr_size_t len)
+static ap_filter_t *insert_old_write_filter(request_rec *r)
{
- conn_rec *c = r->connection;
ap_filter_t *f;
old_write_filter_ctx *ctx;
- if (len == 0)
- return APR_SUCCESS;
-
/* future optimization: record some flags in the request_rec to
* say whether we've added our filter, and whether it is first.
*/
@@ -1433,24 +1431,38 @@ static apr_status_t buffer_output(request_rec *r,
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);
}
- /* grab the context from our filter */
- ctx = r->output_filters->ctx;
-
if (ctx->bb == NULL) {
ctx->bb = apr_brigade_create(r->pool, c->bucket_alloc);
}
@@ -1605,13 +1617,17 @@ AP_DECLARE_NONSTD(int) ap_rvputs(request_rec *r, ...)
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;
+
+ f = insert_old_write_filter(r);
+ ctx = f->ctx;
- bb = apr_brigade_create(r->pool, c->bucket_alloc);
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)
return -1;
return 0;
