You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
This repository has been archived by the owner on Feb 27, 2024. It is now read-only.
Hi there! We've identified a potential race condition when counting down rate limits. Let me explain:
List of actions
There are two actions that can are atomically executed for each request:
GET key: returns the current remaining count as a usize
DECRBY key 1: decrement the counter by 1
Say there's only 1 request left for the rate limiting. If two (probably more) requests are issued concurrently the following can happen:
req A: Get key -> returns 1
req B: Get key -> returns 1
req A: DECRBY key 1 -> key = 0
req B: DECRBY key 1 -> key = -1
What now?
The current code only deals with usize counters, so the behavior depends on the implementation of the stores, but overall I think the code is too optimistic for this scenario.
One solution would be to use an atomically safe Get + Decr, using locks for instance. But I think the cost is too high for the normal case.
I would rather document the possibility of this scenarion and I would recommend 2 changes to the current implementation:
use i32 instead of usize to express the counter to acknowledge this scenario
make sure the code checks that remaining <= 0 instead of remaining == 0
If you agree, I can contribute to make these changes. I wanted to make sure we agree and we didn't forget anything before moving on.
The text was updated successfully, but these errors were encountered:
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Hi there! We've identified a potential race condition when counting down rate limits. Let me explain:
List of actions
There are two actions that can are atomically executed for each request:
usize
Say there's only 1 request left for the rate limiting. If two (probably more) requests are issued concurrently the following can happen:
What now?
The current code only deals with
usize
counters, so the behavior depends on the implementation of the stores, but overall I think the code is too optimistic for this scenario.One solution would be to use an atomically safe Get + Decr, using locks for instance. But I think the cost is too high for the normal case.
I would rather document the possibility of this scenarion and I would recommend 2 changes to the current implementation:
i32
instead ofusize
to express the counter to acknowledge this scenarioremaining <= 0
instead ofremaining == 0
If you agree, I can contribute to make these changes. I wanted to make sure we agree and we didn't forget anything before moving on.
The text was updated successfully, but these errors were encountered: