api-rate-limit-bundle icon indicating copy to clipboard operation
api-rate-limit-bundle copied to clipboard

Handling race conditions

Open toby-griffiths opened this issue 5 years ago • 3 comments

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?

toby-griffiths avatar Nov 12 '18 10:11 toby-griffiths

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 :)

IndraGunawan avatar Nov 14 '18 11:11 IndraGunawan

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

toby-griffiths avatar Nov 20 '18 12:11 toby-griffiths

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

toby-griffiths avatar Feb 20 '19 11:02 toby-griffiths