From f341c774deb00ae0c8d7b3ba192cddb1cefd01e6 Mon Sep 17 00:00:00 2001 From: Martin Gauk Date: Thu, 20 Jun 2024 17:24:33 +0200 Subject: [PATCH] MDL-80345 core_lock: deal with hash collisions in postgres_lock_factory --- lib/classes/lock/postgres_lock_factory.php | 63 +++++++++++--- lib/tests/lock/postgres_lock_factory_test.php | 87 +++++++++++++++++++ 2 files changed, 136 insertions(+), 14 deletions(-) create mode 100644 lib/tests/lock/postgres_lock_factory_test.php diff --git a/lib/classes/lock/postgres_lock_factory.php b/lib/classes/lock/postgres_lock_factory.php index 822d8c6039c..684a321a367 100644 --- a/lib/classes/lock/postgres_lock_factory.php +++ b/lib/classes/lock/postgres_lock_factory.php @@ -25,7 +25,9 @@ use coding_exception; * 2 different forms of lock functions, some accepting a single int, and some accepting 2 ints. This implementation * uses the 2 int version so that it uses a separate namespace from the session locking. The second note, * is because postgres uses integer keys for locks, we first need to map strings to a unique integer. This is done - * using a prefix of a sha1 hash converted to an integer. + * using a prefix of a sha1 hash converted to an integer. There is a realistic chance of collisions by using this + * prefix when locking multiple resources at the same time (multiple resource identifiers leading to the + * same token/prefix). We need to deal with that. * * @package core * @category lock @@ -46,8 +48,11 @@ class postgres_lock_factory implements lock_factory { /** @var string $type Used to prefix lock keys */ protected $type; - /** @var array $openlocks - List of held locks - used by auto-release */ - protected $openlocks = array(); + /** @var int[] $resourcetokens Mapping of held locks (resource identifier => internal token) */ + protected $resourcetokens = []; + + /** @var int[] $locksbytoken Mapping of held locks (db connection => internal token => number of locks held) */ + static protected $locksbytoken = []; /** * Calculate a unique instance id based on the database name and prefix. @@ -139,17 +144,26 @@ class postgres_lock_factory implements lock_factory { * @param string $resource - The identifier for the lock. Should use frankenstyle prefix. * @param int $timeout - The number of seconds to wait for a lock before giving up. * @param int $maxlifetime - Unused by this lock type. - * @return boolean - true if a lock was obtained. + * @return \core\lock\lock|boolean - An instance of \core\lock\lock if the lock was obtained, or false. */ public function get_lock($resource, $timeout, $maxlifetime = 86400) { + $dbid = spl_object_id($this->db); $giveuptime = time() + $timeout; + $resourcekey = $this->type . '_' . $resource; + $token = $this->get_index_from_key($resourcekey); - $token = $this->get_index_from_key($this->type . '_' . $resource); - - if (isset($this->openlocks[$token])) { + if (isset($this->resourcetokens[$resourcekey])) { return false; } + if (isset(self::$locksbytoken[$dbid][$token])) { + // There is a hash collision, another resource identifier leads to the same token. + // As we already hold an advisory lock for this token, we just increase the counter. + self::$locksbytoken[$dbid][$token]++; + $this->resourcetokens[$resourcekey] = $token; + return new lock($resourcekey, $this); + } + $params = [ 'locktype' => $this->dblockid, 'token' => $token @@ -167,8 +181,9 @@ class postgres_lock_factory implements lock_factory { } while (!$locked && time() < $giveuptime); if ($locked) { - $this->openlocks[$token] = 1; - return new lock($token, $this); + self::$locksbytoken[$dbid][$token] = 1; + $this->resourcetokens[$resourcekey] = $token; + return new lock($resourcekey, $this); } return false; } @@ -179,12 +194,32 @@ class postgres_lock_factory implements lock_factory { * @return boolean - true if the lock is no longer held (including if it was never held). */ public function release_lock(lock $lock) { - $params = array('locktype' => $this->dblockid, - 'token' => $lock->get_key()); + $dbid = spl_object_id($this->db); + $resourcekey = $lock->get_key(); + + if (isset($this->resourcetokens[$resourcekey])) { + $token = $this->resourcetokens[$resourcekey]; + } else { + return true; + } + + if (self::$locksbytoken[$dbid][$token] > 1) { + // There are multiple resource identifiers that lead to the same token. + // We will unlock the token later when the last resource is released. + unset($this->resourcetokens[$resourcekey]); + self::$locksbytoken[$dbid][$token]--; + return true; + } + + $params = [ + 'locktype' => $this->dblockid, + 'token' => $token, + ]; $result = $this->db->get_record_sql('SELECT pg_advisory_unlock(:locktype, :token) AS unlocked', $params); $result = $result->unlocked === 't'; if ($result) { - unset($this->openlocks[$lock->get_key()]); + unset($this->resourcetokens[$resourcekey]); + unset(self::$locksbytoken[$dbid][$token]); } return $result; } @@ -202,8 +237,8 @@ class postgres_lock_factory implements lock_factory { */ public function auto_release() { // Called from the shutdown handler. Must release all open locks. - foreach ($this->openlocks as $key => $unused) { - $lock = new lock($key, $this); + foreach ($this->resourcetokens as $resourcekey => $unused) { + $lock = new lock($resourcekey, $this); $lock->release(); } } diff --git a/lib/tests/lock/postgres_lock_factory_test.php b/lib/tests/lock/postgres_lock_factory_test.php new file mode 100644 index 00000000000..09bb2a3f54b --- /dev/null +++ b/lib/tests/lock/postgres_lock_factory_test.php @@ -0,0 +1,87 @@ +. + +namespace core\lock; + +/** + * Unit tests for the postgres lock factory. + * + * @covers \core\lock\postgres_lock_factory + * @package core + * @copyright 2024 Martin Gauk + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +final class postgres_lock_factory_test extends \advanced_testcase { + /** + * Set up. + */ + public function setUp(): void { + global $DB; + parent::setUp(); + // Skip tests if not using Postgres. + if (!($DB instanceof \pgsql_native_moodle_database)) { + $this->markTestSkipped('Postgres-only test'); + } + } + + /** + * Test locking resources that lead to the same token computed by postgres_lock_factory::get_index_from_key. + * + * It is known that get_index_from_key computes the same token for the strings 'cron_core_cron' and + * 'cron_adhoc_82795762'. + */ + public function test_resources_with_same_hash(): void { + $cronlockfactory = new postgres_lock_factory('cron'); + $lock1 = $cronlockfactory->get_lock('core_cron', 0); + $this->assertNotFalse($lock1); + $lock2 = $cronlockfactory->get_lock('adhoc_82795762', 0); + $this->assertNotFalse($lock2); + + $lock2->release(); + $lock1->release(); + } + + /** + * Test auto_release() method. + * + * Check that all locks were released after calling auto_release(). + */ + public function test_auto_release(): void { + $cronlockfactory = new postgres_lock_factory('test'); + $lock1 = $cronlockfactory->get_lock('1', 0); + $this->assertNotFalse($lock1); + $lock2 = $cronlockfactory->get_lock('2', 0); + $this->assertNotFalse($lock2); + + // Trying to get the lock again should fail as the lock is already held. + $this->assertFalse($cronlockfactory->get_lock('1', 0)); + + $cronlockfactory->auto_release(); + + // We can now lock the resources again. + $lock1again = $cronlockfactory->get_lock('1', 0); + $this->assertNotFalse($lock1again); + $lock2again = $cronlockfactory->get_lock('2', 0); + $this->assertNotFalse($lock2again); + + $lock1again->release(); + $lock2again->release(); + + // Need to explicitly release the locks (although they were already released) to avoid the debug message. + $lock1->release(); + $lock2->release(); + } +}