I like this work a lot and als like reading this explanation and details
about intentions and rabbit holes encountered. I think there could
be more of it from everyone, including myself.
About these changes in particular: making io handling more and
easier async is excellent. I'd like to use that in h2.
+1 for ap_filter_private.
re ap_filter_recycle() would a new meta bucket help solving this?
Something to insert after an EOR?
Cheers, Stefan
> Am 05.09.2018 um 19:32 schrieb Yann Ylavic <ylavic....@gmail.com>:
>
> 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.
