Hi Luca, We’ve tested this in house and it does seem to resolve the issues from the previous patch. Looks good to us. :)
Thanks,
Cory
> On Aug 27, 2018, at 8:50 AM, Cory McIntire <c...@cpanel.net> wrote:
>
> 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
