Repository navigation
Cache: rate limiting fixes, Redis/prefix safety, Cache::store(), Cache::tags() - #351
Merged
Merged
Conversation
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.
Pull Request Checklist
Summary
An audit of the cache code, with every suspected problem reproduced against the real classes (array, file and Redis) before changing anything. It found ten defects, mostly in the rate limiter and the Redis path, and two gaps: the config defines several stores but only the default was usable, and there were no tags. This PR fixes the defects and adds those features.
Bug fixes
Rate limiting (
ThrottleRequests,RateLimiter)$next($request)first and checked the limit afterwards, so a request over the limit still executed the controller (a throttled login was still evaluated). It also ran the handler a second time when the window had ended. With a limit of 2, the handler ran 5 times for 5 requests. The limit is now checked before$next, and only allowed requests reach it.sha1(user id or ip), so traffic on a 100/min API route used up a 5/min login limit. The limit is now part of the key. Routes with the samethrottle:N,Mstill share a counter per client.Retry-After/X-RateLimit-Resetwere wrong. The reset timer was pushed back on every allowed hit while the counter kept its original expiry (it reported 3s with 1s left). The timer is now set when the window opens and not moved;X-RateLimit-Resetis derived from it.CacheStoreincrement()/decrement()crashed on thearraystore.DateTime::createFromFormat('U', (string) $expiry)returnsfalsefor a float expiry, giving aTypeErrorinexpiresAt(); rate limiting broke on that driver. Expiry is now restored without string parsing.stash()could returnnullfor a value that had just expired, because it didhas()thenget(). It now reads once. See the stampede note below.forever()expired on Redis. The adapter applied its default lifetime (the shipped config has'ttl' => 3600) toexpiresAfter(null). Verified withTTLon the key. The default ttl is now applied byCacheStore, not the adapter.CacheLauncherCACHE_PREFIXmadeCache::clear()flush the whole Redis database (Symfony clears everything when namespace and prefix are both empty). The prefix is now never empty; it falls back todoppar_cache_.APP_NAME="My App"): the filesystem adapter only accepts[-+_.A-Za-z0-9]. Other characters are replaced with_.TypeError(nullpassed tocreateCustomAdapter(array)); it now throwsCache store [x] is not defined., and an unsupported driver gets its own message.options.parameters(password, database) were silently ignored; only the connection URL was read. They are now used, and the URL still takes precedence. A URL username is passed toAUTHfor ACL users.New
Cache::store('name')caching.stores. Stores are built once; every store can reach the others.store()without a name returns the current store.Cache::tags(string|array)TaggedCachewithset,forever,get,has,pull,delete/forget,stash,stashForeverandflush(). Built on Symfony'sTagAwareAdapterin its own sub-namespace.flush()removes items carrying any of the tags.Cache::pull($key, $default),Cache::missing($key)has().cache:clear --store=namettl,prefixcaching.stores.*.Tag semantics worth reviewing:
tags(), notget().tags()).Cache::clear()also clears tagged items.tags()throwsThis cache store does not support tags.if the adapter has noNamespacedPoolInterface(all built-in adapters do).stash/stashForever/stashWhennow call the adapter'sget($key, $callback)when it supports it, so the built-in adapters run the callback once for concurrent misses (a per-server lock; not distributed).Behaviour changes to be aware of
set()/setMultiple()/add()/stash()still get the configuredttl;forever()andstashForever()no longer do (that was the bug). Custom adapters that carry their own default lifetime are unaffected.options.parameters.databaseis now honoured. The shipped config saysenv('REDIS_DB', 1), so an install that never setREDIS_DBmoves its cache from database 0 to 1. An unset or empty value of the formREDIS_DB = ""stays on 0.ThrottleRequests:buildResponse()(protected) was replaced byrejectIfStillLimited(), andresolveRequestSignature()takes the limit as two extra optional arguments. A subclass overriding either needs updating.RateLimiterTestcases asserted the old timer reset on every hit; they now assert that an open window's timer is not touched.Files
Cache/CacheStore.php, newCache/TaggedCache.php,Cache/RateLimiter.phpLaunchers/CacheLauncher.phpMiddleware/ThrottleRequests.phpConsole/Commands/ClearCacheCommand.php,Support/Facades/Cache.php(docblock)Checklist