Treat null cache value as a miss to fix has/get race#520
Open
mattiasgeniar wants to merge 1 commit intospatie:mainfrom
Open
Treat null cache value as a miss to fix has/get race#520mattiasgeniar wants to merge 1 commit intospatie:mainfrom
mattiasgeniar wants to merge 1 commit intospatie:mainfrom
Conversation
The middleware called hasBeenCached() then getCachedResponseFor() as two separate cache calls. If the key expired, was evicted, or was removed by a concurrent forget() between the two, has() returned true and get() returned null. The repository then evaluated unserialize($cache->get($key) ?? '') and threw CouldNotUnserialize, which the middleware caught, served a fresh response, and reported as an exception, surfacing what is normal behaviour on any cache with a finite TTL as noise. Collapse the two calls into a single atomic get(): null means miss, Response means hit. The race window is closed and we save a cache round-trip per cached request. hasBeenCached() stays on the public API for external callers.
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
1 participant
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.
CacheResponse::getCachedResponse()does two separate cache calls —hasBeenCached()followed by
getCachedResponseFor(). The race:has()returnstrue— the key exists.forget()/responsecache:clear.get()returnsnull.ResponseCacheRepository::get()evaluatesunserialize($cache->get($key) ?? '')→unserialize('')→CouldNotUnserialize.`report().The end result is correct, but normal behaviour on any cache with a finite TTL under moderate traffic gets surfaced as an exception. This was hit in production: traces showed a cache hit and miss on the same key microseconds apart, on a TTL boundary.
The fix collapses the two calls into a single atomic
get():nullmeans miss,Responsemeans hit. The race window is closed and we save a cache round-trip per cached request.hasBeenCached()stays on the public API for external callers.has+get)get)report()on race