diff --git a/lib/classes/lock/lock.php b/lib/classes/lock/lock.php index 7d1a2816553..7ba43a2205b 100644 --- a/lib/classes/lock/lock.php +++ b/lib/classes/lock/lock.php @@ -52,12 +52,9 @@ class lock { $this->factory = $factory; $this->key = $key; $this->released = false; - $caller = debug_backtrace(true, 2)[1]; - if ($caller && array_key_exists('file', $caller ) ) { - $this->caller = $caller['file'] . ' on line ' . $caller['line']; - } else if ($caller && array_key_exists('class', $caller)) { - $this->caller = $caller['class'] . $caller['type'] . $caller['function']; - } + + // Track where the lock was raised, so we can report un-released locks in a helpful way. + $this->caller = format_backtrace(debug_backtrace(), true); } /** @@ -113,7 +110,6 @@ class lock { if ($withexception) { throw new \core\exception\coding_exception(<<caller} - Code should look like: \$factory = \core\lock\lock_config::get_lock_factory('type'); @@ -128,6 +124,7 @@ class lock { * Print debugging if this lock falls out of scope before being released. */ public function __destruct() { - $this->release_if_not_released(defined('PHPUNIT_TEST')); + global $CFG; + $this->release_if_not_released(defined('BEHAT_SITE_RUNNING') || PHPUNIT_TEST || $CFG->debugdeveloper); } } diff --git a/lib/tests/lock_test.php b/lib/tests/lock_test.php index 50c85c7afbd..62148cbb7ee 100644 --- a/lib/tests/lock_test.php +++ b/lib/tests/lock_test.php @@ -16,6 +16,8 @@ namespace core; +use core\exception\coding_exception; + /** * Unit tests for our locking implementations. * @@ -23,6 +25,9 @@ namespace core; * @category test * @copyright 2013 Damyon Wiese * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @covers \core\lock\db_record_lock_factory + * @covers \core\lock\file_lock_factory + * @covers \core\lock\lock */ final class lock_test extends \advanced_testcase { @@ -37,9 +42,9 @@ final class lock_test extends \advanced_testcase { /** * Run a suite of tests on a lock factory class. * - * @param class $lockfactoryclass - A lock factory class to test + * @param string $lockfactoryclass - name of a lock factory class to test. */ - protected function run_on_lock_factory($lockfactoryclass) { + protected function run_on_lock_factory(string $lockfactoryclass): void { $modassignfactory = new $lockfactoryclass('mod_assign'); $tooltaskfactory = new $lockfactoryclass('tool_task'); @@ -127,8 +132,24 @@ final class lock_test extends \advanced_testcase { // Manually create the core no-configuration factories. $this->run_on_lock_factory(\core\lock\db_record_lock_factory::class); $this->run_on_lock_factory(\core\lock\file_lock_factory::class); - } + public function test_exception_for_unreleased_locks(): void { + $factory = new \core\lock\db_record_lock_factory('mod_assign'); + $lock = $factory->get_lock('abc', 0); + + // Using try/catch, not expectException, because I want to verify a few things. + try { + $lock->release_if_not_released(true); + } catch (coding_exception $e) { + $this->assertStringContainsString('A lock was created but not released at:', $e->getMessage()); + $this->assertStringContainsString('call to core\lock\db_record_lock_factory->get_lock()', $e->getMessage()); + $this->assertStringContainsString('call to core\lock_test->test_exception_for_unreleased_locks()', $e->getMessage()); + return; + } + + // Now use expectException to get the standard failure message. + $this->expectException(coding_exception::class); + } }