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) diff --git a/CHANGELOG-7.0.md b/CHANGELOG-7.0.md new file mode 100644 index 0000000..f4dd56f --- /dev/null +++ b/CHANGELOG-7.0.md @@ -0,0 +1,212 @@ +# 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. + +### `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 +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/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", 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..d873fd5 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); } } @@ -159,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:*'); @@ -178,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; } @@ -200,55 +278,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); - $result = $this->redis->decr($this->fixKey($key), $value); + if (is_array($list)) { + return array_map(fn($item) => $this->decode($item), $list); + } - if ($ttl) { - $this->redis->expire($this->fixKey($key), $this->convertToSeconds($ttl)); + $this->convertStringToList($key); } - return $result; + throw new StorageErrorException("Could not append to '$key': the key kept changing type"); + } + + /** + * 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. + } + + $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; + } - $result = $this->redis->rPush($fixKey, is_object($value) || is_array($value) ? serialize($value) : $value); + #[\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)] + ); + } + + /** 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..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 @@ -396,4 +398,57 @@ public function testAtomicAdd(BaseCacheEngine $cacheEngine) $this->markTestIncomplete('Does not support atomic add or it is native'); } } + + /** + * 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 + * 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..33f765c --- /dev/null +++ b/tests/CompareAndSwapTest.php @@ -0,0 +1,161 @@ + [EngineFactory::REDIS], + 'memcached' => [EngineFactory::MEMCACHED], + ]; + } + + #[\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(string $engineName): BaseCacheEngine&CompareAndSwapInterface + { + $engine = EngineFactory::make($engineName); + if (!$engine->isAvailable()) { + $this->markTestSkipped(get_class($engine) . ' is not available'); + } + + $this->started[] = $engine; + $engine->delete('cas-key'); + + return $engine; + } + + #[DataProvider('engineProvider')] + public function testSetIfAbsentSucceedsOnlyForTheFirstCaller(string $engineName): void + { + $engine = $this->engineOrSkip($engineName); + + $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(string $engineName): void + { + $engine = $this->engineOrSkip($engineName); + + $this->assertTrue($engine->setIfAbsent('cas-key', 'token-a', 1)); + $this->assertFalse($engine->setIfAbsent('cas-key', 'token-b', 1)); + + sleep(2); + + $this->assertTrue($engine->setIfAbsent('cas-key', 'token-b', 30)); + $this->assertEquals('token-b', $engine->get('cas-key')); + } + + #[DataProvider('engineProvider')] + public function testSetIfAbsentAppliesTheTtlWithTheWrite(string $engineName): void + { + $engine = $this->engineOrSkip($engineName); + + $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(string $engineName): void + { + $engine = $this->engineOrSkip($engineName); + + $engine->setIfAbsent('cas-key', 'token-a', 30); + + $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($engine->deleteIfEquals('cas-key', 'token-a')); + $this->assertNull($engine->get('cas-key')); + } + + #[DataProvider('engineProvider')] + public function testDeleteIfEqualsOnAMissingKeyReturnsFalse(string $engineName): void + { + $engine = $this->engineOrSkip($engineName); + + $this->assertFalse($engine->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(string $engineName): void + { + $engine = $this->engineOrSkip($engineName); + + $engine->setIfAbsent('cas-key', 'stale-owner', 1); + sleep(2); + $this->assertTrue($engine->setIfAbsent('cas-key', 'new-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(string $engineName): void + { + $engine = $this->engineOrSkip($engineName); + + $engine->setIfAbsent('cas-key', 'token-a', 1); + $this->assertTrue($engine->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(string $engineName): void + { + $engine = $this->engineOrSkip($engineName); + + $engine->setIfAbsent('cas-key', 'token-a', 1); + $this->assertFalse($engine->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(string $engineName): void + { + $engine = $this->engineOrSkip($engineName); + + $this->assertTrue($engine->setIfAbsent('cas-key', ['a' => 1, 'b' => 2], 30)); + $this->assertEquals(['a' => 1, 'b' => 2], $engine->get('cas-key')); + + $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 new file mode 100644 index 0000000..61ba1a6 --- /dev/null +++ b/tests/ConcurrencyTest.php @@ -0,0 +1,208 @@ + [EngineFactory::REDIS], + 'memcached' => [EngineFactory::MEMCACHED], + ]; + } + + #[\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 $engineName): BaseCacheEngine&CompareAndSwapInterface + { + $engine = EngineFactory::make($engineName); + if (!$engine->isAvailable()) { + $this->markTestSkipped("$engineName 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 $engineName): void + { + $this->engineOrSkip($engineName)->delete('race-lock'); + + $codes = $this->fork(function (int $i) use ($engineName): bool { + $engine = EngineFactory::make($engineName); + 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 $engineName): void + { + $this->engineOrSkip($engineName)->delete('race-counter'); + + $this->fork(function () use ($engineName): bool { + $engine = EngineFactory::make($engineName); + $engine->increment('race-counter'); + return true; + }); + + $this->assertEquals( + self::CHILDREN, + $this->engineOrSkip($engineName)->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 $engineName): void + { + $this->engineOrSkip($engineName)->delete('race-list'); + + $this->fork(function (int $i) use ($engineName): bool { + $engine = EngineFactory::make($engineName); + $engine->add('race-list', "item-$i"); + return true; + }); + + $stored = $this->engineOrSkip($engineName)->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 $engineName): void + { + $setup = $this->engineOrSkip($engineName); + $setup->delete('race-convert'); + $setup->set('race-convert', 'seed'); + + $this->fork(function (int $i) use ($engineName): bool { + $engine = EngineFactory::make($engineName); + $engine->add('race-convert', "item-$i"); + return true; + }); + + $stored = $this->engineOrSkip($engineName)->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 $engineName): void + { + $setup = $this->engineOrSkip($engineName); + $setup->delete('race-owned'); + $setup->setIfAbsent('race-owned', 'the-owner', 60); + + $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($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()