mirror_php/moodle
1
0
Fork 0
mirror of https://github.com/moodle/moodle.git synced 2025-01-17 21:49:15 +01:00
Code Issues Projects Releases Wiki Activity

MDL-80345 core_lock: deal with hash collisions in postgres_lock_factory

Browse Source
This commit is contained in:
Martin Gauk 2024-06-20 17:24:33 +02:00
parent cd781ab1b7
commit f341c774de
2 changed files with 136 additions and 14 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

63
lib/classes/lock/postgres_lock_factory.php
View File

@ -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();
}
}

87
lib/tests/lock/postgres_lock_factory_test.php Normal file
View File

@ -0,0 +1,87 @@
<?php
// This file is part of Moodle - http://moodle.org/
//
// Moodle is free software: you can redistribute it and/or modify
// it under the terms of the GNU General Public License as published by
// the Free Software Foundation, either version 3 of the License, or
// (at your option) any later version.
//
// Moodle is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU General Public License for more details.
//
// You should have received a copy of the GNU General Public License
// along with Moodle. If not, see <http://www.gnu.org/licenses/>.
namespace core\lock;
/**
* Unit tests for the postgres lock factory.
*
* @covers \core\lock\postgres_lock_factory
* @package core
* @copyright 2024 Martin Gauk <martin.gauk@tu-berlin.de>
* @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();
}
}