httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joe Orton <>
Subject Re: [RFC] Guide to writing output filters
Date Mon, 19 Mar 2007 14:06:04 GMT
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

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 
problem and PR 38014)

> - 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.  
It's almost always safer to use _cleanup.

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.
> - Filtering brigades:
>   - Although this is not my opinion I know that there had been some discussions in the
>     past that no examples should be given on how you should *not* do things.

I was wary of doing this too, but it's such a common mistake that I 
thought it to explain explicitly why it's bad.

> - Maintaining state:
>   - f->ctx = state = apr_palloc(sizeof(*state), f->r->pool); instead of
>     f->ctx = state = apr_palloc(sizeof *state, f->r->pool); ?

I vaguely prefer sizeof without the brackets where valid ;)

>   - IMHO ap_save_brigade can operate on an existing brigade. So this can
>     be a brigade created once per invocation. I agree with the warning that
>     especially on PIPE buckets ap_save_brigade can consume quite a lot of
>     memory. Plus ap_save_brigade does a blocking read on the bucket.

Good points, I'll add some text.


View raw message