[12.x] Ability to refresh cache locks #58349
[12.x] Ability to refresh cache locks #58349
8000Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds the ability to atomically refresh cache locks across all cache store implementations, allowing long-running operations to extend lock expiration times without releasing and re-acquiring locks. This prevents indefinite locking due to bugs, crashes, or server issues when using short TTLs that get extended as operations progress.
Changes:
- Added
refresh($seconds = null)method to all lock implementations (Redis, PhpRedis, Memcached, File, Database, DynamoDB, Array, and NoLock) - Added Lua script for atomic refresh operations in Redis
- Added integration tests for File, Redis, and Database locks to verify refresh functionality
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Illuminate/Cache/Lock.php | Added base refresh() method that throws RuntimeException by default |
| src/Illuminate/Cache/RedisLock.php | Implemented atomic refresh using Lua script |
| src/Illuminate/Cache/PhpRedisLock.php | Implemented atomic refresh using Lua script with PhpRedis argument packing |
| src/Illuminate/Cache/MemcachedLock.php | Implemented refresh using Memcached CAS for atomicity |
| src/Illuminate/Cache/FileLock.php | Implemented refresh delegating to FileStore's refreshIfOwned |
| src/Illuminate/Cache/FileStore.php | Added refreshIfOwned method with file locking for atomicity |
| src/Illuminate/Cache/DatabaseLock.php | Implemented refresh with database update query |
| src/Illuminate/Cache/DynamoDbLock.php | Implemented refresh delegating to DynamoDbStore's refreshIfOwned |
| src/Illuminate/Cache/DynamoDbStore.php | Added refreshIfOwned method with conditional expression for atomicity |
| src/Illuminate/Cache/ArrayLock.php | Implemented refresh for in-memory lock store |
| src/Illuminate/Cache/NoLock.php | Implemented no-op refresh that always returns true |
| src/Illuminate/Cache/LuaScripts.php | Added refreshLock Lua script for atomic Redis operations |
| tests/Integration/Cache/FileCacheLockTest.php | Added tests for file lock refresh functionality |
| tests/Integration/Cache/RedisCacheLockTest.php | Added tests for Redis lock refresh functionality |
| tests/Integration/Database/DatabaseLockTest.php | Added tests for database lock refresh functionality |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public function refresh($seconds = null) | ||
| { | ||
| if (! $this->isOwnedByCurrentProcess()) { | ||
| return false; | ||
| } | ||
|
|
||
| $seconds ??= $this->seconds; | ||
|
|
||
| $this->store->locks[$this->name]['expiresAt'] = $seconds === 0 ? null : Carbon::now()->addSeconds($seconds); | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
The ArrayLock refresh functionality lacks test coverage. While CacheArrayStoreTest has comprehensive tests for lock acquisition and release, there are no tests for the newly added refresh method. Consider adding test cases that verify: (1) successful refresh, (2) refresh preventing acquisition by another process, (3) refresh failure when attempted by another owner, (4) refresh with default seconds parameter, and (5) refresh behavior with expired locks.
| public function refresh($seconds = null) | ||
| { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
The NoLock refresh functionality lacks test coverage. While NoLockTest has tests for lock acquisition and release, there are no tests for the newly added refresh method. Since NoLock always succeeds, add a test case to verify that refresh always returns true, maintaining the no-op behavior of this lock implementation.
| public function testLockCanBeRefreshed() | ||
| { | ||
| $lock = Cache::driver('database')->lock('foo', 10); | ||
| $this->assertTrue($lock->get()); | ||
|
|
||
| // Refresh the lock for another 20 seconds | ||
| $this->assertTrue($lock->refresh(20)); | ||
|
|
||
| // Lock should still be held | ||
| $this->assertFalse(Cache::driver('database')->lock('foo', 10)->get()); | ||
|
|
||
| $lock->release(); | ||
| } | ||
|
|
||
| public function testLockCannotBeRefreshedByAnotherOwner() | ||
| { | ||
| $firstLock = Cache::driver('database')->lock('foo', 10); | ||
| $this->assertTrue($firstLock->get()); | ||
|
|
||
| // Create a new lock with a different owner | ||
| $secondLock = Cache::store('database')->restoreLock('foo', 'other_owner'); | ||
|
|
||
| // Second lock should not be able to refresh | ||
| $this->assertFalse($secondLock->refresh(20)); | ||
|
|
||
| // Original lock should still be able to refresh | ||
| $this->assertTrue($firstLock->refresh(20)); | ||
|
|
||
| $firstLock->release(); | ||
| } |
There was a problem hiding this comment.
The DatabaseLockTest is missing a test case for refreshing with default seconds (when no seconds parameter is provided to refresh). The FileCacheLockTest and RedisCacheLockTest both include testLockRefreshWithDefaultSeconds test methods that verify the lock can be refreshed using the original duration. Consider adding this test case to ensure consistent test coverage across all lock implementations.
| public function refresh($seconds = null) | ||
| { | ||
| $seconds ??= $this->seconds; | ||
|
|
||
| $value = $this->memcached->get($this->name, null, \Memcached::GET_EXTENDED); | ||
|
|
||
| if ($value === false || ($value['value'] ?? null) !== $this->owner) { | ||
| return false; | ||
| } | ||
|
|
||
| return $this->memcached->cas($value['cas'], $this->name, $this->owner, $seconds); | ||
| } |
There was a problem hiding this comment.
The MemcachedLock refresh functionality lacks test coverage. While MemcachedCacheLockTestCase has comprehensive tests for lock acquisition and release, there are no tests for the newly added refresh method. Consider adding test cases similar to those in FileCacheLockTest and RedisCacheLockTest that verify: (1) successful refresh, (2) refresh preventing acquisition by another process, (3) refresh failure when attempted by another owner, and (4) refresh with default seconds parameter.
| public function refresh($seconds = null) | ||
| { | ||
| $seconds ??= $this->seconds; | ||
|
|
||
| if ($seconds <= 0) { | ||
| $seconds = 86400; | ||
| } | ||
|
|
||
| return $this->dynamo->refreshIfOwned($this->name, $this->owner, $seconds); | ||
| } |
There was a problem hiding this comment.
The DynamoDbLock refresh functionality lacks test coverage. There are no integration tests for DynamoDbLock at all. Consider adding test cases similar to those in FileCacheLockTest, RedisCacheLockTest, and DatabaseLockTest that verify: (1) successful refresh, (2) refresh preventing acquisition by another process, (3) refresh failure when attempted by another owner, and (4) refresh with default seconds parameter. Also verify the special handling of seconds <= 0 defaulting to 86400.
| public function refresh($seconds = null) | ||
| { | ||
| if (! $this->isOwnedByCurrentProcess()) { | ||
| return false; | ||
| } | ||
|
|
||
| $seconds ??= $this->seconds; | ||
|
|
||
| $this->store->locks[$this->name]['expiresAt'] = $seconds === 0 ? null : Carbon::now()->addSeconds($seconds); | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
The refresh operation should verify that the lock hasn't expired before allowing the refresh. Currently, isOwnedByCurrentProcess checks ownership but doesn't validate that the lock's expiration hasn't passed. This means an expired lock could be refreshed by its previous owner. Consider adding expiration validation similar to how the acquire method checks if expiration.isFuture() before preventing acquisition.
src/Illuminate/Cache/DynamoDbLock.php
Show resolved
Hide resolved
|
|
||
| return $this->connection->table($this->table) | ||
| ->where('key', $this->name) | ||
| ->where('owner', $this->owner) |
There was a problem hiding this comment.
The refresh operation should verify that the lock hasn't expired before allowing the refresh. Currently, the update query only checks if the key and owner match, but doesn't validate that the current expiration hasn't passed. This means an expired lock could be refreshed by its previous owner, potentially extending a lock that should have been released. Consider adding a condition to check that the current expiration is greater than the current time, similar to how the acquire method handles expired locks.
| ->where('owner', $this->owner) | |
| ->where('owner', $this->owner) | |
| ->where('expiration', '>', $this->currentTime()) |
| $this->seconds = $seconds; | ||
|
|
||
| return $this->connection->table($this->table) | ||
| ->where('key', $this->name) | ||
| ->where('owner', $this->owner) | ||
| ->update(['expiration' => $this->expiresAt()]) >= 1; |
There was a problem hiding this comment.
The refresh operation modifies the instance's $seconds property as a side effect. This state mutation is unexpected for a method that's meant to atomically refresh a lock's expiration. If refresh is called multiple times with different durations, the instance state will change each time. Consider removing this line to avoid side effects, as the refresh operation should only update the lock's expiration in the storage backend without modifying the lock instance's state.
| $this->seconds = $seconds; | |
| return $this->connection->table($this->table) | |
| ->where('key', $this->name) | |
| ->where('owner', $this->owner) | |
| ->update(['expiration' => $this->expiresAt()]) >= 1; | |
| $expiration = $this->currentTime() + $seconds; | |
| return $this->connection->table($this->table) | |
| ->where('key', $this->name) | |
| ->where('owner', $this->owner) | |
| ->update(['expiration' => $expiration]) >= 1; |
This PR adds the ability to increase the duration of an acquired lock atomically for all cache stores.
It's useful for long running operations so that you can set the TTL to a short value and then extend the lock expiration as the operation runs. When using expiring locks, it's possible that lock isn't released due to bugs, segmentation faults, temporary server issues, therefore long expiry times result in further issues.
Symfony has support for this, see https://symfony.com/doc/current/components/lock.html#expiring-locks
Support for this has been attempted before in #42342 but rejected (possibly due to race conditions?)
Demand for this functionality still exists, see #52778