[count-inc] Add decrement logic to the incoming function #843
Conversation
| @@ -1,29 +1,68 @@ | |||
| local loader = require 'apicast.configuration_loader.remote_v1' | |||
| local _M = require 'apicast.configuration_loader.remote_v1' | |||
There was a problem hiding this comment.
This commit is not related to the rest of the PR, so it should be in a different one.
| @@ -194,7 +194,7 @@ describe('Rate limit policy', function() | |||
| assert(rate_limit_policy:access(context)) | |||
| assert.returns_error('limits exceeded', rate_limit_policy:access(context)) | |||
|
|
|||
| assert.equal('2', redis:get('11110_fixed_window_test3')) | |||
| assert.equal('1', redis:get('11110_fixed_window_test3')) | |||
There was a problem hiding this comment.
I don't fully understand the purpose of this change.
Before, we increased the number of hits even when going over limits. Now we force the value to be at most equal to the limit. Why do we need to do this?
There was a problem hiding this comment.
Before, we increased the number of hits even when going over limits.
First, we should not increase the number of hits when going over limits, because we should count the number the client calls the API backend.
The previous behavior is the below:
Case 1:
fixed_window_limiters : [
{ "key" : { "name" : "a"}, "count" : 1, "window" : 10 },
{ "key" : { "name" : "b"}, "count" : 2, "window" : 10 }
]After 2 API requests,
the number of hits 'a' is '1',
the number of hits 'b' is '1'.
-> correct
Case 2:
fixed_window_limiters : [
{ "key" : { "name" : "a"}, "count" : 2, "window" : 10 },
{ "key" : { "name" : "b"}, "count" : 1, "window" : 10 }
]After 2 API requests,
the number of hits 'a' is '1',
the number of hits 'b' is '2'.
-> wrong
y-tabata
force-pushed
the
count-inc-decr
branch
from
4973f82 to
c4231f0
Compare
| @@ -33,6 +33,17 @@ function _M.incoming(self, key, commit) | |||
| if not count then | |||
| return nil, err | |||
| end | |||
|
|
|||
| if count > limit then | |||
| count, err = dict:incr(key, -1) | |||
There was a problem hiding this comment.
I believe this should be done by the .uncommit that is being called by resty.limit.traffic.
There was a problem hiding this comment.
@mikz you mean .uncommit needs to be called here with the condition of i == n, right?
https://github.com/openresty/lua-resty-limit-traffic/blob/master/lib/resty/limit/traffic.lua#L27-L29
However resty.limit.conn includes this.
So we should choose which file we edit, resty.limit.traffic and resty.limit.conn, or resty.limit.count-inc.
There was a problem hiding this comment.
@y-tabata I think we don't need to change any resty.limit library. We have to use it correctly.
resty.limit.traffic calls :incoming(key, false) until the last limiter which is called as :incoming(key, true). If any of those return an error, it is fine to just exit. Nothing was committed to the database.
When all of them succeed, it tries to call again :incoming(key, true), but in this case, if any of them fails then call :uncommit(key) for all limiters that it already called :incoming.
So I'd suspect our limiters are not rolled back because they are not wrapped in one resty.limit.traffic.
There was a problem hiding this comment.
resty.limit.traffic calls :incoming(key, false) until the last limiter which is called as :incoming(key, true). If any of those return an error, it is fine to just exit. Nothing was committed to the database.
Using the current count-inc, I think that when the last limiter, which is called as :incoming(key, true), only returns nil and "rejected", the last one is committed to the database.
There was a problem hiding this comment.
Ok. I got it now. Implemented a fix to the upstream library: openresty/lua-resty-limit-traffic@c53f224 (openresty/lua-resty-limit-traffic#34)
I think we can just copy that here.
There was a problem hiding this comment.
It is just cosmetically different from
openresty/lua-resty-limit-traffic#34 which is not merged anyway, so 👍 .
Closes #830.