BitMEX / node-redis-token-bucket-ratelimiter

MIT License
34 stars 11 forks source link

use() throws error instead of returning RollingLimiterResult #11

Open legopin opened 2 years ago

legopin commented 2 years ago

BASIC INFO

version used: "redis-token-bucket-ratelimiter": "^0.5.1"

EXPECTED

When I await limiter.use(), I expected to get RollingLimiterResult object. As documented on readme page example Example below uses `limit.rejected' to tell if a request was rejected

async function rateLimitMiddleware(req, res, next) {
  const id = getUserId(req);
  const limit = await requestLimiter.use(id);

  // Your max tokens
  res.set('X-RateLimit-Limit', String(limit.limit));
  // Remaining tokens; this continually refills
  res.set('X-RateLimit-Remaining', String(limit.remaining));
  // The time at which it's valid to do the same request again; this is almost always now()
  const retrySec = Math.ceil(limit.retryDelta / 1000);
  res.set(
    'X-RateLimit-Reset',
    String(Math.ceil(Date.now() / 1000) + retrySec)
  );

  if (limit.rejected) {
    res.set('Retry-After', String(retrySec));
    res.status(429).json({
      error: {
        message: `Rate limit exceeded, retry in ${retrySec} seconds.`,
        name: 'RateLimitError',
      },
    });
    return;
  }
  next();
}

ACTUAL

When use() amount exceeds limit, the function throws error instead

As shown in code here: https://github.com/BitMEX/node-redis-token-bucket-ratelimiter/blob/master/rollingLimit.js#L53

if (amount > this.limit) throw new Error(`amount must be < limit (${this.limit})`);

Which one is the correct behavior. Thanks

STRML commented 2 years ago

I think you are correct that this is unexpected behavior, as it is reasonable to set someone's limit to 0 if their account is, say, disabled. In that case, the error thrown rather than a rejection is quite unexpected and will require an additional catch() clause to correctly trap.

I assume this is what you are doing? Can you tell me more about your use case?

legopin commented 2 years ago

Thanks for the quick reply. In our case we are not actually setting the limit to zero.

We are trying to rate limit a GraphQL API endpoint using query complexity calculations. Since graphQL allows arbitrary queries to be written, we plan to set our api limit based on points per minute. Therefore each api call cost is larger than one and deducted from the limit. Occasionally a single query might be larger than the total limit for that time, that's what triggered the error.

const limiter = new RollingLimit({
  interval:60000,
  limit: 100,
  force: false,
});
// when complexityScore > 100 the limiter throws error instead of reject
 const limitRes =  await limiter.use(storeId, complexityScore);
if (limitRes.rejected) {
  //set header and things
}
legopin commented 2 years ago

My current workaround is to implement a pre-check to see if the amount to be used is larger than the limit