From c094b88944c0d437af70afde6b2cb19142b1f073 Mon Sep 17 00:00:00 2001 From: Joao Gilberto Magalhaes Date: Fri, 7 Aug 2026 10:28:29 -0400 Subject: [PATCH 1/6] Add CompareAndSwapInterface and fix races in atomic operations Introduce CompareAndSwapInterface with setIfAbsent, deleteIfEquals and expireIfEquals: conditional writes the engine resolves in one indivisible step, which is what a correct distributed lock needs. Implemented by RedisCacheEngine (SET NX EX plus Lua) and MemcachedEngine (native add plus cas). Kept separate from AtomicOperationInterface so existing implementors are untouched. Fix three real races in the existing atomic operations: - Memcached seeded a missing key with get()===false then set() in increment, decrement and add. The increment itself is atomic but the seeding is not, so a slower caller's set(0) lands after a faster caller's increment and reissues the same value. Twenty concurrent increments ended at 8. Now seeded with Memcached::add(). - Redis add() converted a string key into a list with GET, DEL and re-push, unprotected, so concurrent callers deleted a list the others were rebuilding. Twenty concurrent appends duplicated the seed value 16 times. The conversion is now a compare-guarded script. - Redis increment and decrement applied the TTL as a separate EXPIRE, leaving a counter with no expiry if the process died in between. Both now run as one script, and add() finally honours the $ttl it had been accepting and discarding. Also fix FileSystem, where the atomic operations passed a relative TTL where an absolute timestamp was expected, so anything written with a TTL expired in 1970; and where the expiry file was dropped before the lock was taken. FileSystem does not implement CompareAndSwapInterface: flock binds to an inode and the engine unlinks on delete, so two processes can hold the lock on two different inodes at the same path. Tests fork twenty real processes at a synchronised barrier; each fix has a case that fails without it. --- CHANGELOG-7.0.md | 192 +++++++++++++++++++++++++ README.md | 2 + docs/atomic-operations.md | 19 +++ docs/compare-and-swap.md | 141 ++++++++++++++++++ src/CompareAndSwapInterface.php | 65 +++++++++ src/Psr16/FileSystemCacheEngine.php | 31 ++-- src/Psr16/MemcachedEngine.php | 123 ++++++++++++---- src/Psr16/RedisCacheEngine.php | 212 ++++++++++++++++++++++----- tests/CachePSR16Test.php | 32 +++++ tests/CompareAndSwapTest.php | 164 +++++++++++++++++++++ tests/ConcurrencyTest.php | 213 ++++++++++++++++++++++++++++ 11 files changed, 1115 insertions(+), 79 deletions(-) create mode 100644 CHANGELOG-7.0.md create mode 100644 docs/compare-and-swap.md create mode 100644 src/CompareAndSwapInterface.php create mode 100644 tests/CompareAndSwapTest.php create mode 100644 tests/ConcurrencyTest.php diff --git a/CHANGELOG-7.0.md b/CHANGELOG-7.0.md new file mode 100644 index 0000000..579be28 --- /dev/null +++ b/CHANGELOG-7.0.md @@ -0,0 +1,192 @@ +# Changelog - Version 7.0 + +## Overview + +Version 7.0 adds `CompareAndSwapInterface`, the conditional-write primitive needed to build correct +distributed locks on top of a cache engine, and repairs three genuine race conditions in the +existing atomic operations. + +The races were not theoretical. A forked-process test reproduces each one against 6.x: twenty +concurrent `increment()` calls on Memcached ended at **8** instead of 20, and twenty concurrent +`add()` calls on Redis duplicated the seed value **16 times** while dropping appends. + +## Breaking Changes + +| Area | Before (6.x) | After (7.0) | Description | +|------|--------------|-------------|-------------| +| **Redis atomic operations** | `INCR`/`RPUSH` + separate `EXPIRE` | Lua scripts (`EVAL`) | `increment()`, `decrement()` and `add()` are now single scripts so the value and its TTL cannot be split apart. **Requires Lua scripting to be enabled on the Redis server**; some managed offerings restrict `EVAL`. | +| **Redis `add()` TTL** | `$ttl` accepted and silently ignored | `$ttl` applied | A list built with `add($key, $value, 60)` now actually expires. Code that relied on the list living forever despite passing a TTL will see it expire. | +| **FileSystem atomic TTL** | raw `$ttl` written as the expiry timestamp | converted with `addToNow()` | `increment($key, 1, 60)` wrote `60` into the expiry file — a moment in 1970 — so the entry was already expired when written. It now means 60 seconds from now, and the value survives as intended. | + +## New Features + +### CompareAndSwapInterface + +A new interface for conditional writes that the storage engine resolves in one indivisible step: + +- `setIfAbsent(string $key, mixed $value, DateInterval|int|null $ttl = null): bool` — stores only if + the key is free; exactly one of any number of concurrent callers gets `true`. The TTL is applied + in the same step as the write, so a process that dies mid-operation cannot leave a key with no + expiry behind. +- `deleteIfEquals(string $key, mixed $value): bool` — deletes only while the key still holds your + value, so an owner whose TTL quietly lapsed cannot destroy the entry someone else has taken over. +- `expireIfEquals(string $key, mixed $value, DateInterval|int|null $ttl): bool` — extends the TTL + under the same guard. + +**Implemented by:** `RedisCacheEngine` (`SET NX EX` plus Lua for the guarded operations) and +`MemcachedEngine` (native `add()` plus `cas()`). + +`FileSystemCacheEngine` deliberately does **not** implement it. `flock()` attaches to an inode +rather than a path, and this engine unlinks the file on delete, so two processes can hold "the +lock" on two different inodes at the same path. The file system is not a reliable substrate for +mutual exclusion and the engine does not pretend otherwise. + +This is a separate interface rather than an addition to `AtomicOperationInterface`, so nothing that +already implements the latter breaks. Probe with `instanceof` and degrade explicitly: + +```php +if (!$cache instanceof \ByJG\Cache\CompareAndSwapInterface) { + throw new RuntimeException('This engine cannot guarantee mutual exclusion'); +} +``` + +### Documentation + +- New [Compare and Swap](docs/compare-and-swap.md) guide, including a worked lock example and an + explicit section on what the interface does *not* give you (no fairness, no reentrancy, the TTL + is still a guess). +- [Atomic Operations](docs/atomic-operations.md) now documents the TTL semantics and what happens + when `add()` is called on a key previously written by `set()`. + +## Bug Fixes + +### Memcached: non-atomic seeding in every atomic operation + +`increment()`, `decrement()` and `add()` all initialised a missing key with `get() === false` +followed by `set()`. Memcached's own `increment()` is atomic, but those two preparatory calls are +not, and the interleaving loses updates: + +``` +P1: get() === false P2: get() === false +P1: set(0) +P1: increment() -> 1 + P2: set(0) <- resets the counter + P2: increment() -> 1 <- the same value issued twice +``` + +All three now seed with `Memcached::add()`, which the server resolves in a single step: exactly one +caller creates the key and the rest move on. + +`add()` was additionally hardened — it no longer dereferences the CAS token when the key expired +mid-loop, and it claims an absent key with `add()` instead of `set()`. + +### Redis: `add()` corrupted the list when converting from a plain value + +The first `add()` to a key written by `set()` has to turn a string into a list. That was done as +`GET` → `DEL` → re-push → `RPUSH` with no protection, so concurrent callers each deleted a list the +others were mid-way through rebuilding. The conversion is now a compare-guarded script that only +rewrites the key while it still holds the value that was read; losing that compare means another +process already converted it. + +### Redis: TTL applied as a separate command + +`increment()` and `decrement()` set the expiry with a follow-up `EXPIRE`. A crash between the two +left a counter with no expiry at all. Both now apply it inside the script. + +### FileSystem: expiry dropped outside the lock + +`putContents()` deleted the `.ttl` file before acquiring the lock, leaving the value briefly +immortal — a reader arriving in that window saw an entry that should already have expired. The +delete now happens under the lock. + +## Testing + +- `tests/CompareAndSwapTest.php` — 18 tests across both engines covering expiry, non-owner + rejection and the stale-owner-versus-new-owner case. +- `tests/ConcurrencyTest.php` — forks twenty real processes at a synchronised barrier. Sequential + tests cannot prove atomicity; these fail against 6.x and pass on 7.0. +- A TTL regression test added to `tests/CachePSR16Test.php` covering every engine that implements + `AtomicOperationInterface`. + +Note for anyone writing similar tests: forked children inherit the parent's Memcached socket and +close it as they exit, killing the parent's connection. Reconnect in the parent before asserting. + +## Migration Path from 6.x to 7.0 + +### Step 1: Confirm Lua scripting is available on your Redis server + +`increment()`, `decrement()` and `add()` now use `EVAL`. This is enabled by default in Redis, but +some managed and proxied deployments restrict it. Verify with: + +```bash +redis-cli EVAL "return 1" 0 # should print (integer) 1 +``` + +If `EVAL` is unavailable in your environment, stay on 6.x or open an issue. + +### Step 2: Update the dependency + +```bash +composer require byjg/cache-engine:^7.0 +composer update +``` + +### Step 3: Review any use of `add()` with a TTL on Redis + +The TTL was previously ignored, so lists built this way never expired. They now do. If you were +relying on the old behaviour, drop the TTL argument: + +```php +$cache->add('my-key', 'value', 3600); // now expires after an hour +$cache->add('my-key', 'value'); // never expires, as 6.x behaved +``` + +### Step 4: Review any use of atomic operations with a TTL on FileSystem + +These were broken and returned already-expired entries, so working code is unlikely to depend on +them. If you worked around the bug by omitting the TTL, you can now pass it. + +### Step 5: Adopt compare-and-swap where you were hand-rolling a lock + +If you have code shaped like this, replace it — it has a race between the two calls: + +```php +// Before - two operations with a gap between them +if (!$cache->has('lock')) { + $cache->set('lock', $token, 30); +} + +// After - one indivisible operation +if ($cache->setIfAbsent('lock', $token, 30)) { + try { + // ... + } finally { + $cache->deleteIfEquals('lock', $token); + } +} +``` + +### Common Migration Issues + +**Issue**: `NOSCRIPT` or "unknown command EVAL" errors from Redis +**Solution**: Lua scripting is disabled or proxied away in your deployment. See Step 1. + +**Issue**: Cached lists built with `add()` now disappear +**Solution**: You are passing a TTL that was previously ignored. See Step 3. + +**Issue**: `$cache->setIfAbsent()` does not exist +**Solution**: The engine does not implement `CompareAndSwapInterface`. Only `RedisCacheEngine` and +`MemcachedEngine` do. Probe with `instanceof` before calling. + +## Notes + +- `AtomicOperationInterface` is unchanged; existing implementations continue to work untouched. +- No changes to the PSR-6 or PSR-16 interface implementations. +- Existing cache data remains compatible across versions. +- PHP requirement is unchanged at `>=8.3 <8.6`. + +## Links + +- [Full Commit History](https://github.com/byjg/php-cache-engine/compare/6.0.1...7.0.0) +- [Documentation](https://github.com/byjg/php-cache-engine/tree/master/docs) +- [Report Issues](https://github.com/byjg/php-cache-engine/issues) diff --git a/README.md b/README.md index c297706..cc60f8a 100644 --- a/README.md +++ b/README.md @@ -20,6 +20,7 @@ A powerful, versatile cache implementation providing both PSR-6 and PSR-16 inter - **PSR-6 Cache Pool interface** - More verbose caching with fine-grained control - **Multiple storage backends** - Choose from memory, file system, Redis, Memcached and more - **Atomic operations** - Support for increment, decrement and add operations in compatible engines +- **Compare and swap** - Conditional writes for correct distributed locking on Redis and Memcached - **Garbage collection** - Automatic cleanup of expired items - **PSR-11 container support** - Retrieve cache keys via dependency container - **Logging capabilities** - PSR-3 compatible logging of cache operations @@ -68,6 +69,7 @@ $value = $item->get(); ### Advanced Features - [Atomic Operations](docs/atomic-operations.md) +- [Compare and Swap](docs/compare-and-swap.md) - [Garbage Collection](docs/garbage-collection.md) - [Logging](docs/setup-log-handler.md) - [PSR-11 Container Usage](docs/psr11-usage.md) diff --git a/docs/atomic-operations.md b/docs/atomic-operations.md index 542b8cf..8dc2539 100644 --- a/docs/atomic-operations.md +++ b/docs/atomic-operations.md @@ -24,6 +24,13 @@ The engines that support atomic operations implement the `AtomicOperationInterfa - FileSystemCacheEngine - TmpfsCacheEngine (inherits from FileSystemCacheEngine) +These operations read and modify a value in one step. If what you need instead is a *conditional* +write — "store this only if the key is free", "delete this only if it is still mine" — see +[Compare and Swap](compare-and-swap.md). + +All three operations accept an optional TTL, expressed in seconds from now (or as a `DateInterval`), +and apply it as part of the same operation. + ## Increment The increment operation is used to increment a value by a given number. @@ -58,3 +65,15 @@ $cache->add('my-key', 'value3'); print_r($cache->get('my-key')); // ['value1', 'value2', 'value3'] ``` +If the key already holds a plain value written by `set()`, the first `add()` converts it into a +list and keeps the original value as the first element: + +```php +set('my-key', 'value1'); +$cache->add('my-key', 'value2'); + +print_r($cache->get('my-key')); // ['value1', 'value2'] +``` + + diff --git a/docs/compare-and-swap.md b/docs/compare-and-swap.md new file mode 100644 index 0000000..1257460 --- /dev/null +++ b/docs/compare-and-swap.md @@ -0,0 +1,141 @@ +--- +sidebar_position: 12 +--- + +# Compare and Swap + +Compare-and-swap means: read the current state, compare it against what you expect, and only write +when it matches — all resolved by the storage engine in a single indivisible step. + +The reason this needs its own interface is that the obvious way of writing it is wrong: + +```php +has('lock')) { // <- another process can write here + $cache->set('lock', $token, 30); +} +``` + +Between the `has()` and the `set()` there is a gap. Two processes can both find the key absent and +both write, and both walk away believing they own it. No amount of re-reading afterwards closes +that gap. Compare-and-swap removes it by never splitting the check from the write. + +The engines that support it implement `CompareAndSwapInterface`. + +**Engines that support compare and swap:** + +- RedisCacheEngine +- MemcachedEngine + +FileSystemCacheEngine does **not** implement this interface. See +[the note at the bottom](#why-not-filesystemcacheengine). + +## setIfAbsent + +Stores the value only if the key currently holds nothing. Exactly one of any number of concurrent +callers gets `true`. A key whose TTL has already elapsed counts as absent. + +```php +setIfAbsent('my-lock', $token, 30)) { + // This process, and only this process, owns 'my-lock' for the next 30 seconds +} +``` + +The TTL is applied in the same step as the write. This matters: an engine that wrote the value +first and set the expiry afterwards would leave a key with no expiry at all if the process died in +between — a lock nobody can ever release. + +## deleteIfEquals + +Deletes the key only if it still holds exactly the value you pass. + +```php +deleteIfEquals('my-lock', $token); +``` + +Use this instead of `delete()` whenever the key might have changed hands. If your TTL quietly +expired while you were still working, another process may already hold the key — a plain `delete()` +would destroy *their* lock. `deleteIfEquals()` refuses and returns `false`. + +## expireIfEquals + +Rewrites the TTL, but only while the key still holds your value. This lets a long-running owner +keep its claim alive without any risk of extending a claim that has already passed to someone else. + +```php +expireIfEquals('my-lock', $token, 30)) { + // We lost the lock. Stop - somebody else is doing this work now. + break; + } +} +``` + +Passing `null` as the TTL removes the expiry, making the key permanent. + +## Putting it together + +```php +setIfAbsent('rebuild-report', $token, 60)) { + return; // Another worker already has it +} + +try { + rebuildTheReport(); +} finally { + $cache->deleteIfEquals('rebuild-report', $token); +} +``` + +The random token is what makes the release safe. Without it there is no way to tell your own lock +apart from the one a later process took over after your TTL lapsed. + +## Detecting support + +Not every engine can offer these guarantees. Probe before relying on them, and degrade explicitly +rather than silently: + +```php +putContents($this->fixKey($key), $value, $ttl, function ($currentValue, $value) { + // addToNow() is what turns a relative TTL into the absolute timestamp the .ttl file holds. + // Passing the raw $ttl through wrote "60" as the expiry, i.e. a moment in 1970, so any + // counter created with a TTL was already expired by the time it was written. + return $this->putContents($this->fixKey($key), $value, $this->addToNow($ttl), function ($currentValue, $value) { return intval($currentValue) + $value; }); } @@ -287,7 +292,7 @@ public function increment(string $key, int $value = 1, DateInterval|int|null $tt #[Override] public function decrement(string $key, int $value = 1, DateInterval|int|null $ttl = null): int { - return $this->putContents($this->fixKey($key), $value, $ttl, function ($currentValue, $value) { + return $this->putContents($this->fixKey($key), $value, $this->addToNow($ttl), function ($currentValue, $value) { return intval($currentValue) - $value; }); } @@ -295,7 +300,7 @@ public function decrement(string $key, int $value = 1, DateInterval|int|null $tt #[Override] public function add(string $key, $value, DateInterval|int|null $ttl = null): array { - return $this->putContents($this->fixKey($key), $value, $ttl, function ($currentValue, $value) { + return $this->putContents($this->fixKey($key), $value, $this->addToNow($ttl), function ($currentValue, $value) { if (empty($currentValue)) { return [$value]; } diff --git a/src/Psr16/MemcachedEngine.php b/src/Psr16/MemcachedEngine.php index 71200ab..b6d328f 100644 --- a/src/Psr16/MemcachedEngine.php +++ b/src/Psr16/MemcachedEngine.php @@ -3,6 +3,7 @@ namespace ByJG\Cache\Psr16; use ByJG\Cache\AtomicOperationInterface; +use ByJG\Cache\CompareAndSwapInterface; use ByJG\Cache\Exception\InvalidArgumentException; use ByJG\Cache\Exception\StorageErrorException; use DateInterval; @@ -12,8 +13,13 @@ use Psr\Log\LoggerInterface; use Psr\Log\NullLogger; -class MemcachedEngine extends BaseCacheEngine implements AtomicOperationInterface +class MemcachedEngine extends BaseCacheEngine implements AtomicOperationInterface, CompareAndSwapInterface { + /** + * Memcached expires an item immediately when handed a negative expiration. That is the only + * way to retire a key through cas(), since the extension exposes no compare-and-delete. + */ + private const EXPIRE_NOW = -1; /** * @@ -231,16 +237,13 @@ public function increment(string $key, int $value = 1, DateInterval|int|null $tt { $this->lazyLoadMemCachedServers(); - $ttl = $this->convertToSeconds($ttl); - - if ($this->memCached->get($this->fixKey($key)) === false) { - $this->memCached->set($this->fixKey($key), 0, is_null($ttl) ? 0 : $ttl); - } + $fixKey = $this->fixKey($key); + $this->seed($fixKey, 0, $ttl); - $result = $this->memCached->increment($this->fixKey($key), $value); + $result = $this->memCached->increment($fixKey, $value); $this->logger->info("[Memcached] Increment '$key' result " . $this->memCached->getResultCode()); if ($this->memCached->getResultCode() !== Memcached::RES_SUCCESS) { - $this->logger->error("[Memcached] Set '$key' failed with status " . $this->memCached->getResultCode()); + $this->logger->error("[Memcached] Increment '$key' failed with status " . $this->memCached->getResultCode()); } return $result; @@ -257,21 +260,34 @@ public function decrement(string $key, int $value = 1, DateInterval|int|null $tt { $this->lazyLoadMemCachedServers(); - $ttl = $this->convertToSeconds($ttl); - - if ($this->memCached->get($this->fixKey($key)) === false) { - $this->memCached->set($this->fixKey($key), 0, is_null($ttl) ? 0 : $ttl); - } + $fixKey = $this->fixKey($key); + $this->seed($fixKey, 0, $ttl); - $result = $this->memCached->decrement($this->fixKey($key), $value); + $result = $this->memCached->decrement($fixKey, $value); $this->logger->info("[Memcached] Decrement '$key' result " . $this->memCached->getResultCode()); if ($this->memCached->getResultCode() !== Memcached::RES_SUCCESS) { - $this->logger->error("[Memcached] Set '$key' failed with status " . $this->memCached->getResultCode()); + $this->logger->error("[Memcached] Decrement '$key' failed with status " . $this->memCached->getResultCode()); } return $result; } + /** + * Create the key only if it is absent, so a counter can be started without a race. + * + * increment()/decrement() fail on a missing key, which forces every caller to initialise it + * first. Doing that with get()-then-set() is what used to break: two processes could both read + * the key as absent, and the slower one's set(0) would land AFTER the faster one's increment, + * resetting the counter and handing out the same value twice. add() is resolved server-side in + * a single step, so exactly one caller creates the key and everybody else silently moves on. + */ + private function seed(string $fixKey, mixed $initial, DateInterval|int|null $ttl): void + { + $ttl = $this->convertToSeconds($ttl); + + $this->memCached->add($fixKey, $initial, is_null($ttl) ? 0 : $ttl); + } + /** * @throws NotFoundExceptionInterface * @throws InvalidArgumentException @@ -284,29 +300,80 @@ public function add(string $key, $value, DateInterval|int|null $ttl = null): arr $this->lazyLoadMemCachedServers(); $ttl = $this->convertToSeconds($ttl); + $expiration = is_null($ttl) ? 0 : $ttl; $fixKey = $this->fixKey($key); - if ($this->memCached->get($fixKey) === false) { - $this->memCached->set($fixKey, [], is_null($ttl) ? 0 : $ttl); - } - - do { + while (true) { $data = $this->memCached->get($fixKey, null, Memcached::GET_EXTENDED); - $casToken = $data['cas']; - $currentValue = $data['value']; - if ($currentValue === false) { - $currentValue = []; + // Absent: claim it with add(), which only succeeds for the first caller to get there. + // Whoever loses simply goes round again and finds the list the winner created. + if ($data === false) { + if ($this->memCached->add($fixKey, [$value], $expiration)) { + return [$value]; + } + continue; } + $currentValue = $data['value']; if (!is_array($currentValue)) { $currentValue = [$currentValue]; } - $currentValue[] = $value; - $success = $this->memCached->cas($casToken, $fixKey, $currentValue, is_null($ttl) ? 0 : $ttl); - } while (!$success); - return $currentValue; + // cas() writes only while the item is untouched since the read above; if another + // append slipped in, the token is stale, the write is refused and we retry on top of it. + if ($this->memCached->cas($data['cas'], $fixKey, $currentValue, $expiration)) { + return $currentValue; + } + } + } + + #[\Override] + public function setIfAbsent(string $key, mixed $value, DateInterval|int|null $ttl = null): bool + { + $this->lazyLoadMemCachedServers(); + + $ttl = $this->convertToSeconds($ttl); + + // Memcached's own add() is the primitive this whole interface is named after. + return $this->memCached->add($this->fixKey($key), $value, is_null($ttl) ? 0 : $ttl); + } + + #[\Override] + public function deleteIfEquals(string $key, mixed $value): bool + { + // No compare-and-delete exists, so retire the item by expiring it in the same cas() that + // proves we are still looking at the value we expect. + return $this->casIfEquals($key, $value, self::EXPIRE_NOW); + } + + #[\Override] + public function expireIfEquals(string $key, mixed $value, DateInterval|int|null $ttl): bool + { + $ttl = $this->convertToSeconds($ttl); + + return $this->casIfEquals($key, $value, is_null($ttl) ? 0 : $ttl); + } + + /** + * Rewrite the item with a new expiration, but only while it still holds $value. + * + * The cas token read here is invalidated server-side by any competing write, so a caller whose + * item was replaced between the get() and the cas() is refused rather than silently clobbering + * the new owner's value. + */ + private function casIfEquals(string $key, mixed $value, int $expiration): bool + { + $this->lazyLoadMemCachedServers(); + + $fixKey = $this->fixKey($key); + $data = $this->memCached->get($fixKey, null, Memcached::GET_EXTENDED); + + if ($data === false || $data['value'] != $value) { + return false; + } + + return $this->memCached->cas($data['cas'], $fixKey, $data['value'], $expiration); } } diff --git a/src/Psr16/RedisCacheEngine.php b/src/Psr16/RedisCacheEngine.php index 20b9d26..92f7aee 100644 --- a/src/Psr16/RedisCacheEngine.php +++ b/src/Psr16/RedisCacheEngine.php @@ -3,7 +3,9 @@ namespace ByJG\Cache\Psr16; use ByJG\Cache\AtomicOperationInterface; +use ByJG\Cache\CompareAndSwapInterface; use ByJG\Cache\Exception\InvalidArgumentException; +use ByJG\Cache\Exception\StorageErrorException; use DateInterval; use Psr\Container\ContainerExceptionInterface; use Psr\Container\NotFoundExceptionInterface; @@ -12,8 +14,66 @@ use Redis; use RedisException; -class RedisCacheEngine extends BaseCacheEngine implements AtomicOperationInterface +class RedisCacheEngine extends BaseCacheEngine implements AtomicOperationInterface, CompareAndSwapInterface { + /** + * Every multi-step operation below runs as a Lua script instead of a sequence of commands. + * Redis executes a script to completion before serving any other client, so the read and the + * write it performs cannot be interleaved - which is exactly the guarantee the callers need. + */ + private const LUA_INCREMENT_BY = <<<'LUA' + local result = redis.call('INCRBY', KEYS[1], ARGV[1]) + if tonumber(ARGV[2]) > 0 then + redis.call('EXPIRE', KEYS[1], ARGV[2]) + end + return result + LUA; + + /** Appends to the list and hands back the result the caller would otherwise have to re-read. */ + private const LUA_APPEND = <<<'LUA' + if redis.call('TYPE', KEYS[1])['ok'] == 'string' then + return false + end + redis.call('RPUSH', KEYS[1], ARGV[1]) + if tonumber(ARGV[2]) > 0 then + redis.call('EXPIRE', KEYS[1], ARGV[2]) + end + return redis.call('LRANGE', KEYS[1], 0, -1) + LUA; + + /** Rewrites a string key as a list, but only while it still holds the value we inspected. */ + private const LUA_EXPLODE_STRING = <<<'LUA' + if redis.call('GET', KEYS[1]) ~= ARGV[1] then + return 0 + end + redis.call('DEL', KEYS[1]) + for i = 2, #ARGV do + redis.call('RPUSH', KEYS[1], ARGV[i]) + end + return 1 + LUA; + + private const LUA_DELETE_IF_EQUALS = <<<'LUA' + if redis.call('GET', KEYS[1]) == ARGV[1] then + return redis.call('DEL', KEYS[1]) + end + return 0 + LUA; + + private const LUA_EXPIRE_IF_EQUALS = <<<'LUA' + if redis.call('GET', KEYS[1]) ~= ARGV[1] then + return 0 + end + if tonumber(ARGV[2]) > 0 then + redis.call('EXPIRE', KEYS[1], ARGV[2]) + else + redis.call('PERSIST', KEYS[1]) + end + return 1 + LUA; + + /** Bound on the string-to-list conversion retry; only a pathological writer ever gets close. */ + private const MAX_CONVERSION_ATTEMPTS = 10; /** * @@ -72,6 +132,25 @@ protected function fixKey(string $key): string return "cache:$key"; } + /** + * Redis stores scalars verbatim and everything else serialized. get(), add() and the + * compare-and-swap operations must agree on this representation, otherwise a CAS comparison + * would be made against a string the caller never actually wrote. + */ + protected function encode(mixed $value): mixed + { + return is_object($value) || is_array($value) ? serialize($value) : $value; + } + + protected function decode(mixed $value): mixed + { + if (is_string($value) && preg_match('/^[Oa]:\d+:["{]/', $value)) { + return unserialize($value); + } + + return $value; + } + /** * @param string $key * @param mixed $default @@ -90,10 +169,7 @@ public function get(string $key, mixed $default = null): mixed $type = $this->redis->type($fixKey); if ($type === Redis::REDIS_STRING) { - $value = $this->redis->get($fixKey); - if (is_string($value) && preg_match('/^[Oa]:\d+:["{]/', $value)) { - $value = unserialize($value); - } + $value = $this->decode($this->redis->get($fixKey)); } else if ($type === Redis::REDIS_LIST) { $value = $this->redis->lRange($fixKey, 0, -1); } else { @@ -102,9 +178,7 @@ public function get(string $key, mixed $default = null): mixed if (is_array($value)) { foreach ($value as $k => $v) { - if (is_string($v) && preg_match('/^[Oa]:\d+:["{]/', $v)) { - $value[$k] = unserialize($v); - } + $value[$k] = $this->decode($v); } } @@ -200,55 +274,117 @@ public function isAvailable(): bool #[\Override] public function increment(string $key, int $value = 1, DateInterval|int|null $ttl = null): int { - $this->lazyLoadRedisServer(); + return $this->incrementBy($key, $value, $ttl); + } - $result = $this->redis->incr($this->fixKey($key), $value); + #[\Override] + public function decrement(string $key, int $value = 1, DateInterval|int|null $ttl = null): int + { + return $this->incrementBy($key, -$value, $ttl); + } - if ($ttl) { - $this->redis->expire($this->fixKey($key), $this->convertToSeconds($ttl)); - } + /** + * INCRBY covers both directions. Applying the TTL inside the script matters: as two separate + * commands, a process that died in between would leave the counter with no expiry at all. + */ + private function incrementBy(string $key, int $delta, DateInterval|int|null $ttl): int + { + return (int)$this->script(self::LUA_INCREMENT_BY, $key, [$delta, $this->ttlInSeconds($ttl)]); + } + + /** + * Run one of the scripts above against a single key. + * + * Every script here takes exactly one key and reads the rest of its inputs from ARGV, so the + * lazy connect and the eval() shape are the same each time and belong in one place. + */ + private function script(string $lua, string $key, array $args = []): mixed + { + $this->lazyLoadRedisServer(); - return $result; + return $this->redis->eval($lua, array_merge([$this->fixKey($key)], $args), 1); } + /** + * @throws StorageErrorException + */ #[\Override] - public function decrement(string $key, int $value = 1, DateInterval|int|null $ttl = null): int + public function add(string $key, $value, DateInterval|int|null $ttl = null): array { - $this->lazyLoadRedisServer(); + $args = [$this->encode($value), $this->ttlInSeconds($ttl)]; + + // A key previously written by set() is a plain string and has to become a list before it + // can be appended to. The append itself never contends - the loop only exists so a caller + // that loses that one-shot conversion to a concurrent writer can pick up and carry on. + for ($attempt = 0; $attempt < self::MAX_CONVERSION_ATTEMPTS; $attempt++) { + $list = $this->script(self::LUA_APPEND, $key, $args); + + if (is_array($list)) { + return array_map(fn($item) => $this->decode($item), $list); + } + + $this->convertStringToList($key); + } - $result = $this->redis->decr($this->fixKey($key), $value); + throw new StorageErrorException("Could not append to '$key': the key kept changing type"); + } - if ($ttl) { - $this->redis->expire($this->fixKey($key), $this->convertToSeconds($ttl)); + /** + * Turn a string key into the list add() appends to, without ever exposing a moment where the + * key is missing. The script rewrites it only while it still holds the value we just read, so + * losing the race means another process already converted it and there is nothing left to do. + */ + private function convertStringToList(string $key): void + { + $current = $this->redis->get($this->fixKey($key)); + if ($current === false) { + return; // Expired or deleted meanwhile; the next append creates a fresh list. } - return $result; + $decoded = $this->decode($current); + $elements = is_array($decoded) ? array_values($decoded) : [$decoded]; + + $this->script( + self::LUA_EXPLODE_STRING, + $key, + array_merge([$current], array_map(fn($item) => $this->encode($item), $elements)) + ); } #[\Override] - public function add(string $key, $value, DateInterval|int|null $ttl = null): array + public function setIfAbsent(string $key, mixed $value, DateInterval|int|null $ttl = null): bool { $this->lazyLoadRedisServer(); - $fixKey = $this->fixKey($key); - $type = $this->redis->type($fixKey); + $seconds = $this->ttlInSeconds($ttl); + // SET NX writes the value and its expiry as one command - there is no window in which the + // key exists without a TTL, so a crash here cannot leave the key behind forever. + $options = $seconds > 0 ? ['nx', 'ex' => $seconds] : ['nx']; - if ($type === Redis::REDIS_STRING) { - $currValue = $this->redis->get($fixKey); - if (is_string($currValue) && preg_match('/^[Oa]:\d+:["{]/', $currValue)) { - $currValue = unserialize($currValue); - } - if (is_object($currValue)) { - $currValue = [$currValue]; - } - $this->redis->del($fixKey); - foreach ((array)$currValue as $items) { - $this->add($key, $items); - } - } + return $this->redis->set($this->fixKey($key), $this->encode($value), $options) !== false; + } + + #[\Override] + public function deleteIfEquals(string $key, mixed $value): bool + { + return (bool)$this->script(self::LUA_DELETE_IF_EQUALS, $key, [$this->encode($value)]); + } + + #[\Override] + public function expireIfEquals(string $key, mixed $value, DateInterval|int|null $ttl): bool + { + return (bool)$this->script( + self::LUA_EXPIRE_IF_EQUALS, + $key, + [$this->encode($value), $this->ttlInSeconds($ttl)] + ); + } - $result = $this->redis->rPush($fixKey, is_object($value) || is_array($value) ? serialize($value) : $value); + /** Lua has no notion of "no TTL", so the absence of one is passed down as a plain zero. */ + private function ttlInSeconds(DateInterval|int|null $ttl): int + { + $seconds = $this->convertToSeconds($ttl); - return $result ? $this->get($key) : []; + return is_int($seconds) ? $seconds : 0; } } diff --git a/tests/CachePSR16Test.php b/tests/CachePSR16Test.php index fa64c0a..2b99e2a 100644 --- a/tests/CachePSR16Test.php +++ b/tests/CachePSR16Test.php @@ -396,4 +396,36 @@ public function testAtomicAdd(BaseCacheEngine $cacheEngine) $this->markTestIncomplete('Does not support atomic add or it is native'); } } + + /** + * The TTL argument of the atomic operations has to mean the same thing it means everywhere + * else: seconds from now. FileSystem used to hand it straight to the expiry file, which stores + * an absolute timestamp, so a value written with a TTL of 60 expired in January 1970 and the + * key was already unreadable by the time the caller looked at it. + * + * @param BaseCacheEngine $cacheEngine + * @throws \Psr\SimpleCache\InvalidArgumentException + */ + #[DataProvider('CachePoolProvider')] + public function testAtomicOperationsHonourTheTtl(BaseCacheEngine $cacheEngine) + { + $this->cacheEngine = $cacheEngine; + + if ($cacheEngine->isAvailable() && ($cacheEngine instanceof AtomicOperationInterface)) { + $this->assertEquals(1, $cacheEngine->increment('ttl-counter', 1, 30)); + $this->assertEquals(2, $cacheEngine->increment('ttl-counter', 1, 30)); + $this->assertEquals(2, $cacheEngine->get('ttl-counter'), 'A 30s TTL must still be alive'); + + $this->assertEquals(['x'], $cacheEngine->add('ttl-list', 'x', 30)); + $this->assertEquals(['x'], $cacheEngine->get('ttl-list'), 'A 30s TTL must still be alive'); + + $cacheEngine->increment('expiring-counter', 1, 1); + $cacheEngine->add('expiring-list', 'x', 1); + sleep(2); + $this->assertNull($cacheEngine->get('expiring-counter'), 'A 1s TTL must have elapsed'); + $this->assertNull($cacheEngine->get('expiring-list'), 'A 1s TTL must have elapsed'); + } else { + $this->markTestIncomplete('Does not support atomic operations or it is native'); + } + } } diff --git a/tests/CompareAndSwapTest.php b/tests/CompareAndSwapTest.php new file mode 100644 index 0000000..5815260 --- /dev/null +++ b/tests/CompareAndSwapTest.php @@ -0,0 +1,164 @@ + [new RedisCacheEngine()], + 'memcached' => [new MemcachedEngine()], + ]; + } + + #[\Override] + protected function tearDown(): void + { + foreach ($this->started as $engine) { + $engine->delete('cas-key'); + } + $this->started = []; + } + + /** + * Skips instead of failing so the suite still runs without the docker-compose services up, + * which is how the rest of this test suite treats an unreachable backend. + */ + private function engineOrSkip(BaseCacheEngine $engine): CompareAndSwapInterface + { + if (!$engine->isAvailable()) { + $this->markTestSkipped(get_class($engine) . ' is not available'); + } + + $this->assertInstanceOf(CompareAndSwapInterface::class, $engine); + + $this->started[] = $engine; + $engine->delete('cas-key'); + + return $engine; + } + + #[DataProvider('engineProvider')] + public function testSetIfAbsentSucceedsOnlyForTheFirstCaller(BaseCacheEngine $engine): void + { + $cas = $this->engineOrSkip($engine); + + $this->assertTrue($cas->setIfAbsent('cas-key', 'token-a', 30)); + $this->assertFalse($cas->setIfAbsent('cas-key', 'token-b', 30)); + $this->assertEquals('token-a', $engine->get('cas-key'), 'The loser must not have overwritten the winner'); + } + + #[DataProvider('engineProvider')] + public function testSetIfAbsentSucceedsAgainOnceTheTtlHasPassed(BaseCacheEngine $engine): void + { + $cas = $this->engineOrSkip($engine); + + $this->assertTrue($cas->setIfAbsent('cas-key', 'token-a', 1)); + $this->assertFalse($cas->setIfAbsent('cas-key', 'token-b', 1)); + + sleep(2); + + $this->assertTrue($cas->setIfAbsent('cas-key', 'token-b', 30)); + $this->assertEquals('token-b', $engine->get('cas-key')); + } + + #[DataProvider('engineProvider')] + public function testSetIfAbsentAppliesTheTtlWithTheWrite(BaseCacheEngine $engine): void + { + $cas = $this->engineOrSkip($engine); + + $cas->setIfAbsent('cas-key', 'token-a', 1); + sleep(2); + + $this->assertNull($engine->get('cas-key'), 'The key must expire on its own, with no second command'); + } + + #[DataProvider('engineProvider')] + public function testDeleteIfEqualsOnlyRemovesTheMatchingValue(BaseCacheEngine $engine): void + { + $cas = $this->engineOrSkip($engine); + + $cas->setIfAbsent('cas-key', 'token-a', 30); + + $this->assertFalse($cas->deleteIfEquals('cas-key', 'token-b')); + $this->assertEquals('token-a', $engine->get('cas-key'), 'A non-matching caller must not delete'); + + $this->assertTrue($cas->deleteIfEquals('cas-key', 'token-a')); + $this->assertNull($engine->get('cas-key')); + } + + #[DataProvider('engineProvider')] + public function testDeleteIfEqualsOnAMissingKeyReturnsFalse(BaseCacheEngine $engine): void + { + $cas = $this->engineOrSkip($engine); + + $this->assertFalse($cas->deleteIfEquals('cas-key', 'token-a')); + } + + /** + * The scenario the interface exists for: an owner whose TTL quietly lapsed must not be able to + * delete the key that somebody else has legitimately taken over in the meantime. + */ + #[DataProvider('engineProvider')] + public function testAnExpiredOwnerCannotDeleteTheNewOwnersValue(BaseCacheEngine $engine): void + { + $cas = $this->engineOrSkip($engine); + + $cas->setIfAbsent('cas-key', 'stale-owner', 1); + sleep(2); + $this->assertTrue($cas->setIfAbsent('cas-key', 'new-owner', 30)); + + $this->assertFalse($cas->deleteIfEquals('cas-key', 'stale-owner')); + $this->assertFalse($cas->expireIfEquals('cas-key', 'stale-owner', 30)); + $this->assertEquals('new-owner', $engine->get('cas-key')); + } + + #[DataProvider('engineProvider')] + public function testExpireIfEqualsExtendsTheLifetimeOfAMatchingValue(BaseCacheEngine $engine): void + { + $cas = $this->engineOrSkip($engine); + + $cas->setIfAbsent('cas-key', 'token-a', 1); + $this->assertTrue($cas->expireIfEquals('cas-key', 'token-a', 30)); + + sleep(2); + + $this->assertEquals('token-a', $engine->get('cas-key'), 'The original 1s expiry must have been replaced'); + } + + #[DataProvider('engineProvider')] + public function testExpireIfEqualsIsRejectedForANonMatchingValue(BaseCacheEngine $engine): void + { + $cas = $this->engineOrSkip($engine); + + $cas->setIfAbsent('cas-key', 'token-a', 1); + $this->assertFalse($cas->expireIfEquals('cas-key', 'token-b', 30)); + + sleep(2); + + $this->assertNull($engine->get('cas-key'), 'The rejected refresh must not have extended anything'); + } + + #[DataProvider('engineProvider')] + public function testSetIfAbsentRoundTripsNonScalarValues(BaseCacheEngine $engine): void + { + $cas = $this->engineOrSkip($engine); + + $this->assertTrue($cas->setIfAbsent('cas-key', ['a' => 1, 'b' => 2], 30)); + $this->assertEquals(['a' => 1, 'b' => 2], $engine->get('cas-key')); + + $this->assertFalse($cas->deleteIfEquals('cas-key', ['a' => 1])); + $this->assertTrue($cas->deleteIfEquals('cas-key', ['a' => 1, 'b' => 2])); + } +} diff --git a/tests/ConcurrencyTest.php b/tests/ConcurrencyTest.php new file mode 100644 index 0000000..e65ff64 --- /dev/null +++ b/tests/ConcurrencyTest.php @@ -0,0 +1,213 @@ + [RedisCacheEngine::class], + 'memcached' => [MemcachedEngine::class], + ]; + } + + #[\Override] + protected function setUp(): void + { + if (!function_exists('pcntl_fork')) { + $this->markTestSkipped('pcntl is required to test concurrent access'); + } + } + + /** + * Runs $child in CHILDREN separate processes, released as simultaneously as we can manage, and + * returns their exit codes. The barrier matters: without it the first child would routinely + * finish before the last one was even forked, and nothing would ever contend. + * + * @return int[] + */ + private function fork(callable $child): array + { + $startAt = microtime(true) + 0.5; + $pids = []; + + for ($i = 0; $i < self::CHILDREN; $i++) { + $pid = pcntl_fork(); + $this->assertNotSame(-1, $pid, 'Could not fork'); + + if ($pid === 0) { + $status = 1; + try { + usleep((int)max(0.0, ($startAt - microtime(true)) * 1_000_000.0)); + $status = $child($i) ? 0 : 1; + } finally { + exit($status); + } + } + + $pids[] = $pid; + } + + $codes = []; + foreach ($pids as $pid) { + pcntl_waitpid($pid, $status); + $codes[] = pcntl_wifexited($status) ? pcntl_wexitstatus($status) : -1; + } + + return $codes; + } + + /** + * Always hands back a freshly connected engine, and must be called again after fork() to read + * back results: the children inherit the parent's open socket and close it as they exit, which + * leaves the parent's own connection dead. That is a property of forking, not of the engine. + */ + private function engineOrSkip(string $engineClass): BaseCacheEngine + { + $engine = new $engineClass(); + if (!$engine->isAvailable()) { + $this->markTestSkipped("$engineClass is not available"); + } + + return $engine; + } + + /** + * The defining property of setIfAbsent: N processes race for an absent key, exactly one wins. + * A has()-then-set() implementation lets several win here. + */ + #[DataProvider('engineProvider')] + public function testExactlyOneProcessWinsSetIfAbsent(string $engineClass): void + { + $this->engineOrSkip($engineClass)->delete('race-lock'); + + $codes = $this->fork(function (int $i) use ($engineClass): bool { + /** @var CompareAndSwapInterface $engine */ + $engine = new $engineClass(); + return $engine->setIfAbsent('race-lock', "token-$i", 60); + }); + + $winners = count(array_filter($codes, fn($code) => $code === 0)); + $this->assertSame(1, $winners, "Expected exactly one winner, got $winners"); + } + + /** + * increment() must never hand the same number to two callers. This is the test that fails + * against a get()-then-set() seed: the losing process's set(0) lands after the winner's + * increment and resets the counter, so the value 1 gets issued twice. + */ + #[DataProvider('engineProvider')] + public function testIncrementNeverIssuesTheSameValueTwice(string $engineClass): void + { + $this->engineOrSkip($engineClass)->delete('race-counter'); + + $this->fork(function () use ($engineClass): bool { + $engine = new $engineClass(); + $engine->increment('race-counter'); + return true; + }); + + $this->assertEquals( + self::CHILDREN, + $this->engineOrSkip($engineClass)->get('race-counter'), + 'Every increment must be reflected exactly once in the final counter' + ); + } + + /** + * Concurrent appends must all survive. A read-modify-write without a compare loses whichever + * updates landed between another process's read and its write. + */ + #[DataProvider('engineProvider')] + public function testConcurrentAddLosesNothing(string $engineClass): void + { + $this->engineOrSkip($engineClass)->delete('race-list'); + + $this->fork(function (int $i) use ($engineClass): bool { + $engine = new $engineClass(); + $engine->add('race-list', "item-$i"); + return true; + }); + + $stored = $this->engineOrSkip($engineClass)->get('race-list'); + $this->assertIsArray($stored); + $this->assertCount(self::CHILDREN, $stored, 'Every appended item must still be present'); + + sort($stored); + $expected = array_map(fn($i) => "item-$i", range(0, self::CHILDREN - 1)); + sort($expected); + $this->assertEquals($expected, $stored, 'No item may be dropped or duplicated'); + } + + /** + * The first add() to a key that set() wrote has to convert it from a plain value into a list. + * Doing that as read-delete-rewrite means concurrent callers each delete a key the others are + * mid-way through rebuilding, which both duplicates the original value and drops appends. + */ + #[DataProvider('engineProvider')] + public function testConcurrentAddSurvivesTheConversionFromAPlainValue(string $engineClass): void + { + $setup = $this->engineOrSkip($engineClass); + $setup->delete('race-convert'); + $setup->set('race-convert', 'seed'); + + $this->fork(function (int $i) use ($engineClass): bool { + $engine = new $engineClass(); + $engine->add('race-convert', "item-$i"); + return true; + }); + + $stored = $this->engineOrSkip($engineClass)->get('race-convert'); + $this->assertIsArray($stored); + + sort($stored); + $expected = array_merge(['seed'], array_map(fn($i) => "item-$i", range(0, self::CHILDREN - 1))); + sort($expected); + $this->assertEquals($expected, $stored, 'The seed must appear exactly once and no append may be lost'); + } + + /** + * The whole point of a token-guarded release: only the process that actually owns the key may + * remove it, no matter how many others try at the same instant. + */ + #[DataProvider('engineProvider')] + public function testOnlyTheOwnerCanDeleteUnderContention(string $engineClass): void + { + /** @var BaseCacheEngine&CompareAndSwapInterface $setup */ + $setup = $this->engineOrSkip($engineClass); + $setup->delete('race-owned'); + $setup->setIfAbsent('race-owned', 'the-owner', 60); + + $codes = $this->fork(function (int $i) use ($engineClass): bool { + /** @var CompareAndSwapInterface $engine */ + $engine = new $engineClass(); + return $engine->deleteIfEquals('race-owned', "impostor-$i"); + }); + + $this->assertSame(0, count(array_filter($codes, fn($code) => $code === 0)), 'No impostor may succeed'); + + $verify = $this->engineOrSkip($engineClass); + $this->assertEquals('the-owner', $verify->get('race-owned'), 'The key must be untouched'); + $verify->delete('race-owned'); + } +} From a7fdfa09270053bb73bb258483bdc4391893f134 Mon Sep 17 00:00:00 2001 From: Joao Gilberto Magalhaes Date: Fri, 7 Aug 2026 10:32:19 -0400 Subject: [PATCH 2/6] Add changelog for version 6.0 --- CHANGELOG-6.0.md | 152 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 152 insertions(+) create mode 100644 CHANGELOG-6.0.md diff --git a/CHANGELOG-6.0.md b/CHANGELOG-6.0.md new file mode 100644 index 0000000..f57f9eb --- /dev/null +++ b/CHANGELOG-6.0.md @@ -0,0 +1,152 @@ +# Changelog - Version 6.0 + +## Overview + +Version 6.0 represents a major update focused on modernizing the codebase for PHP 8.3+ compatibility, improving code quality with strict typing, and enhancing developer experience with updated tooling. + +## Breaking Changes + +| Area | Before (5.x) | After (6.0) | Description | +|------|--------------|-------------|-------------| +| **PHP Version** | `>=8.1 <8.4` | `>=8.3 <8.6` | Minimum PHP version raised from 8.1 to 8.3. PHP 8.1 and 8.2 are no longer supported. Added support for PHP 8.4 and 8.5. | +| **PHPUnit** | `^9.6` | `^10.5\|^11.5` | PHPUnit 9.6 support dropped. Minimum version is now 10.5. Added support for PHPUnit 11.5. | +| **PSR-3 Logger** | `^1.0\|^1.1\|^2.0` | `^1.0\|^2.0\|^3.0` | Added PSR-3 version 3.0 support. Removed version 1.1 constraint (covered by 1.0). | +| **Psalm** | `^5.9` | `^5.9\|^6.13` | Added support for Psalm 6.13 while maintaining backward compatibility with 5.9. | + +## New Features + +### Code Quality Enhancements +- **PHP 8.3+ Override Attribute**: Added `#[Override]` attributes to all overridden methods across the codebase for better code clarity and IDE support +- **Strict Typing**: Enhanced type declarations throughout the codebase for improved type safety +- **Psalm SARIF Reporting**: Added SARIF (Static Analysis Results Interchange Format) output support for better CI/CD integration + +### Developer Experience +- **Composer Scripts**: Added convenient composer scripts: + - `composer test` - Run PHPUnit tests + - `composer psalm` - Run Psalm static analysis with single thread for stability +- **Gitpod Support**: Added `.gitpod.yml` configuration for cloud-based development environment +- **VSCode Configuration**: Added `.vscode/launch.json` with debugging configurations +- **Improved Documentation**: Enhanced all documentation files with better examples and clearer explanations + +### Testing Improvements +- **Test Class Refactoring**: Reorganized test class hierarchy for better maintainability + - Introduced `TestBase` class as the foundation for all cache tests + - Renamed test classes for clarity and consistency + - Migrated PHPUnit data providers to PHP 8.1+ syntax +- **Enhanced CI/CD**: Updated GitHub Actions workflows with better PHP version matrix testing + +### Engine Improvements +- **Consistent Key Handling**: Fixed key consistency issues in `MemcachedEngine` for more reliable caching +- **FileSystemCacheEngine**: Improved path handling and directory creation logic + +## Bug Fixes + +- Fixed unit test issues related to session handling in GitHub Actions environment +- Improved Memcached availability testing in CI/CD pipelines +- Fixed test execution issues requiring `--stderr` parameter for SessionCacheEngine tests +- Enhanced error handling and edge cases in various cache engines + +## Documentation Updates + +All documentation files have been updated to reflect version 6.0 changes: +- Updated code examples to use PHP 8.3+ syntax +- Improved atomic operations documentation +- Enhanced PSR-16 and PSR-6 usage guides +- Updated all engine-specific documentation pages +- Refreshed README with clearer quick start examples +- Added mermaid diagrams for dependency visualization + +## Migration Path from 5.x to 6.0 + +### Step 1: Update PHP Version +Ensure your environment is running PHP 8.3 or later: +```bash +php -v # Should show 8.3.x, 8.4.x, or 8.5.x +``` + +If you're on PHP 8.1 or 8.2, you must upgrade your PHP version before migrating to version 6.0. + +### Step 2: Update Dependencies +Update your `composer.json`: +```bash +composer require byjg/cache-engine:^6.0 +composer update +``` + +### Step 3: Update Development Dependencies (Optional) +If you're using PHPUnit or Psalm in your project: + +**For PHPUnit:** +```bash +composer require --dev phpunit/phpunit:^10.5 +# or +composer require --dev phpunit/phpunit:^11.5 +``` + +**For Psalm:** +```bash +composer require --dev vimeo/psalm:^6.13 +``` + +### Step 4: Test Your Application +Run your existing tests to ensure compatibility: +```bash +vendor/bin/phpunit +``` + +### Step 5: Optional Enhancements +Consider adding the `#[Override]` attribute to your own classes that extend cache engines for better IDE support and code clarity: + +```php +class MyCustomCache extends BaseCacheEngine +{ + #[Override] + public function get(string $key, mixed $default = null): mixed + { + // Your implementation + } +} +``` + +### Step 6: Update CI/CD Pipelines +Update your CI/CD configuration to use PHP 8.3+ in your testing matrix. Remove PHP 8.1 and 8.2 from your test matrix. + +### Common Migration Issues + +**Issue**: Application fails with "PHP version requirement not satisfied" +**Solution**: Upgrade your PHP version to 8.3 or later + +**Issue**: PHPUnit tests fail to run +**Solution**: Upgrade PHPUnit to version 10.5 or later: `composer require --dev phpunit/phpunit:^10.5` + +**Issue**: Psalm reports new errors +**Solution**: If using Psalm 6.x, review and address the stricter type checking. You can temporarily stay on Psalm 5.9 during migration. + +### Testing Your Migration +After completing the migration steps, verify everything works: + +```bash +# Run tests +composer test + +# Run static analysis +composer psalm + +# If using docker-compose +docker compose up -d +composer test +docker compose down +``` + +## Notes + +- All cache engines maintain backward compatibility at the API level +- No changes to PSR-6 or PSR-16 interface implementations +- Existing cache data remains compatible across versions +- The upgrade primarily affects development-time requirements (PHP version, testing tools) + +## Links + +- [Full Commit History](https://github.com/byjg/php-cache-engine/compare/5.0.4...6.0.0) +- [Documentation](https://github.com/byjg/php-cache-engine/tree/master/docs) +- [Report Issues](https://github.com/byjg/php-cache-engine/issues) From 8dfa45f6d7ea5f21e207d4737d7a9247b7d03c69 Mon Sep 17 00:00:00 2001 From: Joao Gilberto Magalhaes Date: Fri, 7 Aug 2026 10:38:39 -0400 Subject: [PATCH 3/6] Allow psr/simple-cache 3.0 The engines already declare the PSR-16 3.0 signatures - get(string $key, mixed $default = null): mixed - so the constraint excluding ^3.0 kept the package from installing alongside anything that requires it. Drop ^1.0 at the same time. PSR-16 1.0 declares get($key, $default = null) with untyped parameters, and adding a type to a parameter the interface leaves untyped is a fatal declaration error, so that branch of the constraint was never usable. --- composer.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/composer.json b/composer.json index 166244d..ac118e7 100644 --- a/composer.json +++ b/composer.json @@ -15,7 +15,7 @@ "php": ">=8.3 <8.6", "psr/cache": "^1.0|^2.0|^3.0", "psr/log": "^1.0|^2.0|^3.0", - "psr/simple-cache": "^1.0|^2.0", + "psr/simple-cache": "^2.0|^3.0", "psr/container": "^1.0|^1.1|^2.0" }, "require-dev": { @@ -29,7 +29,7 @@ }, "provide": { "psr/cache-implementation": "1.0", - "psr/simple-cache-implementation": "1.0" + "psr/simple-cache-implementation": "2.0|3.0" }, "scripts": { "test": "vendor/bin/phpunit", From bd180d8a300284f207d65bb83e198f8bd6917d71 Mon Sep 17 00:00:00 2001 From: Joao Gilberto Magalhaes Date: Fri, 7 Aug 2026 10:49:42 -0400 Subject: [PATCH 4/6] Connect lazily in RedisCacheEngine::has() and clear() Both read $this->redis without calling lazyLoadRedisServer() first, so either one used as the very first operation on a new instance died with "Call to a member function exists() on null". Every other public method already established the connection; these two were simply missed. Surfaced by calling has() as the opening operation against a fresh engine. --- src/Psr16/RedisCacheEngine.php | 4 ++++ tests/CachePSR16Test.php | 23 +++++++++++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/src/Psr16/RedisCacheEngine.php b/src/Psr16/RedisCacheEngine.php index 92f7aee..d873fd5 100644 --- a/src/Psr16/RedisCacheEngine.php +++ b/src/Psr16/RedisCacheEngine.php @@ -233,6 +233,8 @@ public function delete(string $key): bool #[\Override] public function clear(): bool { + $this->lazyLoadRedisServer(); + $iterator = null; do { $keys = $this->redis->scan($iterator, 'cache:*'); @@ -252,6 +254,8 @@ public function clear(): bool #[\Override] public function has(string $key): bool { + $this->lazyLoadRedisServer(); + $result = $this->redis->exists($this->fixKey($key)); return (bool)$result; } diff --git a/tests/CachePSR16Test.php b/tests/CachePSR16Test.php index 2b99e2a..a1c7530 100644 --- a/tests/CachePSR16Test.php +++ b/tests/CachePSR16Test.php @@ -6,7 +6,9 @@ use ByJG\Cache\Exception\InvalidArgumentException; use ByJG\Cache\GarbageCollectorInterface; use ByJG\Cache\Psr16\BaseCacheEngine; +use ByJG\Cache\Psr16\MemcachedEngine; use ByJG\Cache\Psr16\NoCacheEngine; +use ByJG\Cache\Psr16\RedisCacheEngine; use PHPUnit\Framework\Attributes\DataProvider; class CachePSR16Test extends TestBase @@ -397,6 +399,27 @@ public function testAtomicAdd(BaseCacheEngine $cacheEngine) } } + /** + * Every public method has to establish the connection on its own. RedisCacheEngine::has() and + * clear() read $this->redis without calling lazyLoadRedisServer() first, so either one used as + * the very first operation on a new instance died with "call to a member function on null". + * + * Each assertion needs an instance that has never been touched, so the engines are constructed + * here rather than taken from the shared data provider. + */ + public function testEveryEntryPointConnectsOnItsOwn() + { + if (!(new RedisCacheEngine())->isAvailable()) { + $this->markTestSkipped('Redis is not available'); + } + + $this->assertFalse((new RedisCacheEngine())->has('never-touched-key')); + $this->assertTrue((new RedisCacheEngine())->clear()); + + $this->assertFalse((new MemcachedEngine())->has('never-touched-key')); + $this->assertTrue((new MemcachedEngine())->clear()); + } + /** * The TTL argument of the atomic operations has to mean the same thing it means everywhere * else: seconds from now. FileSystem used to hand it straight to the expiry file, which stores From c59a7babcf2cda739d1270f786704c8ec4df543c Mon Sep 17 00:00:00 2001 From: Joao Gilberto Magalhaes Date: Fri, 7 Aug 2026 11:07:12 -0400 Subject: [PATCH 5/6] Document the simple-cache constraint and lazy connect fixes Both landed after the changelog was written. Listed under Bug Fixes rather than Breaking Changes: widening psr/simple-cache to allow 3.0 only unblocks installs, and the dropped ^1.0 branch could never load in the first place. --- CHANGELOG-7.0.md | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/CHANGELOG-7.0.md b/CHANGELOG-7.0.md index 579be28..f4dd56f 100644 --- a/CHANGELOG-7.0.md +++ b/CHANGELOG-7.0.md @@ -93,6 +93,26 @@ process already converted it. `increment()` and `decrement()` set the expiry with a follow-up `EXPIRE`. A crash between the two left a counter with no expiry at all. Both now apply it inside the script. +### `psr/simple-cache` 3.0 is now allowed + +The engines already declare the PSR-16 3.0 signatures, but the constraint stopped at `^2.0`, so the +package could not be installed alongside anything requiring `psr/simple-cache ^3.0`. The constraint +is now `^2.0|^3.0`. + +### Redis: `has()` and `clear()` never opened the connection + +Both read `$this->redis` without calling `lazyLoadRedisServer()` first. Every other public method +established the connection; these two were missed. Calling either as the first operation on a new +instance died with: + +``` +Error: Call to a member function exists() on null +``` + +It went unnoticed because in practice something else — `isAvailable()`, `get()`, `set()` — almost +always ran first and left the connection open. A regression test now exercises each entry point on +an instance that has never been touched. + ### FileSystem: expiry dropped outside the lock `putContents()` deleted the `.ttl` file before acquiring the lock, leaving the value briefly From da4032db601bb7cfea0fc632296c8f76c9e01b62 Mon Sep 17 00:00:00 2001 From: Joao Gilberto Magalhaes Date: Fri, 7 Aug 2026 11:28:37 -0400 Subject: [PATCH 6/6] Build test engines from the environment, not from hardcoded localhost The compare-and-swap and concurrency tests constructed their engines with no arguments, which points them at 127.0.0.1. CI runs the job inside a container, where the service containers are reachable by service name on the Docker network rather than on loopback, so both classes skipped themselves for the whole matrix and the build went green having never exercised a real Redis or Memcached. TestBase already read REDIS_SERVER and MEMCACHED_SERVER for its data provider. That logic now lives in EngineFactory and every test goes through it. make() returns BaseCacheEngine&AtomicOperationInterface&CompareAndSwapInterface, which is enough for the call sites to drop their local @var annotations and carry one engine variable instead of two. --- tests/CompareAndSwapTest.php | 89 +++++++++++++++++------------------- tests/ConcurrencyTest.php | 63 ++++++++++++------------- tests/EngineFactory.php | 45 ++++++++++++++++++ tests/TestBase.php | 22 +-------- 4 files changed, 119 insertions(+), 100 deletions(-) create mode 100644 tests/EngineFactory.php diff --git a/tests/CompareAndSwapTest.php b/tests/CompareAndSwapTest.php index 5815260..33f765c 100644 --- a/tests/CompareAndSwapTest.php +++ b/tests/CompareAndSwapTest.php @@ -4,8 +4,6 @@ use ByJG\Cache\CompareAndSwapInterface; use ByJG\Cache\Psr16\BaseCacheEngine; -use ByJG\Cache\Psr16\MemcachedEngine; -use ByJG\Cache\Psr16\RedisCacheEngine; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\TestCase; @@ -17,8 +15,8 @@ class CompareAndSwapTest extends TestCase public static function engineProvider(): array { return [ - 'redis' => [new RedisCacheEngine()], - 'memcached' => [new MemcachedEngine()], + 'redis' => [EngineFactory::REDIS], + 'memcached' => [EngineFactory::MEMCACHED], ]; } @@ -35,14 +33,13 @@ protected function tearDown(): void * Skips instead of failing so the suite still runs without the docker-compose services up, * which is how the rest of this test suite treats an unreachable backend. */ - private function engineOrSkip(BaseCacheEngine $engine): CompareAndSwapInterface + private function engineOrSkip(string $engineName): BaseCacheEngine&CompareAndSwapInterface { + $engine = EngineFactory::make($engineName); if (!$engine->isAvailable()) { $this->markTestSkipped(get_class($engine) . ' is not available'); } - $this->assertInstanceOf(CompareAndSwapInterface::class, $engine); - $this->started[] = $engine; $engine->delete('cas-key'); @@ -50,60 +47,60 @@ private function engineOrSkip(BaseCacheEngine $engine): CompareAndSwapInterface } #[DataProvider('engineProvider')] - public function testSetIfAbsentSucceedsOnlyForTheFirstCaller(BaseCacheEngine $engine): void + public function testSetIfAbsentSucceedsOnlyForTheFirstCaller(string $engineName): void { - $cas = $this->engineOrSkip($engine); + $engine = $this->engineOrSkip($engineName); - $this->assertTrue($cas->setIfAbsent('cas-key', 'token-a', 30)); - $this->assertFalse($cas->setIfAbsent('cas-key', 'token-b', 30)); + $this->assertTrue($engine->setIfAbsent('cas-key', 'token-a', 30)); + $this->assertFalse($engine->setIfAbsent('cas-key', 'token-b', 30)); $this->assertEquals('token-a', $engine->get('cas-key'), 'The loser must not have overwritten the winner'); } #[DataProvider('engineProvider')] - public function testSetIfAbsentSucceedsAgainOnceTheTtlHasPassed(BaseCacheEngine $engine): void + public function testSetIfAbsentSucceedsAgainOnceTheTtlHasPassed(string $engineName): void { - $cas = $this->engineOrSkip($engine); + $engine = $this->engineOrSkip($engineName); - $this->assertTrue($cas->setIfAbsent('cas-key', 'token-a', 1)); - $this->assertFalse($cas->setIfAbsent('cas-key', 'token-b', 1)); + $this->assertTrue($engine->setIfAbsent('cas-key', 'token-a', 1)); + $this->assertFalse($engine->setIfAbsent('cas-key', 'token-b', 1)); sleep(2); - $this->assertTrue($cas->setIfAbsent('cas-key', 'token-b', 30)); + $this->assertTrue($engine->setIfAbsent('cas-key', 'token-b', 30)); $this->assertEquals('token-b', $engine->get('cas-key')); } #[DataProvider('engineProvider')] - public function testSetIfAbsentAppliesTheTtlWithTheWrite(BaseCacheEngine $engine): void + public function testSetIfAbsentAppliesTheTtlWithTheWrite(string $engineName): void { - $cas = $this->engineOrSkip($engine); + $engine = $this->engineOrSkip($engineName); - $cas->setIfAbsent('cas-key', 'token-a', 1); + $engine->setIfAbsent('cas-key', 'token-a', 1); sleep(2); $this->assertNull($engine->get('cas-key'), 'The key must expire on its own, with no second command'); } #[DataProvider('engineProvider')] - public function testDeleteIfEqualsOnlyRemovesTheMatchingValue(BaseCacheEngine $engine): void + public function testDeleteIfEqualsOnlyRemovesTheMatchingValue(string $engineName): void { - $cas = $this->engineOrSkip($engine); + $engine = $this->engineOrSkip($engineName); - $cas->setIfAbsent('cas-key', 'token-a', 30); + $engine->setIfAbsent('cas-key', 'token-a', 30); - $this->assertFalse($cas->deleteIfEquals('cas-key', 'token-b')); + $this->assertFalse($engine->deleteIfEquals('cas-key', 'token-b')); $this->assertEquals('token-a', $engine->get('cas-key'), 'A non-matching caller must not delete'); - $this->assertTrue($cas->deleteIfEquals('cas-key', 'token-a')); + $this->assertTrue($engine->deleteIfEquals('cas-key', 'token-a')); $this->assertNull($engine->get('cas-key')); } #[DataProvider('engineProvider')] - public function testDeleteIfEqualsOnAMissingKeyReturnsFalse(BaseCacheEngine $engine): void + public function testDeleteIfEqualsOnAMissingKeyReturnsFalse(string $engineName): void { - $cas = $this->engineOrSkip($engine); + $engine = $this->engineOrSkip($engineName); - $this->assertFalse($cas->deleteIfEquals('cas-key', 'token-a')); + $this->assertFalse($engine->deleteIfEquals('cas-key', 'token-a')); } /** @@ -111,26 +108,26 @@ public function testDeleteIfEqualsOnAMissingKeyReturnsFalse(BaseCacheEngine $eng * delete the key that somebody else has legitimately taken over in the meantime. */ #[DataProvider('engineProvider')] - public function testAnExpiredOwnerCannotDeleteTheNewOwnersValue(BaseCacheEngine $engine): void + public function testAnExpiredOwnerCannotDeleteTheNewOwnersValue(string $engineName): void { - $cas = $this->engineOrSkip($engine); + $engine = $this->engineOrSkip($engineName); - $cas->setIfAbsent('cas-key', 'stale-owner', 1); + $engine->setIfAbsent('cas-key', 'stale-owner', 1); sleep(2); - $this->assertTrue($cas->setIfAbsent('cas-key', 'new-owner', 30)); + $this->assertTrue($engine->setIfAbsent('cas-key', 'new-owner', 30)); - $this->assertFalse($cas->deleteIfEquals('cas-key', 'stale-owner')); - $this->assertFalse($cas->expireIfEquals('cas-key', 'stale-owner', 30)); + $this->assertFalse($engine->deleteIfEquals('cas-key', 'stale-owner')); + $this->assertFalse($engine->expireIfEquals('cas-key', 'stale-owner', 30)); $this->assertEquals('new-owner', $engine->get('cas-key')); } #[DataProvider('engineProvider')] - public function testExpireIfEqualsExtendsTheLifetimeOfAMatchingValue(BaseCacheEngine $engine): void + public function testExpireIfEqualsExtendsTheLifetimeOfAMatchingValue(string $engineName): void { - $cas = $this->engineOrSkip($engine); + $engine = $this->engineOrSkip($engineName); - $cas->setIfAbsent('cas-key', 'token-a', 1); - $this->assertTrue($cas->expireIfEquals('cas-key', 'token-a', 30)); + $engine->setIfAbsent('cas-key', 'token-a', 1); + $this->assertTrue($engine->expireIfEquals('cas-key', 'token-a', 30)); sleep(2); @@ -138,12 +135,12 @@ public function testExpireIfEqualsExtendsTheLifetimeOfAMatchingValue(BaseCacheEn } #[DataProvider('engineProvider')] - public function testExpireIfEqualsIsRejectedForANonMatchingValue(BaseCacheEngine $engine): void + public function testExpireIfEqualsIsRejectedForANonMatchingValue(string $engineName): void { - $cas = $this->engineOrSkip($engine); + $engine = $this->engineOrSkip($engineName); - $cas->setIfAbsent('cas-key', 'token-a', 1); - $this->assertFalse($cas->expireIfEquals('cas-key', 'token-b', 30)); + $engine->setIfAbsent('cas-key', 'token-a', 1); + $this->assertFalse($engine->expireIfEquals('cas-key', 'token-b', 30)); sleep(2); @@ -151,14 +148,14 @@ public function testExpireIfEqualsIsRejectedForANonMatchingValue(BaseCacheEngine } #[DataProvider('engineProvider')] - public function testSetIfAbsentRoundTripsNonScalarValues(BaseCacheEngine $engine): void + public function testSetIfAbsentRoundTripsNonScalarValues(string $engineName): void { - $cas = $this->engineOrSkip($engine); + $engine = $this->engineOrSkip($engineName); - $this->assertTrue($cas->setIfAbsent('cas-key', ['a' => 1, 'b' => 2], 30)); + $this->assertTrue($engine->setIfAbsent('cas-key', ['a' => 1, 'b' => 2], 30)); $this->assertEquals(['a' => 1, 'b' => 2], $engine->get('cas-key')); - $this->assertFalse($cas->deleteIfEquals('cas-key', ['a' => 1])); - $this->assertTrue($cas->deleteIfEquals('cas-key', ['a' => 1, 'b' => 2])); + $this->assertFalse($engine->deleteIfEquals('cas-key', ['a' => 1])); + $this->assertTrue($engine->deleteIfEquals('cas-key', ['a' => 1, 'b' => 2])); } } diff --git a/tests/ConcurrencyTest.php b/tests/ConcurrencyTest.php index e65ff64..61ba1a6 100644 --- a/tests/ConcurrencyTest.php +++ b/tests/ConcurrencyTest.php @@ -4,8 +4,6 @@ use ByJG\Cache\CompareAndSwapInterface; use ByJG\Cache\Psr16\BaseCacheEngine; -use ByJG\Cache\Psr16\MemcachedEngine; -use ByJG\Cache\Psr16\RedisCacheEngine; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\Attributes\Group; use PHPUnit\Framework\TestCase; @@ -26,8 +24,8 @@ class ConcurrencyTest extends TestCase public static function engineProvider(): array { return [ - 'redis' => [RedisCacheEngine::class], - 'memcached' => [MemcachedEngine::class], + 'redis' => [EngineFactory::REDIS], + 'memcached' => [EngineFactory::MEMCACHED], ]; } @@ -82,11 +80,11 @@ private function fork(callable $child): array * back results: the children inherit the parent's open socket and close it as they exit, which * leaves the parent's own connection dead. That is a property of forking, not of the engine. */ - private function engineOrSkip(string $engineClass): BaseCacheEngine + private function engineOrSkip(string $engineName): BaseCacheEngine&CompareAndSwapInterface { - $engine = new $engineClass(); + $engine = EngineFactory::make($engineName); if (!$engine->isAvailable()) { - $this->markTestSkipped("$engineClass is not available"); + $this->markTestSkipped("$engineName is not available"); } return $engine; @@ -97,13 +95,12 @@ private function engineOrSkip(string $engineClass): BaseCacheEngine * A has()-then-set() implementation lets several win here. */ #[DataProvider('engineProvider')] - public function testExactlyOneProcessWinsSetIfAbsent(string $engineClass): void + public function testExactlyOneProcessWinsSetIfAbsent(string $engineName): void { - $this->engineOrSkip($engineClass)->delete('race-lock'); + $this->engineOrSkip($engineName)->delete('race-lock'); - $codes = $this->fork(function (int $i) use ($engineClass): bool { - /** @var CompareAndSwapInterface $engine */ - $engine = new $engineClass(); + $codes = $this->fork(function (int $i) use ($engineName): bool { + $engine = EngineFactory::make($engineName); return $engine->setIfAbsent('race-lock', "token-$i", 60); }); @@ -117,19 +114,19 @@ public function testExactlyOneProcessWinsSetIfAbsent(string $engineClass): void * increment and resets the counter, so the value 1 gets issued twice. */ #[DataProvider('engineProvider')] - public function testIncrementNeverIssuesTheSameValueTwice(string $engineClass): void + public function testIncrementNeverIssuesTheSameValueTwice(string $engineName): void { - $this->engineOrSkip($engineClass)->delete('race-counter'); + $this->engineOrSkip($engineName)->delete('race-counter'); - $this->fork(function () use ($engineClass): bool { - $engine = new $engineClass(); + $this->fork(function () use ($engineName): bool { + $engine = EngineFactory::make($engineName); $engine->increment('race-counter'); return true; }); $this->assertEquals( self::CHILDREN, - $this->engineOrSkip($engineClass)->get('race-counter'), + $this->engineOrSkip($engineName)->get('race-counter'), 'Every increment must be reflected exactly once in the final counter' ); } @@ -139,17 +136,17 @@ public function testIncrementNeverIssuesTheSameValueTwice(string $engineClass): * updates landed between another process's read and its write. */ #[DataProvider('engineProvider')] - public function testConcurrentAddLosesNothing(string $engineClass): void + public function testConcurrentAddLosesNothing(string $engineName): void { - $this->engineOrSkip($engineClass)->delete('race-list'); + $this->engineOrSkip($engineName)->delete('race-list'); - $this->fork(function (int $i) use ($engineClass): bool { - $engine = new $engineClass(); + $this->fork(function (int $i) use ($engineName): bool { + $engine = EngineFactory::make($engineName); $engine->add('race-list', "item-$i"); return true; }); - $stored = $this->engineOrSkip($engineClass)->get('race-list'); + $stored = $this->engineOrSkip($engineName)->get('race-list'); $this->assertIsArray($stored); $this->assertCount(self::CHILDREN, $stored, 'Every appended item must still be present'); @@ -165,19 +162,19 @@ public function testConcurrentAddLosesNothing(string $engineClass): void * mid-way through rebuilding, which both duplicates the original value and drops appends. */ #[DataProvider('engineProvider')] - public function testConcurrentAddSurvivesTheConversionFromAPlainValue(string $engineClass): void + public function testConcurrentAddSurvivesTheConversionFromAPlainValue(string $engineName): void { - $setup = $this->engineOrSkip($engineClass); + $setup = $this->engineOrSkip($engineName); $setup->delete('race-convert'); $setup->set('race-convert', 'seed'); - $this->fork(function (int $i) use ($engineClass): bool { - $engine = new $engineClass(); + $this->fork(function (int $i) use ($engineName): bool { + $engine = EngineFactory::make($engineName); $engine->add('race-convert', "item-$i"); return true; }); - $stored = $this->engineOrSkip($engineClass)->get('race-convert'); + $stored = $this->engineOrSkip($engineName)->get('race-convert'); $this->assertIsArray($stored); sort($stored); @@ -191,22 +188,20 @@ public function testConcurrentAddSurvivesTheConversionFromAPlainValue(string $en * remove it, no matter how many others try at the same instant. */ #[DataProvider('engineProvider')] - public function testOnlyTheOwnerCanDeleteUnderContention(string $engineClass): void + public function testOnlyTheOwnerCanDeleteUnderContention(string $engineName): void { - /** @var BaseCacheEngine&CompareAndSwapInterface $setup */ - $setup = $this->engineOrSkip($engineClass); + $setup = $this->engineOrSkip($engineName); $setup->delete('race-owned'); $setup->setIfAbsent('race-owned', 'the-owner', 60); - $codes = $this->fork(function (int $i) use ($engineClass): bool { - /** @var CompareAndSwapInterface $engine */ - $engine = new $engineClass(); + $codes = $this->fork(function (int $i) use ($engineName): bool { + $engine = EngineFactory::make($engineName); return $engine->deleteIfEquals('race-owned', "impostor-$i"); }); $this->assertSame(0, count(array_filter($codes, fn($code) => $code === 0)), 'No impostor may succeed'); - $verify = $this->engineOrSkip($engineClass); + $verify = $this->engineOrSkip($engineName); $this->assertEquals('the-owner', $verify->get('race-owned'), 'The key must be untouched'); $verify->delete('race-owned'); } diff --git a/tests/EngineFactory.php b/tests/EngineFactory.php new file mode 100644 index 0000000..1032169 --- /dev/null +++ b/tests/EngineFactory.php @@ -0,0 +1,45 @@ + self::redis(), + self::MEMCACHED => self::memcached(), + default => throw new \InvalidArgumentException("Unknown engine '$engine'"), + }; + } + + public static function redis(): RedisCacheEngine + { + return new RedisCacheEngine( + getenv('REDIS_SERVER') ?: '127.0.0.1:6379', + getenv('REDIS_PASSWORD') ?: '' + ); + } + + public static function memcached(): MemcachedEngine + { + return new MemcachedEngine([getenv('MEMCACHED_SERVER') ?: '127.0.0.1:11211']); + } +} diff --git a/tests/TestBase.php b/tests/TestBase.php index 0e8d2f9..9497438 100644 --- a/tests/TestBase.php +++ b/tests/TestBase.php @@ -5,9 +5,7 @@ use ByJG\Cache\Psr16\ArrayCacheEngine; use ByJG\Cache\Psr16\BaseCacheEngine; use ByJG\Cache\Psr16\FileSystemCacheEngine; -use ByJG\Cache\Psr16\MemcachedEngine; use ByJG\Cache\Psr16\NoCacheEngine; -use ByJG\Cache\Psr16\RedisCacheEngine; use ByJG\Cache\Psr16\SessionCacheEngine; use ByJG\Cache\Psr16\ShmopCacheEngine; use ByJG\Cache\Psr16\TmpfsCacheEngine; @@ -32,22 +30,6 @@ protected function tearDown(): void public static function CachePoolProvider() { - if (getenv('MEMCACHED_SERVER')) { - $memcachedServer = [getenv('MEMCACHED_SERVER')]; - } else { - $memcachedServer = ['127.0.0.1:11211']; - } - if (getenv('REDIS_SERVER')) { - $redisCacheServer = getenv('REDIS_SERVER'); - } else { - $redisCacheServer = '127.0.0.1:6379'; - } - if (getenv('REDIS_PASSWORD')) { - $redisPassword = getenv('REDIS_PASSWORD'); - } else { - $redisPassword = ''; - } - return [ 'Array' => [ new ArrayCacheEngine() @@ -68,10 +50,10 @@ public static function CachePoolProvider() new NoCacheEngine() ], 'Memcached' => [ - new MemcachedEngine($memcachedServer) + EngineFactory::memcached() ], 'Redis' => [ - new RedisCacheEngine($redisCacheServer, $redisPassword) + EngineFactory::redis() ], 'Memory' => [ new TmpfsCacheEngine()