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

Throttler not working as expected #2074

Closed
2 of 4 tasks
mukunda- opened this issue Aug 7, 2024 · 4 comments
Closed
2 of 4 tasks

Throttler not working as expected #2074

mukunda- opened this issue Aug 7, 2024 · 4 comments
Labels
bug Something isn't working

Comments

@mukunda-
Copy link

mukunda- commented Aug 7, 2024

Is there an existing issue for this?

  • I have searched the existing issues

Current behavior

When running the latest throttler (6.x), multiple requests are not blocked as expected.

Minimum reproduction code

https://github.com/mukunda-/nestjs-throttler-test

Steps to reproduce

  1. checkout
  2. npm i
  3. npm run test

Expected behavior

The throttler should not allow more requests until the existing blocks expire fully.

Package version

6.0.0

NestJS version

10.3.10

Node.js version

20.15.1

In which operating systems have you tested?

  • macOS
  • Windows
  • Linux

Other

image

@mukunda- mukunda- added the bug Something isn't working label Aug 7, 2024
@jmcdo29
Copy link
Member

jmcdo29 commented Aug 7, 2024

I think I found the issue here. Kinda surprised our test suite didn't catch it already, but we're using a Math.floor calculation on the time difference to see if we should allow further requests, and in this case it was returning a low enough number that it became 0 which meant that the reset was called and the request was allowed through.

Changing to Math.ceil seems to fix this, but I want to update the test suite to reflect this case as well.

@jmcdo29
Copy link
Member

jmcdo29 commented Aug 7, 2024

So, interestingly, it seems this error might be related to how supertest works. I'm still looking into it for the moment, but when I brought the test suite into the @nestjs/throttler suite and used the current http utility (a custom solution I'll be swapping out for pactum later), I couldn't get the 201 on subsequent requests. Not sure why supertest makes it act differently, but I'll keep looking

@jmcdo29
Copy link
Member

jmcdo29 commented Aug 7, 2024

Found the reason it wasn't failing in our test suite, there was a default blockDuration set at a different level with where I added the test. Explicitly set it and got the same result, and changing floor to ceil does indeed fix it and logically make sense. I'll have a fix out for this soon

@mukunda-
Copy link
Author

mukunda- commented Aug 8, 2024

Thanks for the quick fix. I tested it in my project and it looks good from my end. My main reason for upgrading to 6.x was the sliding window behavior which I think is a lot better than the previous behavior. 👍

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

No branches or pull requests

2 participants