httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ruediger Pluem <rpl...@apache.org>
Subject Re: [RFC] Guide to writing output filters
Date Mon, 19 Mar 2007 21:15:03 GMT


On 03/19/2007 03:06 PM, Joe Orton wrote:
> Thanks a lot for the review!
> 
> On Sat, Mar 17, 2007 at 04:30:24PM +0100, Ruediger Pluem wrote:
> 
>>Some comments from my side:
>>
>>- Passing empty brigades: While I agree that a filter should never create
>>  an empty brigade and pass it down the chain, I think it actually should
>>  pass an empty brigade down the chain if it gets one passed instead of
>>  simply returning. For reasons why see
>>
>>http://issues.apache.org/bugzilla/show_bug.cgi?id=40090
>>http://mail-archives.apache.org/mod_mbox/httpd-dev/200607.mbox/%3c44C3DFDA.9080306@apache.org%3e
> 
> 
> This is interesting, but I don't really understand how the problem 
> described happens.  How is ap_finalize_request_protocol() getting called 
> before a response has been sent?  (there is some symmetry between that 

Once we detect that we have a fresh content entity in the cache, the quick handler
of mod_cache starts the filter chain by calling

ap_pass_brigade(r->output_filters, out);

where out is an empty brigade created by the quick handler. The brigade gets filled
with the cached content by the CACHE_OUT filter down in the chain.
In the original code path and PR configuration mod_deflate was the first
filter in r->output_filters which caused the filter chain to return immediately and before
it reached the CACHE_OUT filter (mod_deflate detected an empty brigade and returned
APR_SUCCESS). This caused the quick handler of mod_cache to be left with OK, which triggers
ap_finalize_request_protocol. The correct fix in this situation of cause was to remove all
filters from r->output_filters until we hit mod_cache's CACHE_OUT filter, because this
is the position in the filter chain where the content went to the cache during caching.

> problem and PR 38014)

I think PR 38014 is somewhat different. The ap_http_filter simply was not completely aware
that it could be called in two situations:

- During request processing.
- After request processing in order to cleanup the connection.


> 
> 
>>- apr_brigade_destroy: I think the danger about using apr_brigade_destroy is
>>  to call it and *reuse* this brigade afterwards, because in this case the
>>  removed pool cleanup can cause a memory leak. 
> 
> 
> Right - and that should be true of every brigade which a filter uses 
> (the passed-in brigade should not be destroyed; any brigades the filter 
> itself creates *must* be long-lived and re-used).  So there's really no 
> case in which to recommend use of apr_brigade_destroy() for a filter.  

The only possible one from my perspective is on a brigade that was created
by the filter as long lived state brigade in the case that the filter either
sees an EOS bucket or decides to remove itself from the filter chain for some
other reason. But even in this case _cleanup is sufficient. We can only save
walking thru the brigades pool cleanup once the pool used for its creation
gets destroyed.

> It's almost always safer to use _cleanup.

Agreed. Seeing an apr_brigade_destroy in a filter should ring an alarm bell.

> 
> I'll add a note about using _cleanup() to this section.
> 
> 
>>- Procesing buckets: I think with mmap enabled a file bucket will morph into
>>  a mmap bucket and the remaining file bucket. I think the heap bucket will
>>  only be created if mmap is turned off. But I agree that this possibly introduces
>>  too much complexity to the example and distracts the reader from the important
>>  point.
> 
> 
> Yeah.  It's an implementation detail, and the risk is that if it gets 
> documented, people will rely on it somehow.

Why can't you rely on this? Isn't that the publicly defined behaviour of a file bucket
when MMAP is enabled?

Regards

RĂ¼diger

Mime
View raw message