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

Return accurate reset time for each limited call #44

Merged
merged 3 commits into from
Jul 21, 2018

Conversation

xdmnl
Copy link
Contributor

@xdmnl xdmnl commented Jul 5, 2018

Since the rate limit strategy saves all the calls in Redis, the reset time returned needs to depend on the oldest call in the range of the limiter.
In practice, the oldest call to use to calculate the reset time is the max-th element from the end or the first element of the sorted set.

Closes #43.

@xdmnl xdmnl mentioned this pull request Jul 5, 2018
@xdmnl
Copy link
Contributor Author

xdmnl commented Jul 17, 2018

@noamshemesh any feedback on this?

@noamshemesh
Copy link
Collaborator

noamshemesh commented Jul 17, 2018

Sorry probably missed the previous mail.

Just to make sure we're on the same page here, taking your example:

time sorted set result
t0.0 [0.0] {remaining: 2, reset: 3.0}
t0.1 [0.0, 0.1] {remaining: 1, reset: 3.1}
t0.2 [0.0, 0.1, 0.2] {remaining: 0, reset: 3.0}
t0.3 [0.1, 0.2, 0.3] {remaining: 0, reset: 3.1}
t3.0 [0.2, 0.3, 3.0] {remaining: 0, reset: 3.2}
t3.4 [3.0, 3.1] {remaining: 1, reset: 6.1}

Unless I'm missing something, that's pretty weird. I wouldn't expect the remaining to be on one call bigger than a later call (t0.1 -> t0.2).

What do you think?

@xdmnl
Copy link
Contributor Author

xdmnl commented Jul 17, 2018

@noamshemesh What I would like to have is a reset value computed using the configured limit to be accurate. It is illustrated by the test I added
Since the limiter keeps track of all the timestamps it was accessed, it makes sense that a limited request should have a reset value bigger than the previous one.

I couldn't find where this example come from but, with this PR, the limiter should look something like this:

limit =2
duration = 3
time sorted set result
t0.0 [0.0] {remaining: 2, reset: 3.0}
t0.1 [0.0, 0.1] {remaining: 1, reset: 3.0}
t0.2 [0.0, 0.1, 0.2] {remaining: 0, reset: 3.1}
t3.0 [0.1, 0.2, 3.0] {remaining: 0, reset: 3.2}
t3.1 [0.2, 3.0, 3.1] {remaining: 0, reset: 6.0}
t3.4 [3.0, 3.1, 3.4] {remaining: 0, reset: 6.1}

As you can see, all limited requests (t0.2, t3.0, t3.1, t3.4) return a reset that depends on the oldest timestamps in the last 2 received requests.

An alternative solution could be to use zremrangebyscore before trying to fetch the oldest timestamp to always keep a sorted set of maximum limit timestamps. The operation complexity is the same as adding a second zrange but has the advantage to keep the size of the sorted sets as low as possible. I can update the PR if you think it is a better solution.

var count = parseInt(Array.isArray(res[0]) ? res[1][1] : res[1]);
var oldest = parseInt(Array.isArray(res[0]) ? res[3][1] : res[3]);

var isIoRedis = Array.isArray(res[0]);
Copy link
Contributor

Choose a reason for hiding this comment

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

oh, now I understood better why this check is necessary. I removed it from the promise equivalent package:

https://github.com/tj/node-ratelimiter/pull/44/files#diff-168726dbe96b3ce427e7fedce31bb0bcR77

Making it a bit faster, thanks 🙂

(I will add this PR as well)

@noamshemesh
Copy link
Collaborator

You're right. Merging. Thanks for the PR

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.

3 participants