nic-6443 commented on code in PR #12605:
URL: https://github.com/apache/apisix/pull/12605#discussion_r3085611666
##########
apisix/plugins/limit-req/util.lua:
##########
@@ -14,61 +14,103 @@
-- See the License for the specific language governing permissions and
-- limitations under the License.
--
-local math = require "math"
-local abs = math.abs
-local max = math.max
local ngx_now = ngx.now
-local ngx_null = ngx.null
local tonumber = tonumber
+local core = require("apisix.core")
local _M = {version = 0.1}
+local redis_incoming_script = core.string.compress_script([=[
+ local state_key = KEYS[1] -- state_key (hash), fields:
"excess", "last"
+ local rate = tonumber(ARGV[1]) -- scaled request rate
(configured_rate * 1000)
+ local now = tonumber(ARGV[2]) -- ms
+ local burst = tonumber(ARGV[3]) -- scaled burst (configured_burst *
1000)
+ local commit = tonumber(ARGV[4]) -- 1/0
+
+ local vals = redis.call("HMGET", state_key, "excess", "last")
+ local prev_excess = tonumber(vals[1] or "0")
+ local prev_last = tonumber(vals[2] or "0")
+
+ local new_excess
+ if prev_last > 0 then
+ local elapsed = math.abs(now - prev_last)
+ new_excess = math.max(prev_excess - rate * (elapsed) / 1000 + 1000, 0)
+ else
+ new_excess = 0
+ end
+
+ if new_excess > burst then
+ return {0, new_excess}
+ end
+
+ if commit == 1 then
+ redis.call("HMSET", state_key, "excess", new_excess, "last", now)
+ local ttl = math.ceil(burst / rate) + 1
+ redis.call("EXPIRE", state_key, ttl)
Review Comment:
TTL is too short when `burst=0` with sub-1 request rates.
With `rate=0.1, burst=0` (scaled: `rate=100, burst=0`): `TTL = ceil(0/100) +
1 = 1 second`. But the leaky bucket drain time is `1000 / rate = 10s`. After 1
second the key expires, resetting the limiter — effectively allowing 1 req/s
instead of the configured 0.1 req/s (10× the intended rate).
The fix accounts for the per-request excess increment (`+1000` in scaled
units):
```lua
local ttl = math.ceil((burst + 1000) / rate) + 1
```
For `rate=0.1, burst=0`: TTL = `ceil(1000/100) + 1 = 11s` — correctly covers
the drain window.
For `rate=1, burst=0`: TTL = `ceil(1000/1000) + 1 = 2s` — still correct.
For `rate=1, burst=10000`: TTL = `12s` vs old `11s` — negligible difference.
This is pre-existing (same formula in old code), but since the PR's goal is
fixing rate-limiting correctness, worth addressing here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
