Hello Luca, Sorry for late reply, we’re digging into and testing this version of the patch now, will update this week with more info, preliminary results seem to be positive however.
Thanks,
Cory McIntire
Release Manager - EasyApache
cPanel, Inc.
> On Aug 23, 2018, at 11:25 AM, Luca Toscano <toscano.l...@gmail.com> wrote:
>
> 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
smime.p7s
Description: S/MIME cryptographic signature
