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

use() throws error instead of returning RollingLimiterResult #11

Open
legopin opened this issue May 10, 2022 · 3 comments
Open

use() throws error instead of returning RollingLimiterResult #11

legopin opened this issue May 10, 2022 · 3 comments

Comments

@legopin
Copy link

legopin commented May 10, 2022

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
Copy link
Contributor

STRML commented May 10, 2022

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
Copy link
Author

legopin commented May 10, 2022

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
Copy link
Author

legopin commented May 11, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants