diff --git a/lib/classes/lock/db_record_lock_factory.php b/lib/classes/lock/db_record_lock_factory.php index 83321d23d6da1..e430bc56294c2 100644 --- a/lib/classes/lock/db_record_lock_factory.php +++ b/lib/classes/lock/db_record_lock_factory.php @@ -120,15 +120,17 @@ public function get_lock($resource, $timeout, $maxlifetime = 86400) { $giveuptime = $now + $timeout; $expires = $now + $maxlifetime; - if (!$this->db->record_exists('lock_db', array('resourcekey' => $resource))) { + $resourcekey = $this->type . '_' . $resource; + + if (!$this->db->record_exists('lock_db', array('resourcekey' => $resourcekey))) { $record = new \stdClass(); - $record->resourcekey = $resource; + $record->resourcekey = $resourcekey; $result = $this->db->insert_record('lock_db', $record); } $params = array('expires' => $expires, 'token' => $token, - 'resourcekey' => $resource, + 'resourcekey' => $resourcekey, 'now' => $now); $sql = 'UPDATE {lock_db} SET @@ -143,7 +145,7 @@ public function get_lock($resource, $timeout, $maxlifetime = 86400) { $params['now'] = $now; $this->db->execute($sql, $params); - $countparams = array('owner' => $token, 'resourcekey' => $resource); + $countparams = array('owner' => $token, 'resourcekey' => $resourcekey); $result = $this->db->count_records('lock_db', $countparams); $locked = $result === 1; if (!$locked && $timeout > 0) { diff --git a/lib/classes/lock/lock_config.php b/lib/classes/lock/lock_config.php index 06fddf4038670..31f3082e46600 100644 --- a/lib/classes/lock/lock_config.php +++ b/lib/classes/lock/lock_config.php @@ -38,18 +38,17 @@ class lock_config { /** - * Get an instance of the currently configured locking subclass. + * Get the currently configured locking subclass. * - * @param string $type - Unique namespace for the locks generated by this factory. e.g. core_cron - * @return \core\lock\lock_factory + * @return string class name * @throws \coding_exception */ - public static function get_lock_factory($type) { + public static function get_lock_factory_class(): string { + global $CFG, $DB; - $lockfactory = null; if (during_initial_install()) { - $lockfactory = new \core\lock\installation_lock_factory($type); + $lockfactoryclass = '\core\lock\installation_lock_factory'; } else if (isset($CFG->lock_factory) && $CFG->lock_factory != 'auto') { if (!class_exists($CFG->lock_factory)) { // In this case I guess it is not safe to continue. Different cluster nodes could end up using different locking @@ -57,7 +56,6 @@ public static function get_lock_factory($type) { throw new \coding_exception('Lock factory set in $CFG does not exist: ' . $CFG->lock_factory); } $lockfactoryclass = $CFG->lock_factory; - $lockfactory = new $lockfactoryclass($type); } else { $dbtype = clean_param($DB->get_dbfamily(), PARAM_ALPHA); @@ -66,14 +64,31 @@ public static function get_lock_factory($type) { if (!class_exists($lockfactoryclass)) { $lockfactoryclass = '\core\lock\file_lock_factory'; } - /* @var lock_factory $lockfactory */ - $lockfactory = new $lockfactoryclass($type); + + // Test if the auto chosen lock factory is available. + $lockfactory = new $lockfactoryclass('test'); if (!$lockfactory->is_available()) { // Final fallback - DB row locking. - $lockfactory = new \core\lock\db_record_lock_factory($type); + $lockfactoryclass = '\core\lock\db_record_lock_factory'; } } + return $lockfactoryclass; + } + + /** + * Get an instance of the currently configured locking subclass. + * + * @param string $type - Unique namespace for the locks generated by this factory. e.g. core_cron + * @return \core\lock\lock_factory + * @throws \coding_exception + */ + public static function get_lock_factory(string $type): \core\lock\lock_factory { + $lockfactoryclass = self::get_lock_factory_class(); + $lockfactory = new $lockfactoryclass($type); + if (!$lockfactory->is_available()) { + throw new \coding_exception("Lock factory class $lockfactoryclass is not available."); + } return $lockfactory; } diff --git a/lib/classes/lock/postgres_lock_factory.php b/lib/classes/lock/postgres_lock_factory.php index 87ae64b9e8c77..de1be0815bf39 100644 --- a/lib/classes/lock/postgres_lock_factory.php +++ b/lib/classes/lock/postgres_lock_factory.php @@ -178,7 +178,7 @@ protected function get_index_from_key($key) { public function get_lock($resource, $timeout, $maxlifetime = 86400) { $giveuptime = time() + $timeout; - $token = $this->get_index_from_key($resource); + $token = $this->get_index_from_key($this->type . '_' . $resource); $params = array('locktype' => $this->dblockid, 'token' => $token); diff --git a/lib/tests/lock_config_test.php b/lib/tests/lock_config_test.php index 63aef074d70c4..611eca57cdd00 100644 --- a/lib/tests/lock_config_test.php +++ b/lib/tests/lock_config_test.php @@ -46,31 +46,57 @@ public function test_lock_config() { if (isset($CFG->lock_factory)) { $original = $CFG->lock_factory; } + $originalfilelocking = null; + if (isset($CFG->preventfilelocking)) { + $originalfilelocking = $CFG->preventfilelocking; + } // Test no configuration. unset($CFG->lock_factory); - + $CFG->preventfilelocking = 0; $factory = \core\lock\lock_config::get_lock_factory('cache'); - $this->assertNotEmpty($factory, 'Get a default factory with no configuration'); - $CFG->lock_factory = '\core\lock\file_lock_factory'; + // Test explicit broken lock. + $CFG->lock_factory = '\core\lock\not_a_lock_factory'; + try { + $factory = \core\lock\lock_config::get_lock_factory('cache'); + $this->fail('Exception expected'); + } catch (moodle_exception $ex) { + $this->assertInstanceOf('coding_exception', $ex); + } + // Test explicit file locks. + $CFG->lock_factory = '\core\lock\file_lock_factory'; $factory = \core\lock\lock_config::get_lock_factory('cache'); $this->assertTrue($factory instanceof \core\lock\file_lock_factory, - 'Get a default factory with a set configuration'); + 'Get an explicit file lock factory'); - $CFG->lock_factory = '\core\lock\db_record_lock_factory'; + // Test explicit file locks but with file locks prevented. + $CFG->preventfilelocking = 1; + try { + $factory = \core\lock\lock_config::get_lock_factory('cache'); + $this->fail('Exception expected'); + } catch (moodle_exception $ex) { + $this->assertInstanceOf('coding_exception', $ex); + } + // Test explicit db locks. + $CFG->lock_factory = '\core\lock\db_record_lock_factory'; $factory = \core\lock\lock_config::get_lock_factory('cache'); $this->assertTrue($factory instanceof \core\lock\db_record_lock_factory, - 'Get a default factory with a changed configuration'); + 'Get an explicit db record lock factory'); if ($original) { $CFG->lock_factory = $original; } else { unset($CFG->lock_factory); } + if ($originalfilelocking) { + $CFG->preventfilelocking = $originalfilelocking; + } else { + unset($CFG->preventfilelocking); + } } } diff --git a/lib/tests/lock_test.php b/lib/tests/lock_test.php index 695d3a174e8c8..1d2ae40653d93 100644 --- a/lib/tests/lock_test.php +++ b/lib/tests/lock_test.php @@ -44,11 +44,26 @@ protected function setUp() { } /** - * Run a suite of tests on a lock factory. - * @param \core\lock\lock_factory $lockfactory - A lock factory to test + * Run a suite of tests on a lock factory class. + * + * @param class $lockfactoryclass - A lock factory class to test */ - protected function run_on_lock_factory(\core\lock\lock_factory $lockfactory) { + protected function run_on_lock_factory($lockfactoryclass) { + $modassignfactory = new $lockfactoryclass('mod_assign'); + $tooltaskfactory = new $lockfactoryclass('tool_task'); + + // Test for lock clashes between lock stores. + $assignlock = $modassignfactory->get_lock('abc', 0); + $this->assertNotEmpty($assignlock, 'Get a lock "abc" from store "mod_assign"'); + + $tasklock = $tooltaskfactory->get_lock('abc', 0); + $this->assertNotEmpty($tasklock, 'Get a lock "abc" from store "tool_task"'); + + $assignlock->release(); + $tasklock->release(); + + $lockfactory = new $lockfactoryclass('default'); if ($lockfactory->is_available()) { // This should work. $lock1 = $lockfactory->get_lock('abc', 2); @@ -111,20 +126,16 @@ protected function run_on_lock_factory(\core\lock\lock_factory $lockfactory) { } /** - * Tests the testable lock factories. + * Tests the testable lock factories classes. * @return void */ public function test_locks() { // Run the suite on the current configured default (may be non-core). - $defaultfactory = \core\lock\lock_config::get_lock_factory('default'); - $this->run_on_lock_factory($defaultfactory); + $this->run_on_lock_factory(\core\lock\lock_config::get_lock_factory_class()); // Manually create the core no-configuration factories. - $dblockfactory = new \core\lock\db_record_lock_factory('test'); - $this->run_on_lock_factory($dblockfactory); - - $filelockfactory = new \core\lock\file_lock_factory('test'); - $this->run_on_lock_factory($filelockfactory); + $this->run_on_lock_factory(\core\lock\db_record_lock_factory::class); + $this->run_on_lock_factory(\core\lock\file_lock_factory::class); }