fix(ratelimit): race condition on refill #73
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reported in pelikan-io#72 as a panic on overflow in debug mode. Root cause is a race between a thread that has successfully refilled the token bucket and a thread that then steals the newly acquired token. This can be reliably triggered using the repro code in the original issue with the addition of a sleep in the refill function immediately before the return of `Ok(())`. What happens before this patch is that thread A refills the bucket successfully, meaning that refill result is `Ok(())` but by the time it loads the available count, there are no more tokens available (due to other threads acquiring tokens in parallel). This means that we do not early return a refill error indicating the duration until next refill, but instead proceed with available as zero. This fix adds another loop in the token acquisition path and breaks the inner loop when refill is successful but there are no tokens remaining. This edge-case results in an attempt to refill the token bucket again. When there is no race, we proceed in the inner compare exchange loop as originally written.
mihirn
approved these changes
Sep 28, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reported in #72 as a panic on overflow in debug mode. Root cause is a race between a thread that has successfully refilled the token bucket and a thread that then steals the newly acquired token.
This can be reliably triggered using the repro code in the original issue with the addition of a sleep in the refill function immediately before the return of
Ok(()).What happens before this patch is that thread A refills the bucket successfully, meaning that refill result is
Ok(())but by the time it loads the available count, there are no more tokens available (due to other threads acquiring tokens in parallel). This means that we do not early return a refill error indicating the duration until next refill, but instead proceed with available as zero.This fix adds another loop in the token acquisition path and breaks the inner loop when refill is successful but there are no tokens remaining. This edge-case results in an attempt to refill the token bucket again. When there is no race, we proceed in the inner compare exchange loop as originally written.