diff --git a/cache/classes/loaders.php b/cache/classes/loaders.php index 3d650feb564..5651aca9478 100644 --- a/cache/classes/loaders.php +++ b/cache/classes/loaders.php @@ -1676,24 +1676,34 @@ class cache_application extends cache implements cache_loader_with_locking { * @return bool Returns true if the lock could be acquired, false otherwise. */ public function acquire_lock($key) { - global $CFG; + $releaseparent = false; if ($this->get_loader() !== false) { - $this->get_loader()->acquire_lock($key); + if (!$this->get_loader()->acquire_lock($key)) { + return false; + } + // We need to release this lock later if the lock is not successful. + $releaseparent = true; } - $key = cache_helper::hash_key($key, $this->get_definition()); + $hashedkey = cache_helper::hash_key($key, $this->get_definition()); $before = microtime(true); if ($this->nativelocking) { - $lock = $this->get_store()->acquire_lock($key, $this->get_identifier()); + $lock = $this->get_store()->acquire_lock($hashedkey, $this->get_identifier()); } else { $this->ensure_cachelock_available(); - $lock = $this->cachelockinstance->lock($key, $this->get_identifier()); + $lock = $this->cachelockinstance->lock($hashedkey, $this->get_identifier()); } $after = microtime(true); if ($lock) { - $this->locks[$key] = $lock; + $this->locks[$hashedkey] = $lock; if ((defined('MDL_PERF') && MDL_PERF) || $this->perfdebug) { \core\lock\timing_wrapper_lock_factory::record_lock_data($after, $before, - $this->get_definition()->get_id(), $key, $lock, $this->get_identifier() . $key); + $this->get_definition()->get_id(), $hashedkey, $lock, $this->get_identifier() . $hashedkey); + } + } else { + // If we successfully got the parent lock, but are now failing to get this lock, then we should release + // the parent one. + if ($releaseparent) { + $this->get_loader()->release_lock($key); } } return $lock; diff --git a/cache/tests/cache_test.php b/cache/tests/cache_test.php index 0c6cff0412c..bcd99cc1916 100644 --- a/cache/tests/cache_test.php +++ b/cache/tests/cache_test.php @@ -2274,6 +2274,93 @@ class cache_test extends \advanced_testcase { $this->assertEquals(['x' => 'X', 'y' => 'Y', 'z' => 'Z'], $cache->get_many(['x', 'y', 'z'])); } + /** + * Tests that locking fails correctly when either layer of a 2-layer cache has a lock already. + * + * @covers \cache_loader + */ + public function test_application_locking_multiple_layers_failures(): void { + + $instance = cache_config_testing::instance(true); + $instance->phpunit_add_definition('phpunit/test_application_locking', array( + 'mode' => cache_store::MODE_APPLICATION, + 'component' => 'phpunit', + 'area' => 'test_application_locking', + 'staticacceleration' => true, + 'staticaccelerationsize' => 1, + 'requirelockingbeforewrite' => true + ), false); + $instance->phpunit_add_file_store('phpunittest1'); + $instance->phpunit_add_file_store('phpunittest2'); + $instance->phpunit_add_definition_mapping('phpunit/test_application_locking', 'phpunittest1', 1); + $instance->phpunit_add_definition_mapping('phpunit/test_application_locking', 'phpunittest2', 2); + + $cache = cache::make('phpunit', 'test_application_locking'); + + // We need to get the individual stores so as to set up the right behaviour here. + $ref = new \ReflectionClass('\cache'); + $definitionprop = $ref->getProperty('definition'); + $definitionprop->setAccessible(true); + $storeprop = $ref->getProperty('store'); + $storeprop->setAccessible(true); + $loaderprop = $ref->getProperty('loader'); + $loaderprop->setAccessible(true); + + $definition = $definitionprop->getValue($cache); + $localstore = $storeprop->getValue($cache); + $sharedcache = $loaderprop->getValue($cache); + $sharedstore = $storeprop->getValue($sharedcache); + + // Set the lock waiting time to 1 second so it doesn't take forever to run the test. + $ref = new \ReflectionClass('\cachestore_file'); + $lockwaitprop = $ref->getProperty('lockwait'); + $lockwaitprop->setAccessible(true); + + $lockwaitprop->setValue($localstore, 1); + $lockwaitprop->setValue($sharedstore, 1); + + // Get key details and the cache identifier. + $hashedkey = cache_helper::hash_key('apple', $definition); + $localidentifier = $cache->get_identifier(); + $sharedidentifier = $sharedcache->get_identifier(); + + // 1. Local cache is not locked but parent cache is locked. + $sharedstore->acquire_lock($hashedkey, 'somebodyelse'); + try { + $this->assertFalse($cache->acquire_lock('apple')); + + // Neither store is locked by us, shared store still locked. + $this->assertFalse((bool)$localstore->check_lock_state($hashedkey, $localidentifier)); + $this->assertFalse((bool)$sharedstore->check_lock_state($hashedkey, $sharedidentifier)); + $this->assertTrue((bool)$sharedstore->check_lock_state($hashedkey, 'somebodyelse')); + + } finally { + $sharedstore->release_lock($hashedkey, 'somebodyelse'); + } + + // 2. Local cache is locked, parent cache is not locked. + $localstore->acquire_lock($hashedkey, 'somebodyelse'); + try { + $this->assertFalse($cache->acquire_lock('apple')); + + // Neither store is locked by us, local store still locked. + $this->assertFalse((bool)$localstore->check_lock_state($hashedkey, $localidentifier)); + $this->assertFalse((bool)$sharedstore->check_lock_state($hashedkey, $sharedidentifier)); + $this->assertTrue((bool)$localstore->check_lock_state($hashedkey, 'somebodyelse')); + } finally { + $localstore->release_lock($hashedkey, 'somebodyelse'); + } + + // 3. Just for completion, test what happens if we do lock it. + $this->assertTrue($cache->acquire_lock('apple')); + try { + $this->assertTrue((bool)$localstore->check_lock_state($hashedkey, $localidentifier)); + $this->assertTrue((bool)$sharedstore->check_lock_state($hashedkey, $sharedidentifier)); + } finally { + $cache->release_lock('apple'); + } + } + /** * Test the static cache_helper method purge_stores_used_by_definition. */