Hi Cory,
Il giorno gio 2 ago 2018 alle ore 14:10 Yann Ylavic
<ylavic....@gmail.com> ha scritto:
>
> On Fri, Jul 27, 2018 at 6:56 PM, Cory McIntire <c...@cpanel.net> wrote:
> >
> > {quote}
> > While it will probably resolve the issues we saw, I’d be hesitant to
> > move forward with that patch as it modifies how all output filters
> > work with HEAD requests, this is too large a change, especially when
> > the bug(s) being addressesed are in a single module.
> >
> > I’d recommend making mod_ratelimit do the same “optimization” hack
> > that other modules for HEAD requests instead, and keep the surface
> > area for this bug fix isolated to mod_ratelimit only.
> >
> > Something like what mod_brotli does:
> >
> > if (r->header_only && r->bytes_sent) {
> > ap_remove_output_filter(f);
> > return ap_pass_brigade(f->next, bb);
> > }
> > {quote}
> >
> > If there are any further adjustments to this patch we’d be happy to
> > take a look, just let us know.
>
> Sorry, I missed that proposal.
>
> So, AIUI, the above plus moving the ratelimit filter after the "CHUNK"
> filter, right?
> Otherwise I don't see where we address the "header
> ratelimited/retained before chunking" case (regardless of
> HEAD/GET/..).
> IOW, the patch (limited to mod_ratelimit) would be something like:
>
> @@ -123,6 +123,13 @@ rate_limit_filter(ap_filter_t *f, apr_bucket_briga
> APR_BRIGADE_PREPEND(bb, ctx->holdingbb);
> }
>
> + if (f->r->header_only && f->r->bytes_sent) {
> + ap_remove_output_filter(f);
> + rv = ap_pass_brigade(f->next, bb);
> + apr_brigade_cleanup(bb);
> + return rv;
> + }
> +
> while (!APR_BRIGADE_EMPTY(bb)) {
> apr_bucket *e;
>
> @@ -327,7 +334,7 @@ static void register_hooks(apr_pool_t *p)
> ap_register_output_filter(RATE_LIMIT_FILTER_NAME, rate_limit_filter,
> - NULL, AP_FTYPE_PROTOCOL + 3);
> + NULL, AP_FTYPE_CONNECTION - 1);
>
> I think it does not work for the case where a HEAD response has a
> large header but "Content-Length: 0", such that rate_limit_filter()
> retains the last buckets but is never called to release them (i.e.
> EOS).
> Really we shouldn't have any (protocol) filter that eats EOS, this is
> the garantee that each request filter sees when it should terminate
> and bail out (that's also the purpose of
> ap_finalize_request_protocol() for instance).
>
> r1837130 looks like the right fix to me.
sorry for the late ping but I was on holidays. I know that you and
your team expressed some doubts about the fix for mod_ratelimit, but
it seems that Yann's fix is the correct way to go for me too. Any
thoughts? It would be great to reach some consensus within the
community before proceeding :)
Thanks!
Luca
