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

Fixes out-of-sync TTLs after running decr() #13

Merged
merged 2 commits into from
Oct 21, 2015
Merged

Conversation

kwizzn
Copy link
Contributor

@kwizzn kwizzn commented Oct 20, 2015

Thanks for a great module! I would like to contribute a fix & test related to TTLs that run out of sync when the decr() function updates the count, leaving it with a different TTL than limit & reset.

@noamshemesh
Copy link
Collaborator

Thanks @kwizzn. seems weird that we missed it. I will dig deeper soon.

@kwizzn
Copy link
Contributor Author

kwizzn commented Oct 20, 2015

It is easy to reproduce if you trigger lots of requests around the second when the limit period ends. You'll have a timespan of around 300 ms (probably depending on your Redis) where you get new keys with limit & reset equalling 0.

@noamshemesh noamshemesh merged commit 9831466 into tj:master Oct 21, 2015
@noamshemesh
Copy link
Collaborator

Looks good. thanks @kwizzn

@cymen
Copy link

cymen commented Jan 27, 2016

I was looking into this and it turned out my problem was unrelated. But it's worth pointing out this uses the client date time instead of using the date time on the redis instance. I suspect it would be preferable to use the date time on redis as the client clocks can skew.

That is a big can of worms though but just wanted to mention it in case someone hits it.

@noamshemesh
Copy link
Collaborator

@cymen when you say client you mean a server that uses redis-client? I believe that if someone changes the server time, he could also change the redis server time, right?

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