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

Handling race conditions #10

Open
toby-griffiths opened this issue Nov 12, 2018 · 3 comments
Open

Handling race conditions #10

toby-griffiths opened this issue Nov 12, 2018 · 3 comments

Comments

@toby-griffiths
Copy link

Hi there,

I've just been pointed to your package by @dunglas after submitting a PR for rate limits to the api-platform/core package.

I like the look of your offering, as it's more complete than the PR I submitted to the api-platform/core, however I think that it could be improved to better handle race conditions.

In the RateLimitHandler you read from the cache, check, and update. You would be better of using a cache key that includes all the rate limit details, and a timeframe string for the desired timeframe, incrementing the cache (first trying to add it, but ignoring errors when it already exists), and then verifying whether the resulting value is greater than the rate limit. This avoids issues with the cache value being updated between the read & the write.

I'm happy to submit a PR to update, if you're happy with me doing so?

@IndraGunawan
Copy link
Owner

Hi @toby-griffiths ,
yeah, current implementation doesn't handle race condition gracefully.
i'm very happy if you want to make this bundle much better :)

@toby-griffiths
Copy link
Author

Great stuff. I'll try to get around to it in the next couple of weeks.

@toby-griffiths
Copy link
Author

Sorry it's taken so long to pick this up. It is still on my radar, when I can find a moment.

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