You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Ruediger Pluem <rp...@apache.org> on 2006/06/22 01:38:45 UTC
Re: svn commit: r416165 - /httpd/httpd/trunk/modules/filters/mod_deflate.c
On 06/22/2006 01:16 AM, wrote:
> Author: niq
> Date: Wed Jun 21 16:16:47 2006
> New Revision: 416165
>
> URL: http://svn.apache.org/viewvc?rev=416165&view=rev
> Log:
> PR#39854
> Remove bogus code that chokes on flush buckets
>
>
> if (APR_BUCKET_IS_FLUSH(bkt)) {
> - apr_bucket *tmp_heap;
> - zRC = inflate(&(ctx->stream), Z_SYNC_FLUSH);
> - if (zRC != Z_OK) {
> - ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
> - "Inflate error %d on flush", zRC);
> - inflateEnd(&ctx->stream);
> - return APR_EGENERAL;
> - }
> -
> - ctx->stream.next_out = ctx->buffer;
> - len = c->bufferSize - ctx->stream.avail_out;
> -
> - ctx->crc = crc32(ctx->crc, (const Bytef *)ctx->buffer, len);
> - tmp_heap = apr_bucket_heap_create((char *)ctx->buffer, len,
> - NULL, f->c->bucket_alloc);
> - APR_BRIGADE_INSERT_TAIL(ctx->proc_bb, tmp_heap);
> - ctx->stream.avail_out = c->bufferSize;
> -
> - /* Move everything to the returning brigade. */
> - APR_BUCKET_REMOVE(bkt);
> - break;
> + continue;
Is this correct? In fact you ignore flush buckets now. Shouldn't you add the flush bucket to
ctx->proc_bb, pass it up the chain, clean ctx->proc_bb afterwards and break if the result is not
ok?
Regards
Rüdiger
Re: svn commit: r416165 - /httpd/httpd/trunk/modules/filters/mod_deflate.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 06/22/2006 02:08 AM, Nick Kew wrote:
> On Thursday 22 June 2006 00:38, Ruediger Pluem wrote:
>
>
>>Is this correct? In fact you ignore flush buckets now.
>
>
> Yes, this ignores flush buckets - which is better than choking on them.
Yes, of course.
> I'm not aware of this being a problem worth worrying about unduly,
> but feel free to correct me.
Maybe there is reason to worry. See my possible real world example below.
>> Shouldn't you add
>>the flush bucket to ctx->proc_bb, pass it up the chain, clean ctx->proc_bb
>>afterwards and break if the result is not ok?
>
>
> In principle, yes. Except for the break. But needs delving deeper
> into libz than I feel able to do right now, to figure out if there's going
> to be an issue of orphaned data.
I guess I did not described it correctly. I meant to say that in the case of
an error reported from the chain we should return this error to the calling filter
immediately. Of course it might be needed to clean up some zlib data structures before
to avoid leaving orphaned data
>
> I believe the real life flush bucket causing the error report is coming
> before the compressed data.
>
A possible real world case: A slow application on a Tomcat that is connected via
mod_proxy_ajp (flushing enabled) that delivers compressed content that should be
inflated by mod_deflate.
BTW: I noticed another possible issue:
What about the case where we have a file bucket of a compressed file that expands to
a very large file? As far as I understand the inflate output filter it captures the whole
inflated brigade until it passes it up the chain. So in this case we could use up
a lot of memory. Wouldn't it be better to pass every single heap bucket containing the inflated
data up the chain and let the core output filters decide on possible buffering before sending
data on the wire?
Regards
Rüdiger
Re: svn commit: r416165 - /httpd/httpd/trunk/modules/filters/mod_deflate.c
Posted by Nick Kew <ni...@webthing.com>.
On Thursday 22 June 2006 00:38, Ruediger Pluem wrote:
> Is this correct? In fact you ignore flush buckets now.
Yes, this ignores flush buckets - which is better than choking on them.
I'm not aware of this being a problem worth worrying about unduly,
but feel free to correct me.
> Shouldn't you add
> the flush bucket to ctx->proc_bb, pass it up the chain, clean ctx->proc_bb
> afterwards and break if the result is not ok?
In principle, yes. Except for the break. But needs delving deeper
into libz than I feel able to do right now, to figure out if there's going
to be an issue of orphaned data.
I believe the real life flush bucket causing the error report is coming
before the compressed data.
--
Nick Kew