[12.x] Add Cache::funnel() for concurrency limiting with any cache driver#58439
Draft
mathiasgrimm wants to merge 13 commits into12.xfrom
Draft
[12.x] Add Cache::funnel() for concurrency limiting with any cache driver#58439mathiasgrimm wants to merge 13 commits into12.xfrom
Cache::funnel() for concurrency limiting with any cache driver#58439mathiasgrimm wants to merge 13 commits into12.xfrom
Conversation
Extracts concurrency limiter functionality from Redis-specific implementation to a cache-driver-agnostic base in Illuminate\Cache. - Add funnel() method to Cache Repository - Create base ConcurrencyLimiter and ConcurrencyLimiterBuilder classes - Refactor Redis limiters to extend new base classes - Add LimiterTimeoutException contract in Illuminate\Contracts\Cache
Cache::funnel() for concurrency limiting with any cache driver
# Conflicts: # src/Illuminate/Support/Facades/Cache.php
| use Illuminate\Cache\Limiters\LimiterTimeoutException as BaseLimiterTimeoutException; | ||
|
|
||
| class LimiterTimeoutException extends Exception | ||
| class LimiterTimeoutException extends BaseLimiterTimeoutException |
Member
There was a problem hiding this comment.
I don't think this should extend something from the cache component. I would leave it as a separate exception.
| */ | ||
| public function funnel($name) | ||
| { | ||
| return new ConcurrencyLimiterBuilder($this, enum_value($name)); |
Member
There was a problem hiding this comment.
This should probably make sure the cache actually supports locks.
| use Throwable; | ||
|
|
||
| class ConcurrencyLimiter | ||
| class ConcurrencyLimiter extends BaseConcurrencyLimiter |
Member
There was a problem hiding this comment.
I don't think this should extend the cache component stuff.
Member
|
I think this is a cool feature but I would just make it separate from the Redis stuff even though there will be duplication. 👍 |
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.
arrayis commonly used) and for environments using database or other drivers.Usage
Changes
Extracts concurrency limiter functionality from Redis-specific implementation to a cache-driver-agnostic base in
Illuminate\Cache.funnel()method to Cache RepositoryConcurrencyLimiterandConcurrencyLimiterBuilderclasses inIlluminate\Cache\LimitersLimiterTimeoutExceptioninIlluminate\Cache\LimitersConcurrencyLimiterreuses parent'sblock()vianewTimeoutException()factory methodConcurrencyLimiterBuilderreuses parent'sthen()since exception hierarchy allows itDesign Decisions
Redis
ConcurrencyLimiterdoesn't callparent::__construct()The Redis subclass has a fundamentally different dependency (
ConnectionvsLockProvider). This is consistent with how cache stores likeRedisStore,MemcachedStore, andArrayStoreextendTaggableStorewithout calling parent constructor.block()is shared vianewTimeoutException()factory methodThe base and Redis
block()methods were identical except for which exception they throw. AnewTimeoutException()factory method (overridden by Redis) eliminates the duplication. This follows the framework's established factory method pattern (newHasOne(),newMorphTo(),newPendingRequest(), etc.).No
LockProviderguard onfunnel()Consistent with the existing
withoutOverlapping()method which also calls$store->lock()without checking. All built-in cache stores implementLockProvider.DurationLimiternot extractedKept out of scope —
Cache::throttle()can be a follow-up PR.Manual Testing
This is covered by the Integration tests, but could be useful for some extra manual testing.