jhurliman / node-rate-limiter

A generic rate limiter for node.js. Useful for API clients, web crawling, or other tasks that need to be throttled
MIT License
1.51k stars 135 forks source link

Set current tokens remaining in constructor #18

Closed hongkongkiwi closed 3 years ago

hongkongkiwi commented 9 years ago

In my case, I create the limiter object after the first call using the returned header.

So for now I create the object, then run a removeTokens function and ignore the result. It would be really fantastic if I could simply pass the remaining tokens for this interval into the constructor, that way I wouldn't have to make a simply removeTokens method with a blank callback.

Such as Limiter(120, 'minute', 119);

msiebuhr commented 8 years ago

Yup. I have a somewhat similar use-case with a remote that allows quite agressive bursting and then a trickle of requests. So I have huge initial capacity and then a low-fill rate afterwards.

I tried hacking it, but it does seem the count is kept in a closure, so I can't get to it:

var limiter = new RateLimiter(1, 60 * 1000);
limiter.tokenBucket.content = 10;

Edit:

After hacking around a bit, I think I've found the offending part:

if (count > this.tokenBucket.tokensPerInterval - this.tokensThisInterval) {
  ...
}

In particular, it compares the requested amount of tokens (count) with how many tokens it gets per interval minus how many it has already used this interval.

Nowhere does it actually check how many tokens there are left in the bucket...

jhurliman commented 6 years ago

The check for how many tokens are left in the bucket happens one level deeper, in the call to this.tokenBucket.removeTokens(). A proposed fix for this was submitted in https://github.com/jhurliman/node-rate-limiter/pull/41 but I'd prefer to add another parameter to the constructor as suggested here rather than changing the default behavior. If someone puts together a PR for this I'm happy to review and merge.

SirR4T commented 6 years ago

@hongkongkiwi , @jhurliman time to close this issue?

jhurliman commented 6 years ago

@SirR4T it's a valid request that no one has implemented yet; I think we can keep it open.

SirR4T commented 6 years ago

Sure, misunderstood the requirement first time round.

At first glance, this looks as simple as:

But then, this will not ensure that getTokensRemaining() returns startWithTokens (Before any calls to removeTokens() are made, say).

Is that ok? Or should this be fixed at the TokenBucket level?

bamnet commented 5 years ago

I'd find it helpful if the TokenBucket constructor took an initial bucket content, or had a flag to indicate the bucket should start full instead of the current start empty behavior.

jhurliman commented 3 years ago

TokenBucket content is now exposed in 2.0.0