From 67c7ed25e3a7decb816824f15e60d66956759c00 Mon Sep 17 00:00:00 2001 From: Mark Johnson Date: Tue, 25 Oct 2022 13:55:09 +0100 Subject: [PATCH] MDL-76076 cache: Add locking before write for parent caches --- cache/classes/loaders.php | 29 ++++++++++++-- cache/tests/cache_test.php | 80 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 105 insertions(+), 4 deletions(-) diff --git a/cache/classes/loaders.php b/cache/classes/loaders.php index d60122be400..58374aba77b 100644 --- a/cache/classes/loaders.php +++ b/cache/classes/loaders.php @@ -607,11 +607,18 @@ class cache implements cache_loader { // 6. Set it to the store if we got it from the loader/datasource. Only set to this direct // store; parent method will have set it to all stores if needed. if ($setaftervalidation) { + $lock = null; + if (!empty($this->requirelockingbeforewrite)) { + $lock = $this->acquire_lock($key); + } if ($requiredversion === self::VERSION_NONE) { $this->set_implementation($key, self::VERSION_NONE, $result, false); } else { $this->set_implementation($key, $actualversion, $result, false); } + if ($lock) { + $this->release_lock($key); + } } // 7. Make sure we don't pass back anything that could be a reference. // We don't want people modifying the data in the cache. @@ -719,9 +726,16 @@ class cache implements cache_loader { } foreach ($resultmissing as $key => $value) { $result[$keysparsed[$key]] = $value; + $lock = null; + if (!empty($this->requirelockingbeforewrite)) { + $lock = $this->acquire_lock($key); + } if ($value !== false) { $this->set($key, $value); } + if ($lock) { + $this->release_lock($key); + } } unset($resultmissing); } @@ -1662,6 +1676,9 @@ class cache_application extends cache implements cache_loader_with_locking { */ public function acquire_lock($key) { global $CFG; + if ($this->get_loader() !== false) { + $this->get_loader()->acquire_lock($key); + } $key = $this->parse_key($key); $before = microtime(true); if ($this->nativelocking) { @@ -1708,6 +1725,7 @@ class cache_application extends cache implements cache_loader_with_locking { * @return bool True if the operation succeeded, false otherwise. */ public function release_lock($key) { + $loaderkey = $key; $key = $this->parse_key($key); if ($this->nativelocking) { $released = $this->get_store()->release_lock($key, $this->get_identifier()); @@ -1721,6 +1739,9 @@ class cache_application extends cache implements cache_loader_with_locking { \core\lock\timing_wrapper_lock_factory::record_lock_released_data($this->get_identifier() . $key); } } + if ($this->get_loader() !== false) { + $this->get_loader()->release_lock($loaderkey); + } return $released; } @@ -1792,11 +1813,11 @@ class cache_application extends cache implements cache_loader_with_locking { */ public function set_many(array $keyvaluearray) { if ($this->requirelockingbeforewrite) { - foreach ($keyvaluearray as $id => $pair) { - if (!$this->check_lock_state($pair['key'])) { - debugging('Attempted to set cache key "' . $pair['key'] . '" without a lock. ' + foreach ($keyvaluearray as $key => $value) { + if (!$this->check_lock_state($key)) { + debugging('Attempted to set cache key "' . $key . '" without a lock. ' . 'Locking before writes is required for ' . $this->get_definition()->get_name(), DEBUG_DEVELOPER); - unset($keyvaluearray[$id]); + unset($keyvaluearray[$key]); } } } diff --git a/cache/tests/cache_test.php b/cache/tests/cache_test.php index f5b874236b7..05242ef4f54 100644 --- a/cache/tests/cache_test.php +++ b/cache/tests/cache_test.php @@ -2160,6 +2160,86 @@ class cache_test extends \advanced_testcase { cache::make('phpunit', 'test_application_locking'); } + /** + * Test that locking before write works when writing across multiple layers. + * + * @covers \cache_loader + * @return void + */ + public function test_application_locking_multiple_layers() { + + $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'); + $this->assertInstanceOf(cache_application::class, $cache); + + // Check that we can set a key across multiple layers. + $cache->acquire_lock('a'); + $this->assertTrue($cache->set('a', 'A')); + $cache->release_lock('a'); + + // Delete from the current layer. + $cache->delete('a', false); + + // Check that we can get the value from the deeper layer, which will also re-set it in the current one. + $this->assertEquals('A', $cache->get('a')); + + // Try set/delete/get_many. + $cache->acquire_lock('x'); + $cache->acquire_lock('y'); + $cache->acquire_lock('z'); + $this->assertEquals(3, $cache->set_many(['x' => 'X', 'y' => 'Y', 'z' => 'Z'])); + $cache->release_lock('x'); + $cache->release_lock('y'); + $cache->release_lock('z'); + + $cache->delete_many(['x', 'y', 'z'], false); + $this->assertEquals(['x' => 'X', 'y' => 'Y', 'z' => 'Z'], $cache->get_many(['x', 'y', 'z'])); + + $cache->purge(); + + // Try the tests again with a third layer. + $instance->phpunit_add_file_store('phpunittest3'); + $instance->phpunit_add_definition_mapping('phpunit/test_application_locking', 'phpunittest3', 3); + $cache = cache::make('phpunit', 'test_application_locking'); + $this->assertInstanceOf(cache_application::class, $cache); + + // Check that we can set a key across multiple layers. + $cache->acquire_lock('a'); + $this->assertTrue($cache->set('a', 'A')); + $cache->release_lock('a'); + + // Delete from the current layer. + $cache->delete('a', false); + + // Check that we can get the value from the deeper layer, which will also re-set it in the current one. + $this->assertEquals('A', $cache->get('a')); + + // Try set/delete/get_many. + $cache->acquire_lock('x'); + $cache->acquire_lock('y'); + $cache->acquire_lock('z'); + $this->assertEquals(3, $cache->set_many(['x' => 'X', 'y' => 'Y', 'z' => 'Z'])); + $cache->release_lock('x'); + $cache->release_lock('y'); + $cache->release_lock('z'); + + $cache->delete_many(['x', 'y', 'z'], false); + $this->assertEquals(['x' => 'X', 'y' => 'Y', 'z' => 'Z'], $cache->get_many(['x', 'y', 'z'])); + } + /** * Test the static cache_helper method purge_stores_used_by_definition. */