Conversation
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.
There was a problem hiding this comment.
Psalm found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
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.
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.
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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
CompareAndSwapInterface— the conditional-write primitive needed to build a correctdistributed lock on top of a cache engine — and fixes three genuine race conditions in the existing
atomic operations.
The races are not theoretical. Forked-process tests reproduce each one against
master:increment()calls on Memcached ended at 8 instead of 20.add()calls on Redis duplicated the seed value 16 times and dropped appends.CompareAndSwapInterface
Implemented by
RedisCacheEngine(SET NX EXplus Lua for the guarded operations) andMemcachedEngine(nativeadd()pluscas()).Deliberately a separate interface rather than an addition to
AtomicOperationInterface, sonothing that already implements the latter breaks. Callers probe with
instanceofand degradeexplicitly.
FileSystemCacheEnginedoes not implement it.flock()attaches to an inode rather than a path,and the engine unlinks the file on delete, so two processes can end up holding "the lock" on two
different inodes at the same path. The file system is not a reliable substrate for mutual exclusion
and the engine should not pretend otherwise.
Bug fixes
Memcached — non-atomic seeding in all three atomic operations.
increment,decrementandaddinitialised a missing key withget() === falsethenset(). Memcached's ownincrement()is atomic, but those two preparatory calls are not:
Now seeded with
Memcached::add(), which the server resolves in a single step.Redis —
add()corrupted the list when converting from a plain value. The firstadd()to akey written by
set()didGET→DEL→ re-push →RPUSHunprotected, so concurrent callers eachdeleted a list the others were rebuilding. Now a compare-guarded script that only rewrites the key
while it still holds the value that was read.
Redis — TTL applied as a separate command.
increment/decrementset the expiry with afollow-up
EXPIRE; a crash in between left a counter with no expiry. Both now run as one script.FileSystem — expiry dropped outside the lock, and a broken TTL conversion. The
.ttlfile wasdeleted before the lock was taken, leaving the value briefly immortal. The atomic operations also
passed a relative TTL where an absolute timestamp was expected, so
increment($key, 1, 60)wrote anexpiry in 1970.
Breaking changes
INCR/RPUSH+ separateEXPIREEVALto be availableadd()TTLaddToNow()Full detail and migration steps in
CHANGELOG-7.0.md.Testing
tests/CompareAndSwapTest.php— 18 tests across both engines: expiry, non-owner rejection, andthe stale-owner-versus-new-owner case.
tests/ConcurrencyTest.php— forks twenty real processes at a synchronised barrier. Sequentialtests cannot prove atomicity. Each of these was verified to fail against
masterbefore beingaccepted.
tests/CachePSR16Test.phpfor every engine implementingAtomicOperationInterface.181 tests, 1107 assertions passing. Psalm clean against a clean baseline.
Also included
CHANGELOG-6.0.md, which was sitting untracked in the working tree, committed separately.