You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Brian Pane <br...@gmail.com> on 2010/06/01 16:39:49 UTC
mod_deflate handling of empty initial brigade
In a filter module I'm writing, it's possible for the first pass
through the output filter to end up calling:
ap_pass_brigade(f->next, an_empty_brigade)
If mod_deflate's output filter appears later in the output filter
chain, bad things happen:
1. On the first trip through the output filter chain, mod_deflate just
passes the empty brigade through to the next filter:
/* Do nothing if asked to filter nothing. */
if (APR_BRIGADE_EMPTY(bb)) {
return ap_pass_brigade(f->next, bb);
}
2. Control eventually reaches core_output_filter, which sends the
response headers.
3. On a subsequent trip through the output filter chain, my module calls:
ap_pass_brigade(f->next, a non_empty_brigade)
4. Upon seeing the non empty brigade, mod_deflate's output filter does
all the initialization that it would normally do at the start of a
response. If the response is compressible, deflate_out_filter adds
"Content-Encoding: gzip" to the output headers. The output headers
have been sent already, though, so the Content-Encoding never gets
sent.
5. The client receives a compressed response without a Content-Encoding header.
I can prevent this by not calling ap_pass_brigade in my module until I
have the first non-empty brigade for the rest of the output filter
chain to work with.
I'm curious, though: which module is doing the wrong thing:
- My module, by sending an empty brigade through the rest of the
output filter chain prior to sending the first non-empty brigade?
- Or mod_deflate, by adding fields to the response header after
calling ap_pass_brigade?
Thanks,
-Brian
Re: mod_deflate handling of empty initial brigade
Posted by Eric Covener <co...@gmail.com>.
On Tue, Jun 1, 2010 at 10:39 AM, Brian Pane <br...@gmail.com> wrote:
> In a filter module I'm writing, it's possible for the first pass
> through the output filter to end up calling:
> ap_pass_brigade(f->next, an_empty_brigade)
>
> If mod_deflate's output filter appears later in the output filter
> chain, bad things happen:
> 1. On the first trip through the output filter chain, mod_deflate just
> passes the empty brigade through to the next filter:
> /* Do nothing if asked to filter nothing. */
> if (APR_BRIGADE_EMPTY(bb)) {
> return ap_pass_brigade(f->next, bb);
> }
> 2. Control eventually reaches core_output_filter, which sends the
> response headers.
> 3. On a subsequent trip through the output filter chain, my module calls:
> ap_pass_brigade(f->next, a non_empty_brigade)
> 4. Upon seeing the non empty brigade, mod_deflate's output filter does
> all the initialization that it would normally do at the start of a
> response. If the response is compressible, deflate_out_filter adds
> "Content-Encoding: gzip" to the output headers. The output headers
> have been sent already, though, so the Content-Encoding never gets
> sent.
> 5. The client receives a compressed response without a Content-Encoding header.
>
> I can prevent this by not calling ap_pass_brigade in my module until I
> have the first non-empty brigade for the rest of the output filter
> chain to work with.
>
> I'm curious, though: which module is doing the wrong thing:
> - My module, by sending an empty brigade through the rest of the
> output filter chain prior to sending the first non-empty brigade?
> - Or mod_deflate, by adding fields to the response header after
> calling ap_pass_brigade?
Would it do harm if the core output filter did the same check as
mod_deflate before committing the headers?
Seems like a filter could have just dropped the content of your first
brigade anyway.
--
Eric Covener
covener@gmail.com
Re: mod_deflate handling of empty initial brigade
Posted by Brian Pane <br...@gmail.com>.
On Tue, Jun 1, 2010 at 10:28 AM, Matthew Steele <md...@google.com> wrote:
> I went ahead and created a bug entry/patch to make the (trivial)
> change to mod_deflate to make it conform to the "Guide to writing
> output filters":
>
> https://issues.apache.org/bugzilla/show_bug.cgi?id=49369
>
> Brian, does this patch to mod_deflate fix your problem?
Yes. (I'm still going to proceed with the same defensive logic in my
own filter, though.)
Thanks,
-Brian
Re: mod_deflate handling of empty initial brigade
Posted by Matthew Steele <md...@google.com>.
I went ahead and created a bug entry/patch to make the (trivial)
change to mod_deflate to make it conform to the "Guide to writing
output filters":
https://issues.apache.org/bugzilla/show_bug.cgi?id=49369
Brian, does this patch to mod_deflate fix your problem?
Nick, does this change seem reasonable to you? (I tried to follow the
directions for patches on <http://httpd.apache.org/dev/patches.html>;
apologies if I got it wrong...)
Cheers,
-Matt
On Tue, Jun 1, 2010 at 11:54 AM, Nick Kew <ni...@webthing.com> wrote:
>
> On 1 Jun 2010, at 15:53, Bryan McQuade wrote:
>
>> According to this, mod_deflate should not pass the empty brigade
>> along, and your module also should not be passing and empty brigade to
>> mod_deflate.
>>
>> Eric, others, should we submit a patch to mod_deflate to change its
>> behavior to be consistent with the output filter guide?
>
> +1. Looks like a trivial fix if this is the only error path concerned.
>
> And an extra-robustness check in core output filter can't hurt (can it?)
>
> --
> Nick Kew
>
Re: mod_deflate handling of empty initial brigade
Posted by Nick Kew <ni...@webthing.com>.
On 1 Jun 2010, at 15:53, Bryan McQuade wrote:
> According to this, mod_deflate should not pass the empty brigade
> along, and your module also should not be passing and empty brigade to
> mod_deflate.
>
> Eric, others, should we submit a patch to mod_deflate to change its
> behavior to be consistent with the output filter guide?
+1. Looks like a trivial fix if this is the only error path concerned.
And an extra-robustness check in core output filter can't hurt (can it?)
--
Nick Kew
Re: mod_deflate handling of empty initial brigade
Posted by Matthew Steele <md...@google.com>.
On Tue, Jun 1, 2010 at 11:02 AM, "Plüm, Rüdiger, VF-Group"
<ru...@vodafone.com> wrote:
>> The guide to writing output filters says:
>>
>> https://httpd.apache.org/docs/trunk/developer/output-filters.h
>> tml#invocation
>>
>> "An output filter should never pass an empty brigade down the filter
>> chain. But, for good defensive programming, filters should be prepared
>> to accept an empty brigade, and do nothing."
>
> IMHO do nothing means the same thing as pass it down the chain. So I think
> mod_deflate does the correct thing here.
On the contrary, the link above specifically recommends putting the
following code snippet at the top of your output filter to suppress
the empty brigade (rather than pass it along to the next filter):
apr_status_t dummy_filter(ap_filter_t *f, apr_bucket_brigade *bb)
{
if (APR_BRIGADE_EMPTY(bb)) {
return APR_SUCCESS;
}
....
Does anyone know whether or not this is the right thing to do?
Cheers,
-Matt
Re: mod_deflate handling of empty initial brigade
Posted by to...@aol.com.
> Paul Fee wrote...
>
>> Bryan McQuade wrote:
>> Are there any cases where it's important for ap_pass_bridgade to pass
>> on an empty brigade? Doesn't sound like it, but since this is a core
>> library change I want to double check.
>
> When handling a CONNECT request, the response will have no body. In
> mod_proxy, the CONNECT handler currently skips most filters and writes via
> the connection filters. However there is a block of "#if 0" code which
> intends to send only a FLUSH bucket down the filter chain.
>
> That's not quite the case of an entirely empty brigade, but it seems close
> enough to warrant highlighting.
>
> Thanks,
> Paul
Can't think of anything else that might suffer from the sudden halt of
passing empty brigades down the chain ( unless some 3rd party module using
tandem-work filters is using them as 'place-markers' and actually EXPECTS
them to show up at some point ) but there MIGHT be at least 2 other considerations...
1. If 'ap_pass_brigade()' becomes hard-wired to simply always return SUCCESS
when it sees an empty brigade and it NEVER goes down the chain anymore...
then is there ANY possibility this could become a 'lost brigade'?
When would it ever get reused/released if it's not even making it
down to the core filter set anymore?
2. It doesn't 'feel' right that 'ap_pass_brigade()' should always return
SUCCESS when it starts throwing the empty brigades on the floor and does
NOT send them down the chain. That seems 'misleading'. The call did
NOT actually 'SUCCEED' because ap_pass_brigade() did NOT 'pass the brigade'.
Should there be some other ERROR/WARNING level return code instead
of always tossing back 'SUCCESS'? Might help with debugging later on
or give module writers more options for writing their own 'safety catch'
return code check(s) when calling ap_pass_brigade().
Yours
Kevin Kiley
Re: mod_deflate handling of empty initial brigade
Posted by Paul Fee <pf...@talk21.com>.
Bryan McQuade wrote:
> Are there any cases where it's important for ap_pass_bridgade to pass
> on an empty brigade? Doesn't sound like it, but since this is a core
> library change I want to double check.
When handling a CONNECT request, the response will have no body. In
mod_proxy, the CONNECT handler currently skips most filters and writes via
the connection filters. However there is a block of "#if 0" code which
intends to send only a FLUSH bucket down the filter chain.
That's not quite the case of an entirely empty brigade, but it seems close
enough to warrant highlighting.
Thanks,
Paul
Re: mod_deflate handling of empty initial brigade
Posted by Bryan McQuade <bm...@google.com>.
:) Great idea Brian.
I can submit a patch for this if it'd be helpful. Just let me know if
you'd like us to prepare a patch for this.
Are there any cases where it's important for ap_pass_bridgade to pass
on an empty brigade? Doesn't sound like it, but since this is a core
library change I want to double check.
Thanks,
Bryan
On Wed, Jun 2, 2010 at 7:24 AM, Nick Kew <ni...@webthing.com> wrote:
>
> On 2 Jun 2010, at 12:17, Brian Pane wrote:
>
>> Wait a minute...why not just change the implementation of
>> ap_pass_brigade so that it returns APR_SUCCESS immediately
>> when passed an empty brigade? That would solve the problem
>> globally.
>
> That's just too sensible for this thread :)
>
> --
> Nick Kew
>
Re: mod_deflate handling of empty initial brigade
Posted by Nick Kew <ni...@webthing.com>.
On 2 Jun 2010, at 12:17, Brian Pane wrote:
> Wait a minute...why not just change the implementation of
> ap_pass_brigade so that it returns APR_SUCCESS immediately
> when passed an empty brigade? That would solve the problem
> globally.
That's just too sensible for this thread :)
--
Nick Kew
Re: mod_deflate handling of empty initial brigade
Posted by Brian Pane <br...@gmail.com>.
On Wed, Jun 2, 2010 at 3:37 AM, Nick Kew <ni...@webthing.com> wrote:
>
> On 2 Jun 2010, at 10:49, Joe Orton wrote:
>
>> Maybe we should have a strict API here rather than a loose one:
>>
>> "filters MUST NOT pass an empty brigade down the filter chain."
>
> That's not really an API, it's a rule, and pretty-much unenforcable.
Wait a minute...why not just change the implementation of
ap_pass_brigade so that it returns APR_SUCCESS immediately
when passed an empty brigade? That would solve the problem
globally.
-Brian
Re: mod_deflate handling of empty initial brigade
Posted by Nick Kew <ni...@webthing.com>.
On 2 Jun 2010, at 10:49, Joe Orton wrote:
> Maybe we should have a strict API here rather than a loose one:
>
> "filters MUST NOT pass an empty brigade down the filter chain."
That's not really an API, it's a rule, and pretty-much unenforcable.
I recollect one problem that bit some of my modules when we moved
from 2.0 to 2.2 was that mod_proxy in 2.2 would (intermittently) send
an empty first brigade.
--
Nick Kew
RE: mod_deflate handling of empty initial brigade
Posted by "Plüm, Rüdiger, VF-Group" <ru...@vodafone.com>.
> -----Original Message-----
> From: Joe Orton
> Sent: Mittwoch, 2. Juni 2010 11:49
> To: dev@httpd.apache.org
> Subject: Re: mod_deflate handling of empty initial brigade
>
> On Tue, Jun 01, 2010 at 05:02:08PM +0200, "Plüm, Rüdiger,
> VF-Group" wrote:
> > > The guide to writing output filters says:
> > >
> > > https://httpd.apache.org/docs/trunk/developer/output-filters.h
> > > tml#invocation
> > >
> > > "An output filter should never pass an empty brigade down
> the filter
> > > chain. But, for good defensive programming, filters
> should be prepared
> > > to accept an empty brigade, and do nothing."
> >
> > IMHO do nothing means the same thing as pass it down the
> chain. So I think
> > mod_deflate does the correct thing here.
>
> The wording there is not entirely clear but I think the
> intent matches
> the example, as Matthew points out.
>
> On the general point, is there a reason to prefer having filters pass
> down empty brigades rather than ignore them and return
> success? Other
> than burning a few cycles, it does seem less safe since a
> less defensive
> downstream filter (e.g. the core output filter in this
> example!) may "do
> something" given the empty brigade.
>
> Maybe we should have a strict API here rather than a loose one:
>
> "filters MUST NOT pass an empty brigade down the filter chain."
+1 on clarifying the documentation. I am fine with the approach
if it is documented (and I think it is. I was only too lazy to read
it again until I got the link, hence my impression was wrong).
I think in general a filter should pass along all buckets it doesn't
care / know to handle because some downstream filter might be able
to do this. But I agree that empty brigades do not fall into this category
and I have no concrete example at hand where an empty brigade does
mean something to the downstream filter.
Anyway if this is the case we should rather create another meta bucket
type instead to achieve the same thing.
Regards
Rüdiger
Re: mod_deflate handling of empty initial brigade
Posted by Joe Orton <jo...@redhat.com>.
On Tue, Jun 01, 2010 at 05:02:08PM +0200, "Plüm, Rüdiger, VF-Group" wrote:
> > The guide to writing output filters says:
> >
> > https://httpd.apache.org/docs/trunk/developer/output-filters.h
> > tml#invocation
> >
> > "An output filter should never pass an empty brigade down the filter
> > chain. But, for good defensive programming, filters should be prepared
> > to accept an empty brigade, and do nothing."
>
> IMHO do nothing means the same thing as pass it down the chain. So I think
> mod_deflate does the correct thing here.
The wording there is not entirely clear but I think the intent matches
the example, as Matthew points out.
On the general point, is there a reason to prefer having filters pass
down empty brigades rather than ignore them and return success? Other
than burning a few cycles, it does seem less safe since a less defensive
downstream filter (e.g. the core output filter in this example!) may "do
something" given the empty brigade.
Maybe we should have a strict API here rather than a loose one:
"filters MUST NOT pass an empty brigade down the filter chain."
Regards, Joe
RE: mod_deflate handling of empty initial brigade
Posted by "Plüm, Rüdiger, VF-Group" <ru...@vodafone.com>.
> -----Original Message-----
> From: Bryan McQuade
> Sent: Dienstag, 1. Juni 2010 16:54
> To: dev@httpd.apache.org
> Cc: mdsteele@google.com
> Subject: Re: mod_deflate handling of empty initial brigade
>
> The guide to writing output filters says:
>
> https://httpd.apache.org/docs/trunk/developer/output-filters.h
> tml#invocation
>
> "An output filter should never pass an empty brigade down the filter
> chain. But, for good defensive programming, filters should be prepared
> to accept an empty brigade, and do nothing."
IMHO do nothing means the same thing as pass it down the chain. So I think
mod_deflate does the correct thing here.
Regards
Rüdiger
Re: mod_deflate handling of empty initial brigade
Posted by Bryan McQuade <bm...@google.com>.
The guide to writing output filters says:
https://httpd.apache.org/docs/trunk/developer/output-filters.html#invocation
"An output filter should never pass an empty brigade down the filter
chain. But, for good defensive programming, filters should be prepared
to accept an empty brigade, and do nothing."
According to this, mod_deflate should not pass the empty brigade
along, and your module also should not be passing and empty brigade to
mod_deflate.
Eric, others, should we submit a patch to mod_deflate to change its
behavior to be consistent with the output filter guide?
On Tue, Jun 1, 2010 at 10:39 AM, Brian Pane <br...@gmail.com> wrote:
> In a filter module I'm writing, it's possible for the first pass
> through the output filter to end up calling:
> ap_pass_brigade(f->next, an_empty_brigade)
>
> If mod_deflate's output filter appears later in the output filter
> chain, bad things happen:
> 1. On the first trip through the output filter chain, mod_deflate just
> passes the empty brigade through to the next filter:
> /* Do nothing if asked to filter nothing. */
> if (APR_BRIGADE_EMPTY(bb)) {
> return ap_pass_brigade(f->next, bb);
> }
> 2. Control eventually reaches core_output_filter, which sends the
> response headers.
> 3. On a subsequent trip through the output filter chain, my module calls:
> ap_pass_brigade(f->next, a non_empty_brigade)
> 4. Upon seeing the non empty brigade, mod_deflate's output filter does
> all the initialization that it would normally do at the start of a
> response. If the response is compressible, deflate_out_filter adds
> "Content-Encoding: gzip" to the output headers. The output headers
> have been sent already, though, so the Content-Encoding never gets
> sent.
> 5. The client receives a compressed response without a Content-Encoding header.
>
> I can prevent this by not calling ap_pass_brigade in my module until I
> have the first non-empty brigade for the rest of the output filter
> chain to work with.
>
> I'm curious, though: which module is doing the wrong thing:
> - My module, by sending an empty brigade through the rest of the
> output filter chain prior to sending the first non-empty brigade?
> - Or mod_deflate, by adding fields to the response header after
> calling ap_pass_brigade?
>
> Thanks,
> -Brian
>