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 Thu, 11 Sep 2014 07:51:49 GMT
On Thu, Sep 11, 2014 at 6:39 AM, Graham Leggett <minfrin@sharp.fm> wrote:
> On 11 Sep 2014, at 1:51 AM, Yann Ylavic <ylavic.dev@gmail.com> wrote:
>
>> +{
>> +    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).
>
> The test doesn’t seem to be needed no, that’s legacy.

Indeed, but wasn't part of the API, now it is...

>
>> +    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);
>
> Hmmm… this came from the original code.
>
>
> We can’t set buffered_bb to NULL unless we are sure we created buffered_bb, and this
isn’t necessarily the case. In the core filter, buffered_bb is created when the connection
is created.

If we or any filter has created the brigade on *deferred_write_pool
(or an ancestor, I'm only talking about this case), it is dead now
(unaccessible), and IMO should be marked as so.
In the case the caller let the function handle the brigade (as we do),
using NULL the first time only, (s)he expects to not crash whenever
(s)he come back with it after a clear (which (s)he isn't supposed to
know about).

Another solution would be to not require a pool at all, something like :

AP_DECLARE(apr_status_t) ap_filter_setaside_brigade(ap_filter_t *f,
                                                    apr_bucket_brigade
**buffered_bb,
                                                    apr_bucket_brigade *bb)
{
    if (!APR_BRIGADE_EMPTY(bb)) {
        if (APR_BRIGADE_EMPTY((*buffered_bb))) {
            f->c->data_in_output_filters++;
        }
        if (bb != *buffered_bb) {
            return ap_save_brigade(f, buffered_bb, &bb, f->c->pool);
        }
    }
    else if (*buffered_bb) {
        /*
         * There are no more requests in the pipeline. We can just clear the
         * brigade.
         */
        apr_brigade_cleanup(*buffered_bb);
    }
    return APR_SUCCESS;
}

The core and mod_ssl deferred_write_pool won't be needed anymore.

>
>> +    }
>> +    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).
>
> I don’t follow - that means something different as I’m reading it. We must only signal
that we have less data in the output filters if we actually originally had data in the output
filters, ie buffered_bb existed and wasn’t empty.

You're right, I misread ap_core_output_filter() where I thought
ap_filter_reinstate_brigade() wasn't called when
APR_BRIGADE_EMPTY(bb).

Since ap_filter_reinstate_brigade() can be called with an empty bb,
ap_core_output_filter() can (still) be simplified though, ie :

    should_write = ap_filter_reinstate_brigade(f, ctx->buffered_bb, bb,
            &flush_upto);
    if (APR_BRIGADE_EMPTY(bb)) {
        return APR_SUCCESS;
    }

    if (flush_upto != NULL) {
        ...
    }

    if (should_write) {
        ...
    }

flush_upto will always be NULL with an empty bb.

Regards,
Yann.

Mime
View raw message