diff --git a/src/Cache/SysVCacheItemPool.php b/src/Cache/SysVCacheItemPool.php index 7821d6b09..3b0a2488b 100644 --- a/src/Cache/SysVCacheItemPool.php +++ b/src/Cache/SysVCacheItemPool.php @@ -18,6 +18,8 @@ use Psr\Cache\CacheItemInterface; use Psr\Cache\CacheItemPoolInterface; +use SysvSemaphore; +use SysvSharedMemory; /** * SystemV shared memory based CacheItemPool implementation. @@ -32,6 +34,8 @@ class SysVCacheItemPool implements CacheItemPoolInterface const DEFAULT_PROJ = 'A'; + const DEFAULT_SEM_PROJ = 'B'; + const DEFAULT_MEMSIZE = 10000; const DEFAULT_PERM = 0600; @@ -61,6 +65,18 @@ class SysVCacheItemPool implements CacheItemPoolInterface */ private $hasLoadedItems = false; + /** + * @var SysvSemaphore|false + */ + private SysvSemaphore|false $semId = false; + + /** + * Maintain the process which is currently holding the semaphore to prevent deadlock. + * + * @var int|null + */ + private ?int $lockOwnerPid = null; + /** * Create a SystemV shared memory based CacheItemPool. * @@ -70,13 +86,16 @@ class SysVCacheItemPool implements CacheItemPoolInterface * @type int $variableKey The variable key for getting the data from the shared memory. **Defaults to** 1. * @type string $proj The project identifier for ftok. This needs to be a one character string. * **Defaults to** 'A'. + * @type string $semProj The project identifier for ftok to provide to `sem_get`. This needs to be a one + * character string. + * **Defaults to** 'B'. * @type int $memsize The memory size in bytes for shm_attach. **Defaults to** 10000. * @type int $perm The permission for shm_attach. **Defaults to** 0600. * } */ public function __construct($options = []) { - if (! extension_loaded('sysvshm')) { + if (!extension_loaded('sysvshm')) { throw new \RuntimeException( 'sysvshm extension is required to use this ItemPool' ); @@ -84,12 +103,20 @@ public function __construct($options = []) $this->options = $options + [ 'variableKey' => self::VAR_KEY, 'proj' => self::DEFAULT_PROJ, + 'semProj' => self::DEFAULT_SEM_PROJ, 'memsize' => self::DEFAULT_MEMSIZE, 'perm' => self::DEFAULT_PERM ]; $this->items = []; $this->deferredItems = []; $this->sysvKey = ftok(__FILE__, $this->options['proj']); + + // gracefully handle when `sysvsem` isn't loaded + // @TODO(v2): throw an exception when the extension isn't loaded + if (extension_loaded('sysvsem')) { + $semKey = ftok(__FILE__, $this->options['semProj']); + $this->semId = sem_get($semKey, 1, $this->options['perm'], true); + } } /** @@ -132,9 +159,17 @@ public function hasItem($key): bool */ public function clear(): bool { + if (!$this->acquireLock()) { + return false; + } + $this->items = []; $this->deferredItems = []; - return $this->saveCurrentItems(); + $ret = $this->saveCurrentItems(); + + $this->resetShm(); + $this->releaseLock(); + return $ret; } /** @@ -150,6 +185,10 @@ public function deleteItem($key): bool */ public function deleteItems(array $keys): bool { + if (!$this->acquireLock()) { + return false; + } + if (!$this->hasLoadedItems) { $this->loadItems(); } @@ -157,7 +196,11 @@ public function deleteItems(array $keys): bool foreach ($keys as $key) { unset($this->items[$key]); } - return $this->saveCurrentItems(); + $ret = $this->saveCurrentItems(); + + $this->resetShm(); + $this->releaseLock(); + return $ret; } /** @@ -165,12 +208,18 @@ public function deleteItems(array $keys): bool */ public function save(CacheItemInterface $item): bool { + if (!$this->acquireLock()) { + return false; + } + if (!$this->hasLoadedItems) { $this->loadItems(); } $this->items[$item->getKey()] = $item; - return $this->saveCurrentItems(); + $ret = $this->saveCurrentItems(); + $this->releaseLock(); + return $ret; } /** @@ -187,12 +236,18 @@ public function saveDeferred(CacheItemInterface $item): bool */ public function commit(): bool { + if (!$this->acquireLock()) { + return false; + } + foreach ($this->deferredItems as $item) { if ($this->save($item) === false) { + $this->releaseLock(); return false; } } $this->deferredItems = []; + $this->releaseLock(); return true; } @@ -203,20 +258,21 @@ public function commit(): bool */ private function saveCurrentItems() { - $shmid = shm_attach( - $this->sysvKey, - $this->options['memsize'], - $this->options['perm'] - ); - if ($shmid !== false) { - $ret = shm_put_var( + if (!$this->acquireLock()) { + return false; + } + + if (false !== $shmid = $this->attachShm()) { + $success = shm_put_var( $shmid, $this->options['variableKey'], $this->items ); shm_detach($shmid); - return $ret; + $this->releaseLock(); + return $success; } + $this->releaseLock(); return false; } @@ -227,22 +283,70 @@ private function saveCurrentItems() */ private function loadItems() { - $shmid = shm_attach( - $this->sysvKey, - $this->options['memsize'], - $this->options['perm'] - ); - if ($shmid !== false) { + if (!$this->acquireLock()) { + return false; + } + + if (false !== $shmid = $this->attachShm()) { $data = @shm_get_var($shmid, $this->options['variableKey']); - if (!empty($data)) { - $this->items = $data; - } else { - $this->items = []; - } + $this->items = $data ?: []; shm_detach($shmid); $this->hasLoadedItems = true; + $this->releaseLock(); + return true; + } + $this->releaseLock(); + return false; + } + + private function acquireLock(): bool + { + if ($this->semId === false) { + // if `sysvsem` isn't loaded, or if `sem_get` fails, return true + // this ensures BC with previous versions of the auth library. + // @TODO consider better handling when `sem_get` fails. + return true; + } + + $currentPid = getmypid(); + if ($this->lockOwnerPid === $currentPid) { + // We already have the lock + return true; + } + + if (sem_acquire($this->semId)) { + $this->lockOwnerPid = (int) $currentPid; return true; } return false; } + + private function releaseLock(): bool + { + if ($this->semId === false || $this->lockOwnerPid !== getmypid()) { + return true; + } + + $this->lockOwnerPid = null; + return sem_release($this->semId); + } + + private function resetShm(): void + { + // Remove the shared memory segment and semaphore when clearing the cache + $shmid = @shm_attach($this->sysvKey); + if ($shmid !== false) { + @shm_remove($shmid); + @shm_detach($shmid); + } + } + + private function attachShm(): SysvSharedMemory|false + { + return shm_attach( + $this->sysvKey, + $this->options['memsize'], + $this->options['perm'] + ); + } } diff --git a/tests/Cache/RaceConditionTest.php b/tests/Cache/RaceConditionTest.php index 88dd45946..f6b9e3216 100644 --- a/tests/Cache/RaceConditionTest.php +++ b/tests/Cache/RaceConditionTest.php @@ -45,8 +45,7 @@ public function testRaceCondition(string $cacheClass) if (!function_exists('pcntl_fork')) { $this->markTestSkipped('pcntl_fork is not available'); } - for ($i = 0; $i < 100; $i++) { - + for ($i = 0; $i < 50; $i++) { $pids = []; for ($j = 0; $j < 4; $j++) { $pid = pcntl_fork(); @@ -56,7 +55,7 @@ public function testRaceCondition(string $cacheClass) $pool = $this->createCacheItemPool($cacheClass); $item = $pool->getItem('foo'); $item->set('bar'); - $pool->save($item); + $this->assertTrue($pool->save($item)); if ($pid) { // parent @@ -68,7 +67,7 @@ public function testRaceCondition(string $cacheClass) } // parent - $pool->save($item); + $this->assertTrue($pool->save($item)); foreach ($pids as $pid) { pcntl_waitpid($pid, $status); @@ -78,6 +77,8 @@ public function testRaceCondition(string $cacheClass) $this->assertTrue($pool->hasItem('foo')); $cachedItem = $pool->getItem('foo'); $this->assertEquals('bar', $cachedItem->get()); + + $pool->clear(); } } diff --git a/tests/Cache/SysVCacheItemPoolTest.php b/tests/Cache/SysVCacheItemPoolTest.php index 2c2d5be2e..d85e60152 100644 --- a/tests/Cache/SysVCacheItemPoolTest.php +++ b/tests/Cache/SysVCacheItemPoolTest.php @@ -23,6 +23,8 @@ class SysVCacheItemPoolTest extends BaseTest { + const VARIABLE_KEY = 99; + private $pool; public function setUp(): void @@ -32,10 +34,17 @@ public function setUp(): void 'sysvshm extension is required for running the test' ); } - $this->pool = new SysVCacheItemPool(['variableKey' => 99]); + $this->pool = new SysVCacheItemPool(['variableKey' => self::VARIABLE_KEY]); $this->pool->clear(); } + public function tearDown(): void + { + if (extension_loaded('sysvshm')) { + $this->pool->clear(); + } + } + public function saveItem($key, $value) { $item = $this->pool->getItem($key); @@ -158,4 +167,39 @@ public function testCommitsDeferredItems() $this->pool->getItem($keys[1])->get() ); } + + public function testRaceCondition() + { + if (!extension_loaded('sysvsem')) { + $this->markTestSkipped( + 'sysvsem extension is required for running the race condition test' + ); + } + + $key = 'race-item'; + $initialValue = 0; + $this->saveItem($key, $initialValue); + + $numProcesses = 100; + $processes = []; + for ($i = 0; $i < $numProcesses; $i++) { + $command = sprintf( + 'php %s/sysv_cache_race_condition_writer.php %s %s', + __DIR__, + $key, + self::VARIABLE_KEY + ); + $processes[] = proc_open($command, [], $pipes); + } + + foreach ($processes as $process) { + // proc_close waits for the process to terminate and returns its exit code. + // This ensures that all child processes have completed their writes + // before the parent process proceeds to read the final value. + proc_close($process); + } + + $finalValue = $this->pool->getItem($key)->get(); + $this->assertEquals($numProcesses, $finalValue); + } } diff --git a/tests/Cache/sysv_cache_race_condition_writer.php b/tests/Cache/sysv_cache_race_condition_writer.php new file mode 100644 index 000000000..384db8fcd --- /dev/null +++ b/tests/Cache/sysv_cache_race_condition_writer.php @@ -0,0 +1,26 @@ + $argv[2]]); + +$key = $argv[1]; + +$semKey = ftok(__FILE__, 'B'); +$semId = sem_get($semKey); +if (sem_acquire($semId)) { + $item = $pool->getItem($key); + $value = (int) $item->get(); + $value++; + usleep(10000); // Simulate some work + $item->set($value); + $pool->save($item); + + sem_release($semId); +}