On Wed, Sep 10, 2014 at 6:07 PM, Graham Leggett <[email protected]> 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.
