Hi,
you may have noticed my commits on util_filter lately, which I didn't
propose/argument on dev@ first, so here I am...
My preference was to proceed like this (sequentially on each patch),
with an idea of where to go but not necessarily the words to explain
each part beforehand; I wish you didn't take (too much) offense for
that, and I can always revert if you don't like it finally...
Sorry for the long message by the way (you were warned :)
<context>
First this is only about trunk only code, namely the async write
completion improvements (started in r1706669) which introduced the
notion of pending filters with retained/setaside bucket brigades.
A few times ago I changed that code a bit by using an APR_RING instead
of an apr_hash_t to maintain the pending filters (with a deterministic
order, the same as the filters chain) so that flushing pending data
from MPM event always happens from outer most filters first.
This introduced a bug (PR 62668) which could have been addressed quite
simply (once reproduced/diagnosed) by only saving the "prev" filter
before running the current one in ap_filter_output_pending() loop.
But while fixing this, I noticed that we had a bigger issue with
ap_filter_t's lifetime for request filters: *f is destroyed with
*f->r, so ap_filter_output_pending() can't check f->bb after f is
called, so it can't check whether there are still pending data for f
to stop processing (its very role...).
So passing EOR downstream for ap_filter_output_pending() or any
request filter is a big pain since it can't touch *f afterward, not to
talk about the passed in brigade or temporary brigades it may use
which can also be destroyed at the same time.
We already discussed some aspects of this previously IIRC, and made
minimal fixes for some brigades (ap_reuse_brigade_from_pool) in
filters where this kind of issue was identified, but the issue with
request filters remained for *f.
</context>
So in r1839997, besides fixing PR 62668 as explained above, the goal
is to allocate filters (ap_filter_t) on c->pool in any case, and
recycle them in a ring to avoid leaks (since multiple request filters
are added/removed during the lifetime of a conn_rec).
But this requires more than the existing pending_{in,out}put_filters
context in conn_rec, so I defined an opaque ap_filter_conn_ctx,
attached to each connection, also containing the needed rings.
Follow up r1840002 is about when we should recycle the ap_filter_t's,
because if we do this at the time a filter removes itself from the
chain (i.e. ap_remove_{in,out}put_filter), another filter might reuse
the ap_filter_t before the former filter had a chance to return, thus
*f might end up being misinterpreted or corrupted (think of
ap_remove_output_filter(f) followed by ap_add_output_filter("some
filter") followed by ap_pass_brigade(f->next), f->next is not what one
expect it to be...).
So what ap_remove_output_filter() now does is recycle filters in
temporary ring (namely dead_filters) so that they can't be reused
until the new ap_filter_recycle() function is called, which simply
moves dead_filters to the reusable spare_filters ring.
Now ap_filter_recycle() can be called by both
ap_process_request_after_handler() and ap_filter_output_pending(),
where all the filters have returned, ap_add_*_filter() can't race, and
the more reusable filters for the next request the better.
I would have liked to find a callback where to do this automagically,
but couldn't think of one; any idea welcome because
ap_filter_recycle() looks like an implementation detail for me (maybe
at least we could not AP_DECLARE it).
Ideas welcome!
Next follow up r1840028 is optimizations of what was done in previous commits.
Finally r1840149, aimed to protect struct ap_filter_t from (ab)users,
namely f->pending, f->bb and f->deferred_pool are all util_filter use
only and hidding them (in an opaque struct ap_filter_private) allows
nice assertions (thus optimizations) in "util_filter.c", which would
break if the user touched anything in there.
This commit also adds ap_acquire_brigade() and ap_acquire_brigade() to
replace ap_reuse_brigade_from_pool() added not so long ago. Besides
being more efficient (ring's O(1) instead of O(1 + n/k) for pool's
apr_hash), I think acquire/release semantics allow to cleanup the
brigade at the right place (where ap_release_brigade() is called),
whereas ap_reuse_brigade_from_pool()'s cleanup on reuse can be
problematic.
Hth, comments/feedbacks/PRs welcome ;)
Regards,
Yann.
