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

[mikz]

by keeping track of requests that were made instead of remaining ones
it is possible to change the limit on the fly and share one counter
between several count limiters with different limits

It is possible all the details are not right, I didn't really understand the part of calling DICT:expire.
But I'd at least like to verify the approach and open a discussion.

ref: #23 (comment)

[y-tabata]

Referring to #21, the reason why this module used both incr and expire is that incr didn't support init_ttl yet, I think.
So if you use init_ttl here, L53-L59 are not necessary.

[mikz]

Indeed. I was using latest stable OpenResty and this got in 1.13.6.2 RC1. That explains why it was failing for me locally.

[y-tabata]

Even if :uncommit returns nil and err, this returns nil and rejected here. Is it ok?

[mikz]

Yep. The request was rejected because it was over the limit and the worst thing that can happen is that the count is going to be more than expected.

[y-tabata]

I think this should return err here if :uncommit returns err.
The count in shdict is already incremented in L47, so it is important to know whether :uncommit succeeded or not.
We can notice that the count equals the limit when this returns rejected, and the count may be grater than the limit when this returns err.

[mikz]

All the example code checks for "rejected" as the return value, so changing that would break existing code.
My point is that if the call is rejected once, all other requests to the same key will be rejected too. There is not much value in keeping that value exact anymore if the call was rejected. The key will get expired in some time and a new key created with a new bucket and 0 as a value.

[y-tabata]

I simply think this should be like the following.
Do we have the same understanding?

    if count > limit then
        if commit then
           ok, err = self:uncommit(key)
           if not ok then
               return nil, err
           end
        end
        return nil, "rejected"
    end

[mikz]

I don't. This method is expected to return nil, "rejected" when the request is rejected.

If something breaks during the uncommit and returns something else like return nil, "error" the code checking the return code as in the examples:

local delay, err = lim:incoming(key, true)
if err == "rejected" then
  return ngx.exit(503)
end

Will fail to act on it. It will have to handle it as a generic error - which is not true because the request was indeed rejected. So I believe the returned error should be "rejected" even if it failed to uncommit the limit.

And why it does not matter that much?

If the key is incremented to value 5 and the limit is 5, then the request comes through.
If the next request is incremented to 6 then it is rejected. If it fails to uncommit, the value stays 6. It has no way, to get back under 5 so it would be allowed again. The value will be > 5 until the key expires and gets deleted, then another request will set it to 1.

I can see this being an issue only in some credit based system, where you are actually decrementing the credits and every one counts. But then you need a more reliable system like a database with transactions.

[y-tabata]

And why it does not matter that much?

I think one of the important original purpose of this PR is to offer the flexibility of using different limits on the same key. ref: #23 (comment)

In this case, it does matter much.

However, I understand that it shouldn't be changed in order to follow the examples.

[mikz]

@y-tabata so what would happen if it fails to uncommit? It would still be incremented already.
I understand that it is important to try to keep the real value in some use cases. That's why I'm trying to fix it in this pull request.

Even our code is just checking the "rejected" or returning invalid configuration: https://github.com/3scale/apicast/blob/c4231f040b2ae37bc865ee64f01797b7c69afd98/gateway/src/apicast/policy/rate_limit/rate_limit.lua#L189-L197

Our disagreement is only about the return value. If it failed to uncommit, then there is not much that we can do. The request will be correctly rejected, but the value will be +1 for the future.
The same happens if we would return some different error message. I wonder how you'd want to handle that case to make the situation better somehow.

[y-tabata]

I think it is important for a user to notice there is some configuration issue.
It may be difficult to decrement the count automatically. However if the user notices that, the user can check the configuration or decrement the count manually.

By the way, the conn.lua returns err.

https://github.com/openresty/lua-resty-limit-traffic/blob/master/lib/resty/limit/conn.lua#L59-L62

[mikz]

The user also loses the information if the request was rejected or not.

But the argument of conn.lua returning err is a good one. Will update the PR.

[rainingmaster]

Hi @mikz
Sorry for our delay

I think this PR is good for us to share a counter for difference rate limit policy, just like limit:conn.

And could you split the incr to difference version like here, and the old version Openresty could be supported too.

[rainingmaster]

I think we should support the old version like core lib, maybe we can have two difference incoming for difference version of Openersty

[mikz]

@rainingmaster thanks for the feedback. I no longer work on this, but someone from https://github.com/3scale/APIcast could take over. /cc @eloycoto