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;