Comments (18)
Thanks @thoop
the last line that you mention strips the millis as you say.
do you call the rate limiter with duration: 1000
or with duration: 1
?
from node-ratelimiter.
duration: 1000
is the lowest I call it with and it can be up to 60000
. I'm testing some changes to keep the milliseconds on a fork that looks to be a little better for how hard I'm hitting it. I'll submit a pull request once I'm sure it's working well and stable if you're interested. Not sure what you want to do about that error message being wrapped in two arrays or the best way to even handle that error.
Thanks for the quick response!
from node-ratelimiter.
I tried to write a test that reproduce this issue, and it passed.
var limit = new Limiter({
duration: 1000,
max: 2,
id: 'something',
db: db
});
limit.get(function(err, res) {
res.remaining.should.equal(2);
});
limit.get(function(err, res) {
res.remaining.should.equal(1);
done();
});
any thoughts?
from node-ratelimiter.
Let me see if I can reproduce it. Are you actually using redis in your test or do you have it mocked out?
from node-ratelimiter.
actual redis. the tests are in test directory, you can run them by executing mocha test/
from node-ratelimiter.
Sorry I haven't gotten to this yet. I've been swamped lately. I did run those tests and everything looked good so I want to look into this more once I have a chance!
from node-ratelimiter.
I modified the test and got it to fail reliably around the edge case of the seconds/milliseconds.
In the last test of test/index.js
, change it to be this:
// Warm up and prepare the data.
while( ((Date.now() + 1000) / 1000 | 0) * 1000 - Date.now() > 10 ) {}
limits[0].get(function(err, res) {
if (err) {
done(err);
} else {
res.remaining.should.equal(left--);
// Simulate multiple concurrent requests.
limits.forEach(function(limit) {
limit.get(complete);
});
}
});
You can see the while loop I added. It basically causes the test to wait until it's going to be on the cusp of a rounding issue. It looks like the rounding is causing redis to cleanup the counts before the millisecond resolution. We're seeing issues with this when running on AWS with Elasticache because we get millisecond response times.
By removing the rounding, it looks to fix that issue...but it breaks a few other tests because now it's giving milliseconds back from the calls and not seconds. Thoughts?
from node-ratelimiter.
Hi thoop,
I won't be near a computer until next month. I will take a look then.
Thanks though!
On Wed, Sep 16, 2015, 10:06 Todd Hooper [email protected] wrote:
I modified the test and got it to fail reliably around the edge case of
the seconds/milliseconds.In the last test of test/index.js, change it to be this:
// Warm up and prepare the data.
while( ((Date.now() + 1000) / 1000 | 0) * 1000 - Date.now() > 10 ) {}limits[0].get(function(err, res) { if (err) { done(err); } else { res.remaining.should.equal(left--); // Simulate multiple concurrent requests. limits.forEach(function(limit) { limit.get(complete); }); } });
You can see the while loop I added. It basically causes the test to wait
until it's going to be on the cusp of a rounding issue. It looks like the
rounding is causing redis to cleanup the counts before the millisecond
resolution. We're seeing issues with this when running on AWS with
Elasticache because we get millisecond response times.By removing the rounding, it looks to fix that issue...but it breaks a few
other tests because now it's giving milliseconds back from the calls and
not seconds. Thoughts?—
Reply to this email directly or view it on GitHub
#11 (comment).
from node-ratelimiter.
Sorry for the delay.
I think I found an easy way to reproduce it, take a look at 499e3fc. please checkout this commit and tell me if it reproduce your bug or something else (last test fails without the while addition).
I will add a constant test with fakeredis right after we'll merge #12.
from node-ratelimiter.
I'll look into this soon. I don't think just changing redis to fakeredis would make it fail for the same reason unless fakeredis handles expiration differently.
from node-ratelimiter.
Can you check please if #13 solves your issue?
from node-ratelimiter.
Sure, thanks! I'll report back.
from node-ratelimiter.
closing due to inactivity
from node-ratelimiter.
I am able to recreate this with the latest version from npm (3.3.1).
{ max: 4, duration: 1000 }
const Limiter = require('ratelimiter')
const rateLimiters = {}
const delay = (ms) => new Promise((resolve, reject) => setTimeout(resolve, ms))
const rateLimit = async (bucketName, clientId) => {
const bucketDefinition = bucketDefinitions[bucketName]
if (!bucketDefinition) {
throw new Error(`Bucket definition not found: ${bucketName}`)
}
const key = `${bucketName}:${clientId}`
if (!rateLimiters[key]) {
rateLimiters[key] = new Limiter({
db: cacheModule.getConnection(), // redis, not ioredis or mock-redis
id: key,
max: bucketDefinition.max,
duration: bucketDefinition.duration
})
}
const limit = rateLimiters[key]
const delta = await new Promise((resolve, reject) => {
limit.get((err, rate) => {
if (err) {
return reject(err)
}
if (rate.remaining) {
return resolve(0)
}
let delta = (rate.reset * 1000) - Date.now() | 0
// watch out for weird negative delta problem
if (delta <= 0) {
delta = Math.ceil(bucketDefinition.duration / bucketDefinition.max)
}
logger.log({
level: 'debug',
message: 'Rate limited',
key,
rate,
delta
})
return resolve(delta)
})
})
if (delta) {
await delay(delta)
return rateLimit(bucketName, clientId)
}
}
from node-ratelimiter.
just to make sure I understand - the reset
is set to the floor value of the second even though Date.now()
is already bigger?
For example for epoch 1441268386507
the reset is 1441268386000
already when setting it?
Just to make sure, is it reproducible with duration 2000? what about 1500?
from node-ratelimiter.
{
"level": "debug",
"message": "Rate limited",
"key": "foo:bar",
"rate": {
"remaining": 0,
"reset": 1568990156,
"resetMs": 1568990156474,
"total": 20
},
"delta": -188,
"file": "/home/brandon/foo/rateLimit.js",
"line": "39",
"timestamp": "2019-09-20T14:35:56.188Z"
}
does that help you, or do you need me to log Date.now()
too? I changed the bucket definition to { max: 20, duration: 5000 }
and it still happens.
from node-ratelimiter.
can you use the resetMs instead?
from node-ratelimiter.
https://github.com/tj/node-ratelimiter#example
That code is from your example.
I switched to a different package, thanks!
from node-ratelimiter.
Related Issues (20)
- Reatelimiter not able to create redis key HOT 3
- TTL instead of multiple keys. HOT 12
- Reset functionality HOT 1
- how to use ? HOT 1
- Compatibility issue with ioredis-mock HOT 3
- Race condition when using `async.times`
- more than one limiter HOT 3
- limit.reset always 1499329365 HOT 1
- How To use? Update Readme
- Possible units confusion HOT 1
- Misleading reset value HOT 2
- limiter never frees up HOT 3
- add zremrangebyrank HOT 3
- Add type definitions HOT 1
- Memory consumption HOT 2
- opts tidy is a bug? HOT 3
- Any `get` call will further delay a positive `remaining` value
- Rate limit max value HOT 1
- Throttlers and rate limiters, how they should be implemented HOT 2
- node-redis 4.x support
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from node-ratelimiter.