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

rate-limiter-flexible don't work since 1.11 #2034

Closed
yovanoc opened this issue Oct 17, 2023 · 16 comments · Fixed by #2080
Closed

rate-limiter-flexible don't work since 1.11 #2034

yovanoc opened this issue Oct 17, 2023 · 16 comments · Fixed by #2080
Assignees
Labels
bug Something isn't working

Comments

@yovanoc
Copy link

yovanoc commented Oct 17, 2023

Probably because of this: https://github.com/animir/node-rate-limiter-flexible/blob/846b5a28987f28e0e13b5ec7965def4aa39a22ab/lib/RateLimiterRedis.js#L4

when I log the .consume response it gives me big msBeforeNext

RateLimiterRes {
  _remainingPoints: 4991,
  _msBeforeNext: 281476674260404640,
  _consumedPoints: 9,
  _isFirstInDuration: false
}

dragonfly regression or was working before but an issue in the rate-limiter package ?

@yovanoc yovanoc added the bug Something isn't working label Oct 17, 2023
@romange
Copy link
Collaborator

romange commented Oct 17, 2023

@yovanoc is it possible to get a minimal reproducible example?

@yovanoc
Copy link
Author

yovanoc commented Oct 17, 2023

I can't right now but will try to do that later

@kostasrim
Copy link
Contributor

Small friendly ping/reminder @yovanoc :)

@yovanoc
Copy link
Author

yovanoc commented Oct 19, 2023

Sorry for the delay.

https://github.com/yovanoc/df-repro

@chakaz
Copy link
Collaborator

chakaz commented Oct 26, 2023

Interesting!
It seems like, for some reason, expiration setting via Lua doesn't work as intended. I'm looking into this.

chakaz added a commit that referenced this issue Oct 26, 2023
We used to set `time_now_ms_` only in the non-squashed execution path.

Fixes #2034
@chakaz
Copy link
Collaborator

chakaz commented Oct 26, 2023

Thanks @yovanoc for the interesting bug report, this is indeed a regression.
It should be fixed with #2080

@yovanoc
Copy link
Author

yovanoc commented Oct 26, 2023

oh nice! glad to help as I can

chakaz added a commit that referenced this issue Oct 27, 2023
We used to set `time_now_ms_` only in the non-squashed execution path.

Fixes #2034
@yovanoc
Copy link
Author

yovanoc commented Oct 28, 2023

happy to see this merged, but where can we see your push to prod routine, I see you have dragonfly, helm-dragonfly, and dragonfly-weekly, where its explained your releases process? thanks

@chakaz
Copy link
Collaborator

chakaz commented Oct 28, 2023

We release new versions every few weeks, you can follow our announcements on GitHub / Discord.

@yovanoc
Copy link
Author

yovanoc commented Oct 28, 2023

Yeah but actually If I understand correctly the k8s operator package it's still in 1.10 so I just wanted to know the pattern you follow between your packages

@chakaz
Copy link
Collaborator

chakaz commented Oct 29, 2023

Indeed, the operator still uses 1.10. We'll update it this week.
We manually update the operator to the next release a week or so after the binary release in case there are issues we did not catch in the release.

@romange
Copy link
Collaborator

romange commented Oct 29, 2023

We do not bundle k8s operator and Dragonfly releases. But you can always override theimage parameter and specify the version you want.

@yovanoc
Copy link
Author

yovanoc commented Nov 7, 2023

this issue still there in 1.12.1

@chakaz
Copy link
Collaborator

chakaz commented Nov 7, 2023

@yovanoc I just tried locally, by downloading the latest version, and I cannot reproduce this issue.
How do you run Dragonfly? Can you show the output of dragonfly --version, and the output of your repro script (i.e. npm run start)?

@yovanoc
Copy link
Author

yovanoc commented Nov 7, 2023

My bad I had to delete all the old keys.. because there was still there with these long expirations

@chakaz
Copy link
Collaborator

chakaz commented Nov 7, 2023

Ah, that makes sense :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants