Re: [PR] fix(limit-req): Make Redis path atomic via EVAL + use hash key with TTL [apisix]
AlinsRan commented on PR #12605: URL: https://github.com/apache/apisix/pull/12605#issuecomment-4890178010 Closing this — the underlying issue (#12592) was fixed and merged in #13467, which takes the same approach: single hash key + one atomic `EVAL` for the read-decay-check-write cycle, with a TTL and first-request admission preserved. The one thing this PR adds on top is the `EVALSHA` fast path with `NOSCRIPT` fallback. That's a nice bandwidth optimization but not needed for correctness — happy to see it as a small follow-up PR if you want to pursue it. Thanks for the work 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]
Re: [PR] fix(limit-req): Make Redis path atomic via EVAL + use hash key with TTL [apisix]
AlinsRan closed pull request #12605: fix(limit-req): Make Redis path atomic via EVAL + use hash key with TTL URL: https://github.com/apache/apisix/pull/12605 -- 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]
Re: [PR] fix(limit-req): Make Redis path atomic via EVAL + use hash key with TTL [apisix]
nic-6443 commented on PR #12605: URL: https://github.com/apache/apisix/pull/12605#issuecomment-4676905376 Heads up — master now has this fixed via #13467 (commit 6e877eb44), which made the redis commit path a single atomic EVAL on a hash key with TTL, for both redis and redis-cluster policies — very close to what this PR proposed. Your diagnosis of the GET/GET/SET/SET race was right, but since #13467 is merged I think this PR can be closed. -- 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]
Re: [PR] fix(limit-req): Make Redis path atomic via EVAL + use hash key with TTL [apisix]
falvaradorodriguez commented on code in PR #12605:
URL: https://github.com/apache/apisix/pull/12605#discussion_r3147070255
##
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:
Fixed by @Baoyuantop here
https://github.com/apache/apisix/pull/12605/commits/4deb878f622068dcd6db800aa2fc4fb70b0cb9a5
--
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]
Re: [PR] fix(limit-req): Make Redis path atomic via EVAL + use hash key with TTL [apisix]
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=1`: 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]
Re: [PR] fix(limit-req): Make Redis path atomic via EVAL + use hash key with TTL [apisix]
Baoyuantop commented on PR #12605: URL: https://github.com/apache/apisix/pull/12605#issuecomment-4175193010 It seems the failed CI is unrelated to this PR; I'll try to fix it. -- 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]
Re: [PR] fix(limit-req): Make Redis path atomic via EVAL + use hash key with TTL [apisix]
Baoyuantop commented on PR #12605: URL: https://github.com/apache/apisix/pull/12605#issuecomment-4162828421 Okay, I'll check it after CI finishes executing. -- 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]
Re: [PR] fix(limit-req): Make Redis path atomic via EVAL + use hash key with TTL [apisix]
falvaradorodriguez commented on PR #12605: URL: https://github.com/apache/apisix/pull/12605#issuecomment-4161991110 > Hi @falvaradorodriguez, I see no problems with the code here. Could you please fix the failing test? Hi @Baoyuantop! As you can see [here](https://github.com/apache/apisix/actions/runs/21439970632/job/64626319434?pr=12605), the tests that were failing were not related to this PR. https://github.com/user-attachments/assets/390650b7-6005-44fc-881b-47e966aa958e"; /> https://github.com/user-attachments/assets/506b7720-7077-4319-82bf-82ce6a8b7106"; /> Could you check that again? Thanks -- 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]
Re: [PR] fix(limit-req): Make Redis path atomic via EVAL + use hash key with TTL [apisix]
falvaradorodriguez commented on PR #12605: URL: https://github.com/apache/apisix/pull/12605#issuecomment-4161970467 > Hi @falvaradorodriguez, thank you for making the Redis limit-req path atomic via EVAL! > > Using a Lua script with EVAL to ensure atomicity of the rate limiting operation is the correct approach — the current non-atomic multi-command flow can have race conditions under high concurrency. With 12 reviews, this has been thoroughly discussed. > > **To confirm readiness**: > > 1. Are all 12 review comments addressed? Yes, they have all been checked and corrected where necessary. > 2. Has the EVAL script been tested under concurrent load to verify it resolves the race condition? Yes, in fact, in my case it has been validated in an environment with a high volume of requests > 3. The hash key with TTL approach — does it handle key expiration correctly for sliding windows? Yes, also validated. > This is an important correctness fix. Let's get it finalized! Thank you. Thank you for the review! -- 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]
Re: [PR] fix(limit-req): Make Redis path atomic via EVAL + use hash key with TTL [apisix]
Baoyuantop commented on PR #12605: URL: https://github.com/apache/apisix/pull/12605#issuecomment-4124205937 Hi @falvaradorodriguez, I see no problems with the code here. Could you please fix the failing test? -- 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]
Re: [PR] fix(limit-req): Make Redis path atomic via EVAL + use hash key with TTL [apisix]
Copilot commented on code in PR #12605:
URL: https://github.com/apache/apisix/pull/12605#discussion_r2917010321
##
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]) -- req/s
+ local now= tonumber(ARGV[2]) -- ms
+ local burst = tonumber(ARGV[3]) -- req/s
Review Comment:
The Lua script comments label `rate`/`burst` as "req/s", but in this limiter
implementation they are passed around in the same scaled units used by the
original algorithm (`rate = configured_rate * 1000`, `burst = configured_burst
* 1000`). Updating these comments to reflect the actual units would reduce the
risk of future logic changes introducing subtle math/TTL bugs.
```suggestion
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)
```
##
t/plugin/limit-req-redis.t:
##
@@ -660,3 +660,114 @@ passed
GET /t
--- response_body eval
qr/property \"rate\" validation failed: expected 0 to be greater than 0/
+
+
+
+=== TEST 25: verify atomic Redis operations with hash key structure
+--- config
+location /t {
+content_by_lua_block {
+local redis = require "resty.redis"
+local red = redis:new()
+red:set_timeout(1000)
+
+local ok, err = red:connect("127.0.0.1", 6379)
+if not ok then
+ngx.say("failed to connect: ", err)
+return
+end
+
+-- Clean up any existing keys
+red:del("limit_req:{test_key}:state")
+
+-- Test the new hash-based key structure
+local util = require("apisix.plugins.limit-req.util")
+local limiter = {
+rate = 10,-- 10 req/s
+burst = 1000 -- 1000 req/s burst
+}
+
+-- First request should succeed
+local delay, excess = util.incoming(limiter, red, "test_key", true)
+if delay then
+ngx.say("first request: delay=", delay, " excess=", excess)
+else
+ngx.say("first request failed: ", excess)
+end
+
+-- Verify the Redis hash was created with correct key format
+local vals = red:hmget("limit_req:{test_key}:state", "excess",
"last")
+if vals[1] and vals[2] then
Review Comment:
`resty.redis:hmget` returns `(vals, err)`. This test only captures the first
return value and then unconditionally indexes `vals[1]`/`vals[2]`, which will
throw a Lua runtime error if `hmget` fails and returns `nil`. Capture `err` and
guard for `not vals` (and ideally print the error) to make the test robust and
failures diagnosable.
```suggestion
local vals, err = red:hmget("limit_req:{test_key}:state",
"excess", "last")
if not vals then
ngx.say("failed to hmget: ", err)
elseif vals[1] and vals[2] then
```
--
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]
Re: [PR] fix(limit-req): Make Redis path atomic via EVAL + use hash key with TTL [apisix]
falvaradorodriguez commented on code in PR #12605:
URL: https://github.com/apache/apisix/pull/12605#discussion_r2736404742
##
apisix/plugins/limit-req/util.lua:
##
@@ -14,61 +14,97 @@
-- 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 tostring = tostring
+local type = type
+local core = require("apisix.core")
+local resty_string = require("resty.string")
local _M = {version = 0.1}
+local script = core.string.compress_script([=[
+ local state_key = KEYS[1] -- state_key (hash), fields:
"excess", "last"
+ local rate = tonumber(ARGV[1]) -- req/s
+ local now= tonumber(ARGV[2]) -- ms
+ local burst = tonumber(ARGV[3]) -- req/s
+ 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)
+ end
+
+ return {1, new_excess}
+]=])
+
+-- Pre-calculate SHA1 for EVALSHA optimization
+local script_sha1 = resty_string.to_hex(ngx.sha1_bin(script))
+
+
+local function is_noscript_error(err)
+if not err then
+return false
+end
+
+local s
+if type(err) == "table" then
+s = tostring(err[1] or err.err or err.message or err.msg or err)
Review Comment:
With the changes you mentioned
[here](https://github.com/apache/apisix/pull/12605#discussion_r2734766405),
what I wrote above doesn't makes sense. I have replicated the way you handle
the error you use in the `limit-conn` plugin.
--
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]
Re: [PR] fix(limit-req): Make Redis path atomic via EVAL + use hash key with TTL [apisix]
falvaradorodriguez commented on code in PR #12605:
URL: https://github.com/apache/apisix/pull/12605#discussion_r2736070946
##
apisix/plugins/limit-req/util.lua:
##
@@ -14,61 +14,97 @@
-- 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 tostring = tostring
+local type = type
+local core = require("apisix.core")
+local resty_string = require("resty.string")
local _M = {version = 0.1}
+local script = core.string.compress_script([=[
+ local state_key = KEYS[1] -- state_key (hash), fields:
"excess", "last"
+ local rate = tonumber(ARGV[1]) -- req/s
+ local now= tonumber(ARGV[2]) -- ms
+ local burst = tonumber(ARGV[3]) -- req/s
+ 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)
+ end
+
+ return {1, new_excess}
+]=])
+
+-- Pre-calculate SHA1 for EVALSHA optimization
+local script_sha1 = resty_string.to_hex(ngx.sha1_bin(script))
+
+
+local function is_noscript_error(err)
+if not err then
+return false
+end
+
+local s
+if type(err) == "table" then
+s = tostring(err[1] or err.err or err.message or err.msg or err)
Review Comment:
~~Thanks for the note. In resty.redis the error is indeed typically returned
as a plain string.~~
~~However, since this code path is meant to work with both resty.redis and
resty.rediscluster, I added a small defensive handling for the case where
errors may be propagated as a structured table (e.g. containing err/message
fields) instead of a raw string.~~
~~This doesn’t change the behavior for the common case, but makes the
NOSCRIPT detection more robust across Redis standalone and Redis Cluster
setups, especially during redirects/failovers where error formats may differ.~~
--
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]
Re: [PR] fix(limit-req): Make Redis path atomic via EVAL + use hash key with TTL [apisix]
falvaradorodriguez commented on code in PR #12605:
URL: https://github.com/apache/apisix/pull/12605#discussion_r2736393769
##
apisix/plugins/limit-req/util.lua:
##
@@ -14,61 +14,97 @@
-- 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 tostring = tostring
+local type = type
+local core = require("apisix.core")
+local resty_string = require("resty.string")
local _M = {version = 0.1}
+local script = core.string.compress_script([=[
+ local state_key = KEYS[1] -- state_key (hash), fields:
"excess", "last"
+ local rate = tonumber(ARGV[1]) -- req/s
+ local now= tonumber(ARGV[2]) -- ms
+ local burst = tonumber(ARGV[3]) -- req/s
+ 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)
+ end
+
+ return {1, new_excess}
+]=])
+
+-- Pre-calculate SHA1 for EVALSHA optimization
+local script_sha1 = resty_string.to_hex(ngx.sha1_bin(script))
+
+
+local function is_noscript_error(err)
+if not err then
+return false
+end
+
+local s
+if type(err) == "table" then
+s = tostring(err[1] or err.err or err.message or err.msg or err)
+else
+s = tostring(err)
+end
+
+return s:find("NOSCRIPT", 1, true) ~= nil
+end
+
+
-- the "commit" argument controls whether should we record the event in shm.
function _M.incoming(self, red, key, commit)
local rate = self.rate
local now = ngx_now() * 1000
-key = "limit_req" .. ":" .. key
-local excess_key = key .. "excess"
-local last_key = key .. "last"
+local state_key = "limit_req:{" .. key .. "}:state"
-local excess, err = red:get(excess_key)
-if err then
-return nil, err
-end
-local last, err = red:get(last_key)
-if err then
-return nil, err
-end
+local commit_flag = commit and "1" or "0"
-if excess ~= ngx_null and last ~= ngx_null then
-excess = tonumber(excess)
-last = tonumber(last)
-local elapsed = now - last
-excess = max(excess - rate * abs(elapsed) / 1000 + 1000, 0)
+-- Try EVALSHA first (fast path).
+local res, err = red:evalsha(script_sha1, 1, state_key,
+rate, now, self.burst, commit_flag)
-if excess > self.burst then
-return nil, "rejected"
-end
-else
-excess = 0
+-- If the script isn't cached on this Redis node, fall back to EVAL.
+if not res and is_noscript_error(err) then
Review Comment:
Makes sense, and it is also good practice to keep the logic of the plugins
aligned. I have tried to replicate the functionality you mention here:
https://github.com/apache/apisix/pull/12605/changes/05657f67151416b4af9bd37937bc3b76e9fa5b27
--
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]
Re: [PR] fix(limit-req): Make Redis path atomic via EVAL + use hash key with TTL [apisix]
falvaradorodriguez commented on code in PR #12605:
URL: https://github.com/apache/apisix/pull/12605#discussion_r2736070946
##
apisix/plugins/limit-req/util.lua:
##
@@ -14,61 +14,97 @@
-- 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 tostring = tostring
+local type = type
+local core = require("apisix.core")
+local resty_string = require("resty.string")
local _M = {version = 0.1}
+local script = core.string.compress_script([=[
+ local state_key = KEYS[1] -- state_key (hash), fields:
"excess", "last"
+ local rate = tonumber(ARGV[1]) -- req/s
+ local now= tonumber(ARGV[2]) -- ms
+ local burst = tonumber(ARGV[3]) -- req/s
+ 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)
+ end
+
+ return {1, new_excess}
+]=])
+
+-- Pre-calculate SHA1 for EVALSHA optimization
+local script_sha1 = resty_string.to_hex(ngx.sha1_bin(script))
+
+
+local function is_noscript_error(err)
+if not err then
+return false
+end
+
+local s
+if type(err) == "table" then
+s = tostring(err[1] or err.err or err.message or err.msg or err)
Review Comment:
Thanks for the note. In resty.redis the error is indeed typically returned
as a plain string.
However, since this code path is meant to work with both resty.redis and
resty.rediscluster, I added a small defensive handling for the case where
errors may be propagated as a structured table (e.g. containing err/message
fields) instead of a raw string.
This doesn’t change the behavior for the common case, but makes the NOSCRIPT
detection more robust across Redis standalone and Redis Cluster setups,
especially during redirects/failovers where error formats may differ.
--
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]
Re: [PR] fix(limit-req): Make Redis path atomic via EVAL + use hash key with TTL [apisix]
shreemaan-abhishek commented on code in PR #12605:
URL: https://github.com/apache/apisix/pull/12605#discussion_r2734766405
##
apisix/plugins/limit-req/util.lua:
##
@@ -14,61 +14,97 @@
-- 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 tostring = tostring
+local type = type
+local core = require("apisix.core")
+local resty_string = require("resty.string")
local _M = {version = 0.1}
+local script = core.string.compress_script([=[
+ local state_key = KEYS[1] -- state_key (hash), fields:
"excess", "last"
+ local rate = tonumber(ARGV[1]) -- req/s
+ local now= tonumber(ARGV[2]) -- ms
+ local burst = tonumber(ARGV[3]) -- req/s
+ 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)
+ end
+
+ return {1, new_excess}
+]=])
+
+-- Pre-calculate SHA1 for EVALSHA optimization
+local script_sha1 = resty_string.to_hex(ngx.sha1_bin(script))
+
+
+local function is_noscript_error(err)
+if not err then
+return false
+end
+
+local s
+if type(err) == "table" then
+s = tostring(err[1] or err.err or err.message or err.msg or err)
+else
+s = tostring(err)
+end
+
+return s:find("NOSCRIPT", 1, true) ~= nil
+end
+
+
-- the "commit" argument controls whether should we record the event in shm.
function _M.incoming(self, red, key, commit)
local rate = self.rate
local now = ngx_now() * 1000
-key = "limit_req" .. ":" .. key
-local excess_key = key .. "excess"
-local last_key = key .. "last"
+local state_key = "limit_req:{" .. key .. "}:state"
-local excess, err = red:get(excess_key)
-if err then
-return nil, err
-end
-local last, err = red:get(last_key)
-if err then
-return nil, err
-end
+local commit_flag = commit and "1" or "0"
-if excess ~= ngx_null and last ~= ngx_null then
-excess = tonumber(excess)
-last = tonumber(last)
-local elapsed = now - last
-excess = max(excess - rate * abs(elapsed) / 1000 + 1000, 0)
+-- Try EVALSHA first (fast path).
+local res, err = red:evalsha(script_sha1, 1, state_key,
+rate, now, self.burst, commit_flag)
-if excess > self.burst then
-return nil, "rejected"
-end
-else
-excess = 0
+-- If the script isn't cached on this Redis node, fall back to EVAL.
+if not res and is_noscript_error(err) then
Review Comment:
I observed that reliability of redis evalsha with redis-cluster was very low.
https://github.com/apache/apisix/pull/12872#discussion_r2727073505
So I decided to not use `evalsha` when the `policy` is set to
`rediscluster`. You can refer the PR I just linked to see how I did it.
Let me know if you have any better ideas tho.
--
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]
Re: [PR] fix(limit-req): Make Redis path atomic via EVAL + use hash key with TTL [apisix]
shreemaan-abhishek commented on code in PR #12605:
URL: https://github.com/apache/apisix/pull/12605#discussion_r2734755261
##
apisix/plugins/limit-req/util.lua:
##
@@ -14,61 +14,97 @@
-- 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 tostring = tostring
+local type = type
+local core = require("apisix.core")
+local resty_string = require("resty.string")
local _M = {version = 0.1}
+local script = core.string.compress_script([=[
+ local state_key = KEYS[1] -- state_key (hash), fields:
"excess", "last"
+ local rate = tonumber(ARGV[1]) -- req/s
+ local now= tonumber(ARGV[2]) -- ms
+ local burst = tonumber(ARGV[3]) -- req/s
+ 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)
+ end
+
+ return {1, new_excess}
+]=])
+
+-- Pre-calculate SHA1 for EVALSHA optimization
+local script_sha1 = resty_string.to_hex(ngx.sha1_bin(script))
+
+
+local function is_noscript_error(err)
+if not err then
+return false
+end
+
+local s
+if type(err) == "table" then
+s = tostring(err[1] or err.err or err.message or err.msg or err)
Review Comment:
last time I checked, the type of `err` was always a string. Did you notice
something different that you added `err.err or err.message or err.msg or err`?
--
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]
Re: [PR] fix(limit-req): Make Redis path atomic via EVAL + use hash key with TTL [apisix]
shreemaan-abhishek commented on code in PR #12605:
URL: https://github.com/apache/apisix/pull/12605#discussion_r2734752809
##
apisix/plugins/limit-req/util.lua:
##
@@ -14,61 +14,97 @@
-- 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 tostring = tostring
+local type = type
+local core = require("apisix.core")
+local resty_string = require("resty.string")
local _M = {version = 0.1}
+local script = core.string.compress_script([=[
+ local state_key = KEYS[1] -- state_key (hash), fields:
"excess", "last"
+ local rate = tonumber(ARGV[1]) -- req/s
+ local now= tonumber(ARGV[2]) -- ms
+ local burst = tonumber(ARGV[3]) -- req/s
+ 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
Review Comment:
nice!
--
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]
Re: [PR] fix(limit-req): Make Redis path atomic via EVAL + use hash key with TTL [apisix]
Baoyuantop commented on PR #12605: URL: https://github.com/apache/apisix/pull/12605#issuecomment-3691975013 Hi @falvaradorodriguez, could you please fix the lint bug? -- 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]
Re: [PR] fix(limit-req): Make Redis path atomic via EVAL + use hash key with TTL [apisix]
falvaradorodriguez commented on PR #12605: URL: https://github.com/apache/apisix/pull/12605#issuecomment-3689428635 > Hi @falvaradorodriguez, it appears there are test failures in CI that need to be fixed. There was a node management issue with Redis.cluster. It seems that loading the script does not guarantee that it will be available in the next sha1 evaluation. I have changed the fallback to eval to ensure that the script is executed. Once executed with eval, it can be checked with sha1. -- 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]
Re: [PR] fix(limit-req): Make Redis path atomic via EVAL + use hash key with TTL [apisix]
Baoyuantop commented on PR #12605: URL: https://github.com/apache/apisix/pull/12605#issuecomment-3688522318 Hi @falvaradorodriguez, it appears there are test failures in CI that need to be fixed. -- 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]
Re: [PR] fix(limit-req): Make Redis path atomic via EVAL + use hash key with TTL [apisix]
falvaradorodriguez commented on code in PR #12605:
URL: https://github.com/apache/apisix/pull/12605#discussion_r2642936410
##
apisix/plugins/limit-req/util.lua:
##
@@ -14,60 +14,66 @@
-- 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 script = core.string.compress_script([=[
+ local state_key = KEYS[1] -- state_key (hash), fields:
"excess", "last"
+ local rate = tonumber(ARGV[1]) -- req/s
+ local now= tonumber(ARGV[2]) -- ms
+ local burst = tonumber(ARGV[3]) -- req/s
+ 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)
+redis.call("EXPIRE", state_key, 60)
+ end
+
+ return {1, new_excess}
+]=])
+
+
-- the "commit" argument controls whether should we record the event in shm.
function _M.incoming(self, red, key, commit)
local rate = self.rate
local now = ngx_now() * 1000
-key = "limit_req" .. ":" .. key
-local excess_key = key .. "excess"
-local last_key = key .. "last"
+local state_key = "limit_req:{" .. key .. "}:state"
-local excess, err = red:get(excess_key)
-if err then
-return nil, err
-end
-local last, err = red:get(last_key)
-if err then
+local commit_flag = commit and "1" or "0"
+
+local res, err = red:eval(script, 1, state_key,
Review Comment:
Thanks @membphis, makes all sense, updated
https://github.com/apache/apisix/pull/12605/changes/8263caa4b8831212189b0bda73856e8d16b53bfb
--
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]
Re: [PR] fix(limit-req): Make Redis path atomic via EVAL + use hash key with TTL [apisix]
membphis commented on code in PR #12605:
URL: https://github.com/apache/apisix/pull/12605#discussion_r2639094869
##
apisix/plugins/limit-req/util.lua:
##
@@ -14,60 +14,66 @@
-- 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 script = core.string.compress_script([=[
+ local state_key = KEYS[1] -- state_key (hash), fields:
"excess", "last"
+ local rate = tonumber(ARGV[1]) -- req/s
+ local now= tonumber(ARGV[2]) -- ms
+ local burst = tonumber(ARGV[3]) -- req/s
+ 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)
+redis.call("EXPIRE", state_key, 60)
+ end
+
+ return {1, new_excess}
+]=])
+
+
-- the "commit" argument controls whether should we record the event in shm.
function _M.incoming(self, red, key, commit)
local rate = self.rate
local now = ngx_now() * 1000
-key = "limit_req" .. ":" .. key
-local excess_key = key .. "excess"
-local last_key = key .. "last"
+local state_key = "limit_req:{" .. key .. "}:state"
-local excess, err = red:get(excess_key)
-if err then
-return nil, err
-end
-local last, err = red:get(last_key)
-if err then
+local commit_flag = commit and "1" or "0"
+
+local res, err = red:eval(script, 1, state_key,
Review Comment:
we can use `evalsha`, avoid to repeat pass the `script` every time
official website: https://redis.io/docs/latest/commands/evalsha
here is a demo:
```lua
local redis = require "resty.redis"
-- Create redis instance
local red = redis:new()
-- Connect to Redis
local ok, err = red:connect("127.0.0.1", 6379)
if not ok then
ngx.say("failed to connect: ", err)
return
end
-- Step 1: Load script and get SHA1
local script = "return redis.call('get', KEYS[1])"
local sha1, err = red:script("load", script)
if not sha1 then
ngx.say("failed to load script: ", err)
return
end
ngx.say("Script SHA1: ", sha1)
-- Step 2: Set a test key
red:set("mykey", "hello " .. ngx.time())
-- Step 3: Use evalsha to execute script
-- evalsha[key2 ... ] [arg1 arg2 ...]
local res, err = red:evalsha(sha1, 1, "mykey")
if not res then
ngx.say("failed to evalsha: ", err)
return
end
ngx.say("Result: ", res) -- Output: hello
-- Put connection back to the pool
local ok, err = red:set_keepalive(1, 100)
if not ok then
ngx.say("failed to set keepalive: ", err)
return
end
```
we can cache the `sha1` result
--
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]
Re: [PR] fix(limit-req): Make Redis path atomic via EVAL + use hash key with TTL [apisix]
Baoyuantop commented on PR #12605: URL: https://github.com/apache/apisix/pull/12605#issuecomment-3317583823 Hi @falvaradorodriguez, there are failed CI that need fixing. -- 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]
Re: [PR] fix(limit-req): Make Redis path atomic via EVAL + use hash key with TTL [apisix]
Baoyuantop commented on code in PR #12605:
URL: https://github.com/apache/apisix/pull/12605#discussion_r2374904796
##
apisix/plugins/limit-req/util.lua:
##
@@ -14,60 +14,66 @@
-- 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 script = core.string.compress_script([=[
+ local state_key = KEYS[1] -- state_key (hash), fields:
"excess", "last"
+ local rate = tonumber(ARGV[1]) -- req/s
+ local now= tonumber(ARGV[2]) -- ms
+ local burst = tonumber(ARGV[3]) -- req/s
+ 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)
+redis.call("EXPIRE", state_key, 60)
Review Comment:
Can TTL be dynamically configured based on the rate-limiting window period,
or allow user configuration?
--
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]
Re: [PR] fix(limit-req): Make Redis path atomic via EVAL + use hash key with TTL [apisix]
falvaradorodriguez commented on code in PR #12605:
URL: https://github.com/apache/apisix/pull/12605#discussion_r2375702880
##
apisix/plugins/limit-req/util.lua:
##
@@ -14,60 +14,66 @@
-- 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 script = core.string.compress_script([=[
+ local state_key = KEYS[1] -- state_key (hash), fields:
"excess", "last"
+ local rate = tonumber(ARGV[1]) -- req/s
+ local now= tonumber(ARGV[2]) -- ms
+ local burst = tonumber(ARGV[3]) -- req/s
+ 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)
+redis.call("EXPIRE", state_key, 60)
Review Comment:
In the limit-req plugin, there is no time window like in the limit-count
plugin.
This configuration is simply done to prevent keys from remaining dead in
redis. Currently, if a consumer stops making requests, their key remains in
redis until it is deleted by an action independent of Apisix.
It could be made configurable, but in my opinion, it would not add much
value to this plugin. It would also be an extra feature, not related with this
fix.
In my opinion, since limit-req is always for 1 second, with a 1 minute of
margin in redis, it is acceptable. If the consumer makes requests after 1
minute, the flow management would be the same as for the first request.
--
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]
Re: [PR] fix(limit-req): Make Redis path atomic via EVAL + use hash key with TTL [apisix]
luarx commented on PR #12605: URL: https://github.com/apache/apisix/pull/12605#issuecomment-3381432928 When will this PR be merged? I am having the same issue that this PR fixes 🙏 -- 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]
Re: [PR] fix(limit-req): Make Redis path atomic via EVAL + use hash key with TTL [apisix]
Baoyuantop commented on PR #12605: URL: https://github.com/apache/apisix/pull/12605#issuecomment-3392875648 I will promptly urge other community maintainers to review. -- 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]
Re: [PR] fix(limit-req): Make Redis path atomic via EVAL + use hash key with TTL [apisix]
falvaradorodriguez commented on code in PR #12605:
URL: https://github.com/apache/apisix/pull/12605#discussion_r2375702880
##
apisix/plugins/limit-req/util.lua:
##
@@ -14,60 +14,66 @@
-- 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 script = core.string.compress_script([=[
+ local state_key = KEYS[1] -- state_key (hash), fields:
"excess", "last"
+ local rate = tonumber(ARGV[1]) -- req/s
+ local now= tonumber(ARGV[2]) -- ms
+ local burst = tonumber(ARGV[3]) -- req/s
+ 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)
+redis.call("EXPIRE", state_key, 60)
Review Comment:
In the limit-req plugin, there is no window time like in the limit-count
plugin.
This configuration is simply done to prevent keys from remaining dead in
redis. Currently, if a consumer stops making requests, their key remains in
redis until it is deleted by an action independent of Apisix.
It could be made configurable, but in my opinion, it would not add much
value to this plugin. It would also be an extra feature, not related with this
fix.
In my opinion, since limit-req is always for 1 second, with a 1 minute of
margin in redis, it is acceptable. If the consumer makes requests after 1
minute, the flow management would be the same as for the first request.
--
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]
Re: [PR] fix(limit-req): Make Redis path atomic via EVAL + use hash key with TTL [apisix]
falvaradorodriguez commented on PR #12605: URL: https://github.com/apache/apisix/pull/12605#issuecomment-3320340743 > Hi @falvaradorodriguez, there are failed CI that need fixing. The failures are not related to the current changes. Tests of the modified plugin seems to be fine. ``` [15:23:12] t/plugin/limit-req-redis-cluster.t . ok15865 ms ( 0.01 usr 0.01 sys + 0.91 cusr 2.03 csys = 2.96 CPU) [15:23:29] t/plugin/limit-req-redis.t . ok16682 ms ( 0.02 usr 0.00 sys + 0.95 cusr 2.23 csys = 3.20 CPU) 2025/09/22 15:23:37 Processed 0 requests [15:23:41] t/plugin/limit-req.t ... ok11856 ms ( 0.02 usr 0.00 sys + 0.83 cusr 1.78 csys = 2.63 CPU) [15:23:46] t/plugin/limit-req2.t .. ok 5010 ms ( 0.01 usr 0.00 sys + 0.81 cusr 0.42 csys = 1.24 CPU) [15:23:47] t/plugin/limit-req3.t .. ok 1290 ms ( 0.00 usr 0.00 sys + 0.39 cusr 0.22 csys = 0.61 CPU) ``` -- 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]
Re: [PR] fix(limit-req): Make Redis path atomic via EVAL + use hash key with TTL [apisix]
Baoyuantop commented on PR #12605: URL: https://github.com/apache/apisix/pull/12605#issuecomment-3274152214 Hi @falvaradorodriguez, we need to add the test case for this fix. -- 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]
