[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [users@httpd] mod_ratelimit working by steps ?

2018-06-05 8:19 GMT+02:00 Luca Toscano <toscano.luca@xxxxxxxxx>:

2018-06-04 16:22 GMT+02:00 Luca Toscano <toscano.luca@xxxxxxxxx>:
Hi Yann!

2018-06-04 15:59 GMT+02:00 Yann Ylavic <ylavic.dev@xxxxxxxxx>:
Hi Luca,

On Mon, Jun 4, 2018 at 11:23 AM, Luca Toscano <toscano.luca@xxxxxxxxx> wrote:
> To keep archives happy: opened
> https://bz.apache.org/bugzilla/show_bug.cgi?id=62362 and added a patch in
> there, if anybody wants to review it and give me suggestions I'd be happy :)

The semantic of tmpbb is not very clear in your patch, it's both the
brigade where buckets are saved (for the next call?) and the one that
gets passed to the next filter finally.
It doesn't look right to me, if tmpbb is to be forwarded in the same
pass there is no need to change its buckets' lifetime.
Couldn't we ap_save_brigade(f, &ctx->holdingbb, &ctx->tmpbb, ..) at
the end of the filter only?

Thanks for the review, bare in mind that this is my first "big" patch so I'd probably need a better grasp of the internals first :) I haven't touched holdingbb since I saw that was used elsewhere (in RATE_FULLSPEED), but I can try to check it as well. The idea of my patch (that I aimed to) is to pass the ctx->tmbbb only if it reaches chunk_size (or EOS is seen) and buffer otherwise the buckets in it using ap_save_brigade (waiting for the next call to see if chunk_size is reached).

So if I got your point correctly you would use ctx->holdingbb to store the buckets (and changing their lifetime possibly) between calls, and tmpbb only within the same filter invocation?

After re-reading the code and I think I got your point Yann, I'll try to re-work my idea and create a new patch. Thanks!

Tried to improve the ctx->tmpbb usage adding a new ctx->buffer brigade, with the only duty of buffering buckets between filter's invocations (if needed):

I didn't touch holdingbb since it is already used to allow buckets to skip the rate limit and go full speed, even if I am not sure if this functionality has it ever been used. I would prefer not to touch that special logic and fix only this use case.

I may have misunderstood Yann's suggestions, if so sorry for the spam, but I'd be glad to get a bit more info about how a better patch should look like :)

I promise documentation in the docs/developer section when the task is done! (I can add a "If even Luca did this, you can as well!" section in the docs :P)