feature: implemented `resty.limit.count`. #21

moonming · 2017-09-04T09:20:34Z

implemented resty.limit.count as GitHub request rate limiting: https://developer.github.com/v3/#rate-limiting

agentzh · 2017-09-04T21:50:06Z

@doujiang24 @dndx @membphis Will you please review this PR for me? Many thanks!

dndx · 2017-09-07T07:39:18Z

lib/resty/limit/count.lua

+-- FIXME we have a (small) race-condition window between dict:get() and
+-- dict:set() across multiple nginx worker processes. The size of the
+-- window is proportional to the number of workers.
+function _M.incoming(self, key, commit)


Should we consider "commit" by default since that seems to be a more meaningful default behavior?

@dndx The commit thing is for combining multiple limiters together in the resty.limit.traffic module.

dndx · 2017-09-07T07:42:08Z

lib/resty/limit/count.md

+this method returns `nil` and the error string `"rejected"`.
+
+3. In addition, like the previous two cases, this method
+also returns a third return value indicating the next time to reset the `count` value. This 3nd return value can be used to monitor.


can be used to monitor -> can be used for monitoring

I think both forms are fine? I'm not a native speaker though :)

dndx · 2017-09-07T07:44:12Z

lib/resty/limit/count.lua

+    end
+
+    -- return remaining and time to reset
+    return 0, remaining, reset


I know 0 evaluates to true in Lua, but it just seems weird that we are using 0 as the return value for indicating the limit has not been exceeded.

Why don't we just return true and false for the first return value instead? That way we can just call the value "whether the access should be allowed" and is much easier to understand.

I think we just need to be consistent with other limiter modules here (not sure about other modules' code though).

the first return value is treated as delay in other limiter modules: https://github.com/openresty/lua-resty-limit-traffic/blob/master/lib/resty/limit/traffic.lua#L25.
So we should return 0 instead of true here.

dndx · 2017-09-07T07:45:17Z

lib/resty/limit/count.lua

+            return nil, "shdict abused by other users"
+        end
+        local rec = ffi_cast(const_rec_ptr_type, v)
+        reset = tonumber(rec.reset)


Is the tonumber here necessary since rec.reset was already declared as an integer type?

@dndx It's still needed I guess since an integer typed cdata is still quite different from a Lua number object. Usually we do not want to return cdata objects to the caller when a number is sufficient.

dndx · 2017-09-07T07:59:51Z

lib/resty/limit/count.lua

+end
+
+
+-- sees an new incoming event


I think the race condition here is a big concern as we could be seeing a lot of requests slipping through if the number of worker is high and there are a sudden spike of traffic.

Maybe use https://github.com/openresty/lua-resty-lock to guard the critical section?

Another thought. This whole algorithm could be implemented much more efficiently and elegantly if ngx.shdict.incr() supports preservation of expiration timers and provides API to get the remaining TTL for a key.

Currently, calling incr() on any value forces it's exptime to 0 which makes it never expire. If we support an option to allow incr() to not reset exptime and provides ngx.shared.get_ttl then this algorithm can be implemented lock-free (in Lua land) with no need for FFI hacking.

Although in the short term using explicit locking should works fine.

@agentzh what's your thought on this?

@dndx I gave a stab at shm:ttl() here: openresty/lua-resty-core#140 and am willing to work on incr() as well if we decide to go that way.

Yeah, better enhance the incr() method wrt ttl handling.

@dndx I used incr instead of FFI. please have a look.

membphis · 2017-09-11T09:46:01Z

lib/resty/limit/count.md

+                if not delay then
+                    if err == "rejected" then
+                        ngx.header["X-RateLimit-Limit"] = "5000"
+                        gx.header["X-RateLimit-Reset"] = reset


moonming · 2017-09-12T09:57:33Z

@agentzh @dndx can we merge this PR now?
I will create a new PR to fix race condition after incr() support ttl.

agentzh

The race condition here is just too big to ignore. We should merge the shdict:expire() PR first.

agentzh · 2017-09-12T16:43:16Z

lib/resty/limit/count.lua

+
+    return 0, remaining, reset
+end
+


I think we should use 2 blank lines to separate Lua function definitions consistently in this Lua library. Please fix other similar places as well.

agentzh · 2017-09-12T16:46:51Z

lib/resty/limit/count.lua

+
+    else
+        remaining = limit - 1
+        reset = now + window


reset is confusing, maybe new_ttl is a better name here?

moonming · 2017-09-20T13:09:36Z

@agentzh fixed the race condition by using the shdict:expire(). please have a look, thx.

agentzh · 2017-09-20T18:51:40Z

lib/resty/limit/count.lua

+    assert(key)
+    local dict = self.dict
+
+    local remaining, err = dict:incr(key, 1)


There is a small race condition here between commit() and uncommit in which time window the key is expired. Maybe we should not return an error here when the err is "not found"?

Yes I think we should just get again and return whatever the new value is. The old window has expired and no need to incr it anymore.

@agentzh @dndx fixed race condition. please have a look.

agentzh · 2017-09-20T18:52:09Z

lib/resty/limit/count.md

+                if not delay then
+                    if err == "rejected" then
+                        ngx.header["X-RateLimit-Limit"] = "5000"
+                        ngx.header["X-RateLimit-Remaining"] = 0


Hard-coded remaining?

local delay, err = lim:incoming(key, true)

returns nil, "rejected" here, and remaining must be 0 in this situation.

agentzh · 2017-09-20T18:52:55Z

@thibaultcha @dndx Will you please review the new version of this PR? Thanks!

dndx · 2017-09-20T19:18:40Z

lib/resty/limit/count.lua

+        if not remaining then
+            return nil, err
+        end
+        if remaining == limit - 1 then


Style: need a newline here.

There is a race potential here when key expires between incr and expire. Should run this part again when incr succeeds but expire errors with 'not found'.

dndx · 2017-09-20T19:20:06Z

lib/resty/limit/count.lua

+        end
+
+    else
+        remaining = (dict:get(key) or 0) - 1


Here, should it be remaining = (dict:get(key) or limit) - 1 instead when key does not exist? Otherwise a new window will be counted as -1 and be rejected.

Still need to fix this.

dndx · 2017-09-20T19:26:00Z

lib/resty/limit/count.lua

+    assert(key)
+    local dict = self.dict
+
+    local remaining, err = dict:incr(key, 1)


Yes I think we should just get again and return whatever the new value is. The old window has expired and no need to incr it anymore.

thibaultcha

Nothing much to say on the logic anymore, good job! I did find a few typos or mistakes in the docs and tests though, hope it helps!

thibaultcha · 2017-09-21T03:22:52Z

lib/resty/limit/count.md

+This Lua module's implementation is similar to [GitHub API Rate Limiting](https://developer.github.com/v3/#rate-limiting) But this Lua
+module is flexible in that it can be configured with different rate.
+
+This module depands on [lua-resty-core](https://github.com/openresty/lua-resty-core), so you should enable it like the following codes:


typo: depends

" you should enable it like so:" would probably read better?

thibaultcha · 2017-09-21T03:23:02Z

lib/resty/limit/count.md

+rate by a fixed number of requests in given time window.
+
+This Lua module's implementation is similar to [GitHub API Rate Limiting](https://developer.github.com/v3/#rate-limiting) But this Lua
+module is flexible in that it can be configured with different rate.


different rates (plural) maybe? Or window sizes?

thibaultcha · 2017-09-21T03:23:42Z

lib/resty/limit/count.md

+```nginx
+init_by_lua_block {
+        require "resty.core"
+    }


The indentation seems to be off here

thibaultcha · 2017-09-21T03:25:02Z

lib/resty/limit/count.md

+
+* `count` is the specified number of requests threshold.
+
+* `time_window` is the time window in second before the request count is reset.


"in seconds" (plural)?

thibaultcha · 2017-09-21T03:47:07Z

lib/resty/limit/count.md

+The return values depend on the following cases:
+
+1. If the request does not exceed the `count` value specified in the [new](#new) method, then
+this method returns `0` as the delay as well as remaining count of allowed requests at the current time (as the 2nd return value).


and the remaining count of allowed requests reads better maybe?

thibaultcha · 2017-09-21T03:52:32Z

t/count.t

+            local begin = ngx.time()
+
+            for i = 1, 4 do
+                local delay, err = lim:incoming("foo", i < 3 and true or false)


lim:incoming("foo", i < 3) should be sufficient, no?

thibaultcha · 2017-09-21T03:54:20Z

t/count.t

+--- http_config eval: $::HttpConfig
+--- config
+    location = /t {
+        content_by_lua '


The test suite uses the old content_by_lua directive instead of the modern content_by_lua_block alternative. I believe we have a rule for all new tests to be written with the later, more modern form across the OpenResty codebase.

thibaultcha · 2017-09-21T03:56:43Z

lib/resty/limit/count.md

+* [Description](#description)
+* [Methods](#methods)
+    * [new](#new)
+    * [incoming](#incoming)


We are missing the anchor to the uncommit method here.

thibaultcha · 2017-09-21T03:58:21Z

lib/resty/limit/count.md

+uncommit
+--------
+
+**syntax:** `remaining = obj:uncommit(key)`


I believe the signature is remaining, err here (according to the code) - granted, err is ignored in the traffic module, but other limiter modules document it nonetheless.

thibaultcha · 2017-09-21T04:04:00Z

t/traffic.t

+                            ", states: ", table.concat(states, ", "))
+                end
+                if i == 4 then
+                    ngx.sleep(1)


This extraneous delay doesn't seem necessary to make this test succeed, is it? Considering we are working in a 100s window.

@thibaultcha thanks for your review, fixed all of them.

dndx · 2017-09-21T04:48:19Z

lib/resty/limit/count.lua

+                    remaining, err = dict:incr(key, -1, limit)
+                    if not remaining then
+                        return nil, err
+                    end


Style: need a newline here.

dndx · 2017-09-21T04:50:43Z

lib/resty/limit/count.lua

+    local remaining, err = dict:incr(key, 1)
+    if not remaining then
+        if err == "not found" then
+            remaining = dict:get(key)


I think remaining should be limit here since we just got "not found" from incr().

@dndx fixed, thx.

dndx · 2017-09-21T04:50:55Z

lib/resty/limit/count.lua

+        end
+
+    else
+        remaining = (dict:get(key) or 0) - 1


Still need to fix this.

agentzh · 2017-09-22T05:30:17Z

Merged with minor edits. Thanks!

shawnzhu · 2017-09-24T22:05:27Z

@moonming @agentzh Thank you!

related apisix/apisix#3469

shawnzhu and others added 4 commits September 4, 2017 09:46

new module resty.limit.count

842a4c6

add uncommit() for resty.limit.count

cf68b62

feature: added test cases for limit-count and fixed bugs.

8240512

doc: added more documents.

5c86e6c

dndx requested changes Sep 7, 2017

View reviewed changes

bugfix: used incr instead of FFI.

a2f1f63

membphis reviewed Sep 11, 2017

View reviewed changes

typo: fixed typo.

7d61609

agentzh requested changes Sep 12, 2017

View reviewed changes

bugfix: used dict:incr and dict:expire to avoid race condition.

1779e72

moonming force-pushed the limit-count branch from e2bdcef to 1779e72 Compare September 20, 2017 12:53

agentzh requested changes Sep 20, 2017

View reviewed changes

dndx requested changes Sep 20, 2017

View reviewed changes

bugfix: fixed race condition.

b01ba27

thibaultcha reviewed Sep 21, 2017

View reviewed changes

dndx requested changes Sep 21, 2017

View reviewed changes

bugfix: fixed typo and test cases.

4074607

dndx approved these changes Sep 21, 2017

View reviewed changes

agentzh mentioned this pull request Sep 22, 2017

new module resty.limit.count #17

Closed

agentzh closed this Sep 22, 2017

y-tabata mentioned this pull request May 17, 2018

feature: resty.limit.count counts made instead of remaining requests #34

Open

monkeyDluffy6017 pushed a commit to monkeyDluffy6017/lua-resty-limit-traffic that referenced this pull request Apr 19, 2023

feat: add luarocks OPENSSL config (openresty#21)

dd6a775

related apisix/apisix#3469


		* `count` is the specified number of requests threshold.

		* `time_window` is the time window in second before the request count is reset.

feature: implemented resty.limit.count. #21

feature: implemented resty.limit.count. #21

Uh oh!

Conversation

moonming commented Sep 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

agentzh commented Sep 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

moonming commented Sep 12, 2017

Uh oh!

agentzh left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

moonming commented Sep 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agentzh commented Sep 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thibaultcha left a comment

feature: implemented `resty.limit.count`. #21

feature: implemented `resty.limit.count`. #21

moonming commented Sep 4, 2017 •

edited

Loading

agentzh left a comment •

edited

Loading