On Wed, Sep 10, 2014 at 6:07 PM, Graham Leggett <minf...@sharp.fm> wrote: > > The attached patch does that. >
Hi Graham, nice patch, thank you. A few comments below... +AP_DECLARE(apr_status_t) ap_filter_setaside_brigade(ap_filter_t *f, + apr_bucket_brigade **buffered_bb, + apr_pool_t **deferred_write_pool, + apr_bucket_brigade *bb) +{ + if (bb == NULL) { Is this test really needed? This does not seem to be possible with you patch, and now this is part of the API, a crash if often better in this case (like for any other ap_filter_*() function which is given a NULL bb, or the ap[r] ones in general which tend -sanely- to crash where the code is faulty). + return APR_SUCCESS; + } + if (!APR_BRIGADE_EMPTY(bb)) { + if (APR_BRIGADE_EMPTY((*buffered_bb))) { + f->c->data_in_output_filters++; + } + if (bb != *buffered_bb) { + if (!(*deferred_write_pool)) { + apr_pool_create(deferred_write_pool, f->c->pool); + apr_pool_tag(*deferred_write_pool, "deferred_write"); + } + return ap_save_brigade(f, buffered_bb, &bb, + *deferred_write_pool); + } + } + else if (*deferred_write_pool) { + /* + * There are no more requests in the pipeline. We can just clear the + * pool. + */ Shouldn't *buffered_bb be set to NULL here when *deferred_write_pool == (*buffered_bb)->p, or more generally apr_pool_is_ancestor(*deferred_write_pool, (*buffered_bb)->p). We can't leave a dangling pointer in this case. + apr_pool_clear(*deferred_write_pool); + } + return APR_SUCCESS; +} + +AP_DECLARE(int) ap_filter_reinstate_brigade(ap_filter_t *f, + apr_bucket_brigade *buffered_bb, + apr_bucket_brigade *bb, + apr_bucket **flush_upto) +{ + apr_bucket *bucket, *next; + apr_size_t bytes_in_brigade, non_file_bytes_in_brigade; + int eor_buckets_in_brigade, morphing_bucket_in_brigade; + int loglevel = ap_get_conn_module_loglevel(f->c, APLOG_MODULE_INDEX); + + if ((buffered_bb != NULL) && !APR_BRIGADE_EMPTY(buffered_bb)) { + APR_BRIGADE_PREPEND(bb, buffered_bb); + f->c->data_in_output_filters--; Maybe we could use instead : if (!APR_BRIGADE_EMPTY(bb)) { f->c->data_in_output_filters--; } APR_BRIGADE_PREPEND(bb, buffered_bb); so that it remains symetric with ap_filter_setaside_brigade() above (which increments c->data_in_output_filters). It also has the advantage to simplify the calling code in ap_core_output_filter(), where we could simply do : if ((ctx->buffered_bb != NULL) && !APR_BRIGADE_EMPTY(ctx->buffered_bb)) { should_write = ap_filter_reinstate_brigade(f, ctx->buffered_bb, bb, &flush_upto); } else if (APR_BRIGADE_EMPTY(bb)) { return APR_SUCCESS; } if (flush_upto != NULL) { ... } if (should_write) { ... } removing the "empty" variable danse and the duplicated send_brigade_nonblocking() code. + } + [...] + + bytes_in_brigade = 0; + non_file_bytes_in_brigade = 0; + eor_buckets_in_brigade = 0; + morphing_bucket_in_brigade = 0; + + for (bucket = APR_BRIGADE_FIRST(bb); bucket != APR_BRIGADE_SENTINEL(bb); + bucket = next) { + next = APR_BUCKET_NEXT(bucket); + [...] + + if (APR_BUCKET_IS_FLUSH(bucket) + || non_file_bytes_in_brigade >= THRESHOLD_MAX_BUFFER + || morphing_bucket_in_brigade + || eor_buckets_in_brigade > MAX_REQUESTS_IN_PIPELINE) { + /* this segment of the brigade MUST be sent before returning. */ + [...] + /* + * Defer the actual blocking write to avoid doing many writes. + */ + if (flush_upto) { + *flush_upto = next; + } + + bytes_in_brigade = 0; + non_file_bytes_in_brigade = 0; + eor_buckets_in_brigade = 0; + morphing_bucket_in_brigade = 0; By resetting the counters here, and mainly bytes_in_brigade, the value returned by the function indicates whether we should write (nonblocking) the bytes *after* the flush_upto bucket, ie. not including the write (blocking) we should do for the bytes *before* this bucket. This is not clear in the description of the function (util_filter.h), particularly because flush_upto can be passed NULL. Does it make sense to call it without flush_upto? How would the caller know about the preceding bytes? + } + } + [...] + + return bytes_in_brigade >= THRESHOLD_MIN_WRITE; +} + +AP_DECLARE(apr_status_t) ap_filter_should_yield(ap_filter_t *f) +{ Shouldn't this function return an int? + return f->c->data_in_output_filters; +} + Regards, Yann.