Skip to content
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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

taoky
Copy link

@taoky taoky commented Nov 30, 2024

When debugging the conn limiter today, I tried with this code in development env, to help me get conn counter value in HTTP header:

local delay, err = limit_traffic.combine(limiters, keys, states)
if not delay then
    -- ...
else
    for i, limiter in ipairs(limiters) do
        local delay, err = limiter:incoming(keys[i], false)
        local limiter_name = limiters_name[i]
        ngx.header[ "X-RateLimit-" .. limiter_name ] = err
        ngx.header[ "X-RateLimit-" .. limiter_name .. "-Key" ] = keys[i]
    end
end

for i, limiter in ipairs(limiters) do
    if limiter.is_committed then
        if limiter:is_committed() then
            ngx.log(ngx.WARN, limiters_name[i])
            table.insert(limiters_to_leave, limiter)
            table.insert(keys_to_leave , keys[i])
        end
    end
end

And surprised by that, after I added the limiter:incoming(keys[i], false) here, the limiter:is_committed() returns false even when it is actually committed.

In conn.md, it says:

* `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).

But it is actually not just "dry run" -- incoming would always set committed to false at first not matter what.

function _M.incoming(self, key, commit)
    local dict = self.dict
    local max = self.max

    self.committed = false
    -- ...
end

This PR tries to fix the lua code to align with docs, and adds a testcase for that.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants