Skip to content

Commit

Permalink
Merge branch 'MDL-66935-lock-clashes-MOODLE_36_STABLE' of https://git…
Browse files Browse the repository at this point in the history 
…hub.com/brendanheywood/moodle into MOODLE_36_STABLE
  • Loading branch information
@stronk7
stronk7 committed Oct 24, 2019
2 parents 5d5d2e6 + 1aabe01 commit e2442de
Show file tree
Hide file tree
Showing 5 changed files with 86 additions and 32 deletions.
10 changes: 6 additions & 4 deletions lib/classes/lock/db_record_lock_factory.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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) {
Expand Down
35 changes: 25 additions & 10 deletions lib/classes/lock/lock_config.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,26 +38,24 @@
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
// types because of an installation error.
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);

Expand All @@ -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;
}

Expand Down
2 changes: 1 addition & 1 deletion lib/classes/lock/postgres_lock_factory.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
38 changes: 32 additions & 6 deletions lib/tests/lock_config_test.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
}

33 changes: 22 additions & 11 deletions lib/tests/lock_test.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -90,20 +105,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);

}

Expand Down

0 comments on commit e2442de

Please sign in to comment.