Skip to content

Commit a86e8d3

Browse files
sammarshallousammarshallou
committed
MDL-79429 core_cache: Fix unimportant locking issues
This fixes a number of bugs in get_many (which would be important if anyone used it) and tidies up the locking behaviour in regular get calls.
1 parent a15fca4 commit a86e8d3

3 files changed

Lines changed: 280 additions & 23 deletions

File tree

‎public/cache/classes/application_cache.php‎

Lines changed: 34 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -127,20 +127,33 @@ public function __clone() {
127127
/**
128128
* Acquires a lock on the given key.
129129
*
130-
* This is done automatically if the definition requires it.
131-
* It is recommended to use a definition if you want to have locking although it is possible to do locking without having
132-
* it required by the definition.
133-
* The problem with such an approach is that you cannot ensure that code will consistently use locking. You will need to
134-
* rely on the integrators review skills.
130+
* Some cache definition require locking before you set a key - the requirelockingbeforewrite option. It is also
131+
* possible to use the locking mechanism on all caches.
135132
*
136133
* @param string|int $key The key as given to get|set|delete
137134
* @return bool Always returns true
138135
* @throws moodle_exception If the lock cannot be obtained
139136
*/
140137
public function acquire_lock($key) {
138+
$this->acquire_lock_implementation($key);
139+
return true;
140+
}
141+
142+
/**
143+
* Acquires a lock on the given key.
144+
*
145+
* This protected implementation includes the ability to acquire a lock only on this level
146+
* and not on any parent caches, which is used internally and not available in the public
147+
* API.
148+
*
149+
* @param string|int $key The key as given to get|set|delete
150+
* @param bool $lockparent If true (default), also locks parent cache
151+
* @throws moodle_exception If the lock cannot be obtained
152+
*/
153+
protected function acquire_lock_implementation($key, bool $lockparent = true): void {
141154
$releaseparent = false;
142155
try {
143-
if ($this->get_loader() !== false) {
156+
if ($this->get_loader() !== false && $lockparent) {
144157
$this->get_loader()->acquire_lock($key);
145158
// We need to release this lock later if the lock is not successful.
146159
$releaseparent = true;
@@ -167,7 +180,6 @@ public function acquire_lock($key) {
167180
);
168181
}
169182
$releaseparent = false;
170-
return true;
171183
} else {
172184
throw new moodle_exception(
173185
'ex_unabletolock',
@@ -212,6 +224,17 @@ public function check_lock_state($key) {
212224
* @return bool True if the operation succeeded, false otherwise.
213225
*/
214226
public function release_lock($key) {
227+
return $this->release_lock_implementation($key);
228+
}
229+
230+
/**
231+
* Releases the lock this cache has on the given key.
232+
*
233+
* @param string|int $key
234+
* @param bool $releaseparent If true, also releases parent lock
235+
* @return bool True if the operation succeeded, false otherwise.
236+
*/
237+
protected function release_lock_implementation($key, $releaseparent = true): bool {
215238
$loaderkey = $key;
216239
$key = helper::hash_key($key, $this->get_definition());
217240
if ($this->nativelocking) {
@@ -226,8 +249,10 @@ public function release_lock($key) {
226249
\core\lock\timing_wrapper_lock_factory::record_lock_released_data($this->get_identifier() . $key);
227250
}
228251
}
229-
if ($this->get_loader() !== false) {
230-
$this->get_loader()->release_lock($loaderkey);
252+
if ($this->get_loader() !== false && $releaseparent) {
253+
if (!$this->get_loader()->release_lock($loaderkey)) {
254+
$released = false;
255+
}
231256
}
232257
return $released;
233258
}

‎public/cache/classes/cache.php‎

Lines changed: 23 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -614,17 +614,21 @@ protected function get_implementation($key, int $requiredversion, int $strictnes
614614
try {
615615
// Only try to acquire a lock for this cache if we do not already have one.
616616
if (!empty($this->requirelockingbeforewrite) && !$this->check_lock_state($key)) {
617-
$this->acquire_lock($key);
617+
// Lock only in this store because we're going to set only in this store.
618+
// Note: The acquire_lock_implementation method is only available in the cache_application
619+
// subclass, but that is also the only one which sets requirelockingbeforewrite.
620+
$this->acquire_lock_implementation($key, false);
618621
$lock = true;
619622
}
623+
// Set only in this store.
620624
if ($requiredversion === self::VERSION_NONE) {
621625
$this->set_implementation($key, self::VERSION_NONE, $result, false);
622626
} else {
623627
$this->set_implementation($key, $actualversion, $result, false);
624628
}
625629
} finally {
626630
if ($lock) {
627-
$this->release_lock($key);
631+
$this->release_lock_implementation($key, false);
628632
}
629633
}
630634
}
@@ -734,18 +738,23 @@ public function get_many(array $keys, $strictness = IGNORE_MISSING) {
734738
}
735739
foreach ($resultmissing as $key => $value) {
736740
$result[$keysparsed[$key]] = $value;
737-
$lock = false;
738-
try {
739-
if (!empty($this->requirelockingbeforewrite)) {
740-
$this->acquire_lock($key);
741-
$lock = true;
742-
}
743-
if ($value !== false) {
744-
$this->set($key, $value);
745-
}
746-
} finally {
747-
if ($lock) {
748-
$this->release_lock($key);
741+
if ($value !== false) {
742+
// Set it to the store if we got it from the loader/datasource. Only set to
743+
// this direct store; parent method will set it to other stores if needed.
744+
$lock = false;
745+
try {
746+
// Only try to acquire a lock for this cache if we do not already have one.
747+
if (!empty($this->requirelockingbeforewrite) && !$this->check_lock_state($key)) {
748+
// Lock only in this store because we're going to set only in this store.
749+
$this->acquire_lock_implementation($key, false);
750+
$lock = true;
751+
}
752+
// Set only in this store.
753+
$this->set_implementation($key, self::VERSION_NONE, $value, false);
754+
} finally {
755+
if ($lock) {
756+
$this->release_lock_implementation($key, false);
757+
}
749758
}
750759
}
751760
}

‎public/cache/tests/cache_test.php‎

Lines changed: 223 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -908,6 +908,229 @@ public function test_application_manual_locking(): void {
908908
$this->assertTrue($cache2->set('testkey', 'test data'));
909909
}
910910

911+
/**
912+
* Tests that when you get an item from a 2-layer cache, where it is present in the shared but
913+
* not local cache, the system correctly locks the cache before writing it to local cache if
914+
* required.
915+
*/
916+
public function test_automatic_locking_multiple_layers_get(): void {
917+
$instance = cache_config_testing::instance(true);
918+
$instance->phpunit_add_definition('phpunit/test_application_locking', [
919+
'mode' => store::MODE_APPLICATION,
920+
'component' => 'phpunit',
921+
'area' => 'test_application_locking',
922+
'staticacceleration' => true,
923+
'staticaccelerationsize' => 1,
924+
'requirelockingbeforewrite' => true,
925+
], false);
926+
$instance->phpunit_add_file_store('phpunittest1');
927+
$instance->phpunit_add_file_store('phpunittest2');
928+
$instance->phpunit_add_definition_mapping('phpunit/test_application_locking', 'phpunittest1', 1);
929+
$instance->phpunit_add_definition_mapping('phpunit/test_application_locking', 'phpunittest2', 2);
930+
931+
$cache = cache::make('phpunit', 'test_application_locking');
932+
933+
// We need to get the individual stores so as to set up the right behaviour here.
934+
$ref = new \ReflectionClass('\cache');
935+
$definitionprop = $ref->getProperty('definition');
936+
$storeprop = $ref->getProperty('store');
937+
$loaderprop = $ref->getProperty('loader');
938+
939+
$definition = $definitionprop->getValue($cache);
940+
$localstore = $storeprop->getValue($cache);
941+
$sharedcache = $loaderprop->getValue($cache);
942+
$sharedstore = $storeprop->getValue($sharedcache);
943+
944+
// Set the lock waiting time to 1 second so it doesn't take forever to run the 'fail' test.
945+
$ref = new \ReflectionClass('\cachestore_file');
946+
$lockwaitprop = $ref->getProperty('lockwait');
947+
$lockwaitprop->setValue($localstore, 1);
948+
949+
// The shared store contains the value, but local store does not.
950+
$key = 'frog';
951+
$hashedkey = helper::hash_key($key, $definition);
952+
$sharedstore->set($hashedkey, 'kermit');
953+
954+
// A 'get' returns value from shared store...
955+
$this->assertEquals('kermit', $cache->get($key));
956+
957+
// ...and value is set to local store now.
958+
$this->assertEquals('kermit', $localstore->get($hashedkey));
959+
960+
// Continue testing with a different key.
961+
$key = 'toad';
962+
$hashedkey = helper::hash_key($key, $definition);
963+
$sharedstore->set($hashedkey, 'mr');
964+
965+
// The same scenario also works if we already hold a lock on the value.
966+
$cache->acquire_lock($key);
967+
try {
968+
$this->assertEquals('mr', $cache->get($key));
969+
} finally {
970+
$cache->release_lock($key);
971+
}
972+
$this->assertEquals('mr', $localstore->get($hashedkey));
973+
974+
// Continue testing with a different key.
975+
$key = 'squirrel';
976+
$hashedkey = helper::hash_key($key, $definition);
977+
$sharedstore->set($hashedkey, 'nutkin');
978+
979+
// If somebody else holds a lock on the value, on the shared not local store, this is OK.
980+
$sharedstore->acquire_lock($hashedkey, 'somebodyelse');
981+
try {
982+
$this->assertEquals('nutkin', $cache->get($key));
983+
} finally {
984+
$sharedstore->release_lock($hashedkey, 'somebodyelse');
985+
}
986+
$this->assertEquals('nutkin', $localstore->get($hashedkey));
987+
988+
// Continue testing with a different key.
989+
$key = 'rabbit';
990+
$hashedkey = helper::hash_key($key, $definition);
991+
$sharedstore->set($hashedkey, 'peter');
992+
993+
// If somebody else holds a lock on the value, on the local store, it should fail.
994+
$localstore->acquire_lock($hashedkey, 'somebodyelse');
995+
try {
996+
$cache->get($key);
997+
$this->fail();
998+
} catch (\moodle_exception $e) {
999+
$this->assertStringContainsString('Unable to acquire a lock', $e->getMessage());
1000+
} finally {
1001+
$localstore->release_lock($hashedkey, 'somebodyelse');
1002+
}
1003+
$this->assertEquals(false, $localstore->get($hashedkey));
1004+
}
1005+
1006+
/**
1007+
* Tests that when you get_many items from a 2-layer cache, where it is present in the shared but
1008+
* not local cache, the system correctly locks the cache before writing it to local cache if
1009+
* required.
1010+
*/
1011+
public function test_automatic_locking_multiple_layers_get_many(): void {
1012+
$instance = cache_config_testing::instance(true);
1013+
$instance->phpunit_add_definition('phpunit/test_application_locking', [
1014+
'mode' => store::MODE_APPLICATION,
1015+
'component' => 'phpunit',
1016+
'area' => 'test_application_locking',
1017+
'staticacceleration' => true,
1018+
'staticaccelerationsize' => 1,
1019+
'requirelockingbeforewrite' => true,
1020+
], false);
1021+
$instance->phpunit_add_file_store('phpunittest1');
1022+
$instance->phpunit_add_file_store('phpunittest2');
1023+
$instance->phpunit_add_definition_mapping('phpunit/test_application_locking', 'phpunittest1', 1);
1024+
$instance->phpunit_add_definition_mapping('phpunit/test_application_locking', 'phpunittest2', 2);
1025+
1026+
$cache = cache::make('phpunit', 'test_application_locking');
1027+
1028+
// We need to get the individual stores so as to set up the right behaviour here.
1029+
$ref = new \ReflectionClass('\cache');
1030+
$definitionprop = $ref->getProperty('definition');
1031+
$storeprop = $ref->getProperty('store');
1032+
$loaderprop = $ref->getProperty('loader');
1033+
1034+
$definition = $definitionprop->getValue($cache);
1035+
$localstore = $storeprop->getValue($cache);
1036+
$sharedcache = $loaderprop->getValue($cache);
1037+
$sharedstore = $storeprop->getValue($sharedcache);
1038+
1039+
// Set the lock waiting time to 1 second so it doesn't take forever to run the 'fail' test.
1040+
$ref = new \ReflectionClass('\cachestore_file');
1041+
$lockwaitprop = $ref->getProperty('lockwait');
1042+
$lockwaitprop->setValue($localstore, 1);
1043+
1044+
// Initialises a set of keys for testing.
1045+
// Param: int $index Index used as a suffix on key names.
1046+
// Returns: Array of hashed keys.
1047+
$initkeys = function(int $index) use($definition, $sharedstore): array {
1048+
$keys = ['a' . $index, 'b' . $index, 'c' . $index];
1049+
$hashedkeys = [];
1050+
foreach ($keys as $key) {
1051+
$hashedkeys[$key] = helper::hash_key($key, $definition);
1052+
}
1053+
$sharedstore->set($hashedkeys['a' . $index], 'a');
1054+
$sharedstore->set($hashedkeys['c' . $index], 'c');
1055+
return $hashedkeys;
1056+
};
1057+
$hashedkeys = $initkeys(1);
1058+
1059+
// A 'get_many' returns value from shared store...
1060+
$this->assertEquals(['a1' => 'a', 'b1' => false, 'c1' => 'c'],
1061+
$cache->get_many(['a1', 'b1', 'c1']));
1062+
1063+
// ...and values are set to local store now.
1064+
$this->assertEquals('a', $localstore->get($hashedkeys['a1']));
1065+
$this->assertEquals('c', $localstore->get($hashedkeys['c1']));
1066+
1067+
// Continue testing with different keys.
1068+
$hashedkeys = $initkeys(2);
1069+
1070+
// The same scenario also works if we already hold a lock on all values.
1071+
$cache->acquire_lock('a2');
1072+
$cache->acquire_lock('b2');
1073+
$cache->acquire_lock('c2');
1074+
try {
1075+
$this->assertEquals(['a2' => 'a', 'b2' => false, 'c2' => 'c'],
1076+
$cache->get_many(['a2', 'b2', 'c2']));
1077+
} finally {
1078+
$cache->release_lock('c2');
1079+
$cache->release_lock('b2');
1080+
$cache->release_lock('a2');
1081+
}
1082+
$this->assertEquals('a', $localstore->get($hashedkeys['a2']));
1083+
$this->assertEquals('c', $localstore->get($hashedkeys['c2']));
1084+
1085+
// Continue testing with different keys.
1086+
$hashedkeys = $initkeys(3);
1087+
1088+
// If somebody else holds a lock on the values, on the shared not local store, this is OK.
1089+
$sharedstore->acquire_lock($hashedkeys['a3'], 'somebodyelse');
1090+
$sharedstore->acquire_lock($hashedkeys['b3'], 'somebodyelse');
1091+
$sharedstore->acquire_lock($hashedkeys['c3'], 'somebodyelse');
1092+
try {
1093+
$this->assertEquals(['a3' => 'a', 'b3' => false, 'c3' => 'c'],
1094+
$cache->get_many(['a3', 'b3', 'c3']));
1095+
} finally {
1096+
$sharedstore->release_lock($hashedkeys['c3'], 'somebodyelse');
1097+
$sharedstore->release_lock($hashedkeys['b3'], 'somebodyelse');
1098+
$sharedstore->release_lock($hashedkeys['a3'], 'somebodyelse');
1099+
}
1100+
$this->assertEquals('a', $localstore->get($hashedkeys['a3']));
1101+
$this->assertEquals('c', $localstore->get($hashedkeys['c3']));
1102+
1103+
// Continue testing with different keys.
1104+
$hashedkeys = $initkeys(4);
1105+
1106+
// If somebody else holds a lock on the C value, on the local store, it will fail.
1107+
$localstore->acquire_lock($hashedkeys['c4'], 'somebodyelse');
1108+
try {
1109+
$cache->get_many(['a4', 'b4', 'c4']);
1110+
$this->fail();
1111+
} catch (\moodle_exception $e) {
1112+
$this->assertStringContainsString('Unable to acquire a lock', $e->getMessage());
1113+
} finally {
1114+
$localstore->release_lock($hashedkeys['c4'], 'somebodyelse');
1115+
}
1116+
$this->assertEquals('a', $localstore->get($hashedkeys['a4']));
1117+
$this->assertEquals(false, $localstore->get($hashedkeys['c4']));
1118+
1119+
// Continue testing with different keys.
1120+
$hashedkeys = $initkeys(5);
1121+
1122+
// If somebody else holds a lock on the B value, on the local store, it doesn't care.
1123+
$localstore->acquire_lock($hashedkeys['b5'], 'somebodyelse');
1124+
try {
1125+
$this->assertEquals(['a5' => 'a', 'b5' => false, 'c5' => 'c'],
1126+
$cache->get_many(['a5', 'b5', 'c5']));
1127+
} finally {
1128+
$localstore->release_lock($hashedkeys['b5'], 'somebodyelse');
1129+
}
1130+
$this->assertEquals('a', $localstore->get($hashedkeys['a5']));
1131+
$this->assertEquals('c', $localstore->get($hashedkeys['c5']));
1132+
}
1133+
9111134
/**
9121135
* Tests application cache event invalidation
9131136
*/

0 commit comments

Comments
 (0)