[PATCH] Limit req: r/h and r/d support

[Bernhard Reutner-Fischer]

On 16 February 2018 at 17:52, Maxim Dounin <mdounin at mdounin.ru> wrote:

>> Limit req: r/h and r/d support

> Do you use it yourself, or your patch is based on the feature
> requests in question?  If yes, what is your use case?

I have to limit certain use-cases to a handful of requests per day (don't ask).
Mentioned feature-requests just show that this oddish corner case is a
real knock-out criterion in the real world so i mentioned them to
emphasis that it wasn't just /me but that you're losing users due to
this. Often this is a hard requirement. You don't seem to offer any
sensible, off the shelve (or documented) way to handle "long" request
limits anyway -- think shm -- at least none that i was able to find?

> Style: please add a trailing dot.

Sure, please excuse my sloppy reading of previous logs, i honestly
failed to notice that, added.

> On 32-bit platforms this means that the maximum supported rate
> would be about ~40kr/s.  And I suspect this can hurt real setups.

agreed. Not sure about current typical HTTP server rates, admittedly.
Getting esoteric, but current servers usually satisfy millions of
requests but i'm not sure if you'd usually limit those to let's say
100k/s, but of course i see your point.
[]
> The ctx->rate is unsigned, and the ctx->rate check will only fail
> if ctx->rate is 0.  While this might happen in some cases, it
> certainly won't catch all overflows (which are quite likely in
> real configurations on 32-bit platforms, see above).
>
> Also, the "rate" type is ngx_int_t, so (rate * 100000) will cause
> integer overflow, which is not defined.  It might be a better idea
> to avoid overflows at all.

yea, see below.
>
> Also, the "rate" type is ngx_int_t, not ngx_uint_t, so proper
> format would be "%i".

right.

> In general, it might be a better idea to move the check to
> ngx_atoi(), and test something like NGX_MAX_INT_T_VALUE / 100000.
> This will ensure no overflow can happen during calculations below,
> and will allow logging of the original value.  This will further
> limit maximum rate to about 20kr/s on 32-bit platforms though.
>
> Note that I have no good solution for 32-bit platforms and the
> 40kr/s (20kr/s) limit.

my notes read:
disentangle rate. Should have rate(per unit) and unit(in seconds?
minutes? calc!) where excess would have same base as rate; common
timer conversion helper for rate/unit?

This has much more impact and hence is a much larger patch but would
certainly be way easier to grok compared to the current handling.
Would you be willing to fix the existing rate mess^whandling?

cheers,