MDL-66935 core_lock: Fix resource key clashes in db and postgres locks

lib/classes/lock/db_record_lock_factory.php

@@ -120,15 +120,17 @@ class db_record_lock_factory implements lock_factory {
         $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 @@ class db_record_lock_factory implements lock_factory {
             $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) {

lib/classes/lock/lock_config.php

@@ -38,18 +38,17 @@ defined('MOODLE_INTERNAL') || die();
 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 string class name
-     * @return \core\lock\lock_factory
      * @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 @@ class lock_config {
                 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 @@ class lock_config {
             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;
     }
 

lib/classes/lock/postgres_lock_factory.php

@@ -178,7 +178,7 @@ class postgres_lock_factory implements lock_factory {
     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);

lib/tests/lock_config_test.php

@@ -46,31 +46,57 @@ class lock_config_testcase extends advanced_testcase {
         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');
 
+        // 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);
+        }
     }
 }
 

lib/tests/lock_test.php

@@ -44,11 +44,26 @@ class lock_testcase extends advanced_testcase {
     }
 
     /**
-     * Run a suite of tests on a lock factory.
+     * Run a suite of tests on a lock factory class.
-     * @param \core\lock\lock_factory $lockfactory - A lock factory to test
+     *
+     * @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 @@ class lock_testcase extends advanced_testcase {
     }
 
     /**
-     * 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(\core\lock\lock_config::get_lock_factory_class());
-        $this->run_on_lock_factory($defaultfactory);
 
         // Manually create the core no-configuration factories.
-        $dblockfactory = new \core\lock\db_record_lock_factory('test');
+        $this->run_on_lock_factory(\core\lock\db_record_lock_factory::class);
-        $this->run_on_lock_factory($dblockfactory);
+        $this->run_on_lock_factory(\core\lock\file_lock_factory::class);
-
-        $filelockfactory = new \core\lock\file_lock_factory('test');
-        $this->run_on_lock_factory($filelockfactory);
 
     }