Skip to content

bugfix: mark committed to false only when it is nil in conn #80

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion lib/resty/limit/conn.lua
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,9 @@ function _M.incoming(self, key, commit)
local dict = self.dict
local max = self.max

self.committed = false
if self.committed == nil then
self.committed = false
end

local conn, err
if commit then
Expand Down
3 changes: 2 additions & 1 deletion lib/resty/limit/conn.md
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,8 @@ key so that we can avoid a single client from flooding our service with too many
does not prefix nor suffix the user key so it is the user's responsibility to ensure the key
is unique in the `lua_shared_dict` shm zone).
* `commit` is a boolean value. If set to `true`, the object will actually record the event
in the shm zone backing the current object; otherwise it would just be a "dry run" (which is the default).
in the shm zone backing the current object; otherwise it would just be a "dry run" (which is the default),
which does not change the committed state.

The return values depend on the following cases:

Expand Down
56 changes: 48 additions & 8 deletions t/conn.t
Original file line number Diff line number Diff line change
Expand Up @@ -114,23 +114,23 @@ committed: false
3: 0, conn: 1
committed: true
4: 0, conn: 2
committed: false
committed: true
5: 0, conn: 2
committed: true
6: 1, conn: 3
committed: false
committed: true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I doubt if this change makes sense for is_committed() docs:

- Returns `true` if the previous [incoming](#incoming) call actually commits the event
+ Returns `true` if the previous [incoming](#incoming) calls actually have committed the event

7: 1, conn: 3
committed: false
committed: true
8: 1, conn: 3
committed: false
committed: true
9: 1, conn: 3
committed: false
committed: true
10: 1, conn: 3
committed: false
committed: true
11: 1, conn: 3
committed: false
committed: true
12: 1, conn: 3
committed: false
committed: true
--- no_error_log
[error]
[lua]
Expand Down Expand Up @@ -285,3 +285,43 @@ failed to limit conn: rejected
--- no_error_log
[error]
[lua]



=== TEST 6: incoming dry run shall not modify committed state
--- http_config eval
"
$::HttpConfig

lua_shared_dict store 1m;
"
--- config
location = /t {
content_by_lua_block {
local limit_conn = require "resty.limit.conn"
local lim = limit_conn.new("store", 2, 1, 1)
ngx.shared.store:flush_all()
local key = "foo"

local delay, err = lim:incoming(key, true)
if not delay then
ngx.say("failed to limit conn: ", err)
end
delay, err = lim:incoming(key, false)
if not delay then
ngx.say("failed to limit conn: ", err)
end
if not lim:is_committed() then
ngx.say("not committed")
else
ngx.say("committed")
end
}
}
--- request
GET /t
--- response_body
committed
--- no_error_log
[error]
[lua]