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

This package seems to be prone to race condition errors #188

Open
jason-kreinberg opened this issue Sep 21, 2021 · 0 comments
Open

This package seems to be prone to race condition errors #188

jason-kreinberg opened this issue Sep 21, 2021 · 0 comments

Comments

@jason-kreinberg
Copy link

  • I'm submitting a ...
    [x] bug report
    [ ] feature request
    [ ] question about the decisions made in the repository
    [ ] question about how to use this project

  • Summary

I have a GraphQL application that is rate limiting a resolver using your package. I've set it up using using "Option 3" as explained in your README, and I'm using Redis as my store. We also have the enableBatchRequestCache instance flag set to true as recommended in the README. Our issue is that intermittently this package is throwing the following uncaught exception from Redis:

ERR invalid expire time in set

When I dug through the source code to understand how this package is expiring keys, part of the TTL calculation is using the timestamps that it previously read from the store and subtracting the current time + window by the latest timestamp (see below). This is sometimes resulting in the package setting negative expiry times to Redis, which I've gathered is happening due to a race condition between multiple instances of our server getting and setting from the store in overlapping windows of time.

'EX',
Math.ceil((Date.now() + windowMs - Math.max(...timestamps)) / 1000),

Since this subtraction is not crucial behavior for our application to perform rate limiting, my team was planning to remove the arithmetic where we are subtracting from the expiry time to avoid the possibility of passing a negative expiry time. We know that this will mean that keys will not expire as optimally as they could, but we've decided that it's a fair tradeoff in favor of having these Redis exceptions occur.

As the authors of this package, do you foresee any issues with this? Are there other approaches you'd recommend? Ideally, we'd like to avoid race conditions altogether, but that seems to be unavoidable right now due to the nature of how we're asynchronously reading-then-writing to the Redis server from different instances of our application.

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

1 participant