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

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

2018-06-08 12:43 GMT+02:00 Luca Toscano <toscano.luca@xxxxxxxxx>:
Hi Yann,

2018-06-07 12:59 GMT+02:00 Yann Ylavic <ylavic.dev@xxxxxxxxx>:
On Thu, Jun 7, 2018 at 10:17 AM, Yann Ylavic <ylavic.dev@xxxxxxxxx> wrote:
> On Thu, Jun 7, 2018 at 9:21 AM, Yann Ylavic <ylavic.dev@xxxxxxxxx> wrote:
>> Feel free to take what can serve you, or burn it ;)
> Burn it! Here is v2, hopefully a less buggy :)

FWIW, a v3 with less changes and a few comments where needed.
Less changes because it keeps using tmpbb in the RATE_LIMIT loop (with
APR_BRIGADE_CONCAT(ctx->tmpbb, bb) first).

All my consistency tests are green, and after a first (and quick!) pass of the code in your patch I can definitely say that your version is much better and coincise, I like it! Will try to review it in detail during the next days and then I'll commit it.

Sorry for the delay :)

While reading the code I didn't get if one particular use case may or may not happen at all, it is the only big doubt that I have before committing. Is it possible that the first bb to process (during the first invocation of the filter) is equal to the chunk_size + ctx->burst? IIUC the code relies on the fact that bb is not empty to decide whether or not to pass ctx->tmpp to the next filter in the chain, but if this is not the case then it may loop? 

I tried to dump the brigades of my tests (proxying content via mod_proxy_http and directly from httpd), the response headers in the beginning seems to always guarantee an initial bb of some bytes that make everything work.

Not sure if this is a valuable comment, the rest looks super good :)

Thanks for the patience!