On 11 Sep 2014, at 1:51 AM, Yann Ylavic <[email protected]> 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.
> + 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.
httpd-async-fullstack-ssl2.patch
Description: Binary data
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.
> + }
> + 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.
> 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?
True. I’ve made flush_upto mandatory.
>
> + }
> + }
> +
> [...]
> +
> + 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?
It should. Fixed.
Regards,
Graham
—
httpd-async-fullstack-ssl2.patch
Description: Binary data
