docs: warn that UnstorageStore is not concurrency-safe - #68
Open
MathurAditya724 wants to merge 1 commit into
Open
docs: warn that UnstorageStore is not concurrency-safe#68MathurAditya724 wants to merge 1 commit into
MathurAditya724 wants to merge 1 commit into
Conversation
Unstorage has no atomic increment/CAS primitive, so UnstorageStore's increment()/decrement() do a non-atomic read-modify-write. Under concurrent requests for the same key, callers can all read the same hit count before any writes it back, under-counting and letting a client exceed the configured limit. Document this on the class and increment(), and point users to RedisStore (atomic Lua) for concurrent load. No behaviour change.
Semver Impact of This PR🟢 Patch (bug fixes) 📋 Changelog PreviewThis is how your changes will appear in the changelog. Bug Fixes 🐛
Documentation 📚
Internal Changes 🔧
Other
🤖 This preview updates automatically when you update the PR. |
This file contains hidden or 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
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.
Problem
UnstorageStore.increment()does a non-atomic read-modify-write:Unstorage exposes no atomic increment or compare-and-swap primitive, so under concurrent requests for the same key, callers can all read the same hit count before any of them writes it back. The counter under-counts and a client can exceed the configured limit. A reporter demonstrated 20/20 concurrent requests passing with
limit: 5(using a 50ms-latency driver wrapper).decrement()has the same shape.MemoryStore(synchronous) andRedisStore(atomic Lua) are not affected.Decision
There is no clean way to make this atomic through the unstorage API, and an in-process lock would only cover a single instance (not multi-instance deployments) while adding surprising serialization. Rather than ship a partial fix that implies a guarantee we can't keep, this PR documents the limitation and points users to
RedisStorefor correct counting under concurrent load.Change
Docs-only. Adds
@remarksto theUnstorageStoreclass and toincrement()describing the race, the latency window, the multi-instance case, and the recommended alternatives. No behaviour change.Reported alongside the RedisStore DECR issue. Thanks for the detailed PoC.