httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Yann Ylavic <ylavic....@gmail.com>
Subject Re: [Patch] Async write completion for the full connection filter stack
Date Wed, 10 Sep 2014 23:51:55 GMT
On Wed, Sep 10, 2014 at 6:07 PM, Graham Leggett <minfrin@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.

Mime
View raw message