httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Graham Leggett <minf...@sharp.fm>
Subject Re: [Patch] Async write completion for the full connection filter stack
Date Thu, 11 Sep 2014 15:20:45 GMT
On 11 Sep 2014, at 2:12 PM, Yann Ylavic <ylavic.dev@gmail.com> wrote:

> Again I missed something...
> 
> This code is indeed what I'd like ap_core_output_filter() (or any
> filter) to be able to do, but it won't work without a slight change in
> ap_filter_reinstate_brigade(), which is :
> 
> 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);
> 
>    *flush_upto = NULL;
> 
>    if ((buffered_bb != NULL) && !APR_BRIGADE_EMPTY(buffered_bb)) {
>        int flush = APR_BRIGADE_EMPTY(bb);
>        APR_BRIGADE_PREPEND(bb, buffered_bb);
>        f->c->data_in_output_filters--;
>        if (flush) {
>            return 1;
>        }
>    }
> 
>    ...
> }
> 
> The new thing is the "flush" check to return 1 immediatly when an
> empty bb is given.
> In this case the intent of the caller is to use what we have ("push it
> thru" would say Jim), we don't need to walk the buffered_bb again.
> 
> That's, I think, what the original code did in ap_core_output_filter()
> to send_brigade_nonblocking() immediatly in this case, and what the
> current patch misses.
> 
> Thoughts?

We need this yes - if we’ve been called with an empty brigade and we still have a buffered
brigade that is “too small to write” we want to just write that remaining brigade regardless
of it’s size, otherwise we spin.

I would still do the flush check anyway - the ap_filter_reinstate_brigade() has no idea what
happened outside this function, we may have flushed buckets, we may not have and setaside
those flush buckets for whatever reason, and we want the behaviour to remain predictable.

I’ve updated the patch to force the should_write if the original brigade was empty.

Regards,
Graham
—

Mime
View raw message