search

IndraGunawan / api-rate-limit-bundle

Rate limits protection for api-platform resources.
MIT License
66 stars 10 forks source link

Handling race conditions #10

Open toby-griffiths opened 5 years ago

toby-griffiths commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

toby-griffiths commented 5 years ago

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