Hi Yann,
2018-07-19 21:41 GMT+02:00 Yann Ylavic <ylavic....@gmail.com>:
> On Thu, Jul 19, 2018 at 8:23 PM, Luca Toscano <toscano.l...@gmail.com>
> wrote:
> >
> > Yann, any idea?
>
> Looks like we missed the simplest case :/
>
> Index: modules/filters/mod_ratelimit.c
> ===================================================================
> --- modules/filters/mod_ratelimit.c (revision 1835556)
> +++ modules/filters/mod_ratelimit.c (working copy)
> @@ -208,7 +208,7 @@ rate_limit_filter(ap_filter_t *f, apr_bucket_briga
> ap_remove_output_filter(f);
> }
> else if (!APR_BUCKET_IS_FLUSH(e)) {
> - if (APR_BRIGADE_EMPTY(bb)) {
> + if (ctx->do_sleep && APR_BRIGADE_EMPTY(bb)) {
> /* Wait for more (or next call) */
> break;
> }
> _
>
I confirm that it works fine in my local dev environment, plus now gdb's
dump_brigade makes sense, but I am not sure that I have understood what was
wrong. In my tests, the headers were the first heap bucket reported by
dump_brigade in gdb (before your patch), so IIUC we were saving them to
holdingbb and then finally passing them via ap_pass_brigade. But I thought
that this was they way to go, what am I missing? I am pretty sure this is
basic filter knowledge but I have missed it up to now, if not present in
the docs already I'll make sure to add it.
Other mea culpa: I tested this change with a proxied scenario in which a
file was transferred from a proxied backed to the client via httpd, and I
didn't notice any problem. I also thought that the tests suite would have
caught a case like this one, but I was terribly wrong, so if possible I'll
try to add a test as follow up to make sure that basic proxied responses
are not mangled like this.
Really sorry for this mess, I should have been more careful before
committing. Lesson learned for the next time :(
Luca
