diff --git a/phpBB/includes/lock/db.php b/phpBB/includes/lock/db.php index e9885285d9..092f4f9789 100644 --- a/phpBB/includes/lock/db.php +++ b/phpBB/includes/lock/db.php @@ -42,7 +42,7 @@ class phpbb_lock_db /** * The phpBB configuration - * @var array + * @var phpbb_config */ private $config; @@ -61,7 +61,7 @@ class phpbb_lock_db * @param array $config The phpBB configuration * @param dbal $db A database connection */ - public function __construct($config_name, $config, dbal $db) + public function __construct($config_name, phpbb_config $config, dbal $db) { $this->config_name = $config_name; $this->config = $config; @@ -69,8 +69,8 @@ class phpbb_lock_db } /** - * Tries to acquire the cron lock by updating - * the 'cron_lock' configuration variable in the database. + * Tries to acquire the lock by updating + * the configuration variable in the database. * * As a lock may only be held by one process at a time, lock * acquisition may fail if another process is holding the lock @@ -89,13 +89,11 @@ class phpbb_lock_db if (!isset($this->config[$this->config_name])) { - set_config($this->config_name, '0', true); - global $config; - $this->config = $config; + $this->config->set($this->config_name, '0', false); } $lock_value = $this->config[$this->config_name]; - // make sure cron doesn't run multiple times in parallel + // make sure lock cannot be acquired by multiple processes if ($lock_value) { // if the other process is running more than an hour already we have to assume it @@ -111,33 +109,20 @@ class phpbb_lock_db $this->unique_id = time() . ' ' . unique_id(); - $sql = 'UPDATE ' . CONFIG_TABLE . " - SET config_value = '" . $this->db->sql_escape($this->unique_id) . "' - WHERE config_name = '" . $this->db->sql_escape($this->config_name) . "' - AND config_value = '" . $this->db->sql_escape($lock_value) . "'"; - $this->db->sql_query($sql); - - if ($this->db->sql_affectedrows()) - { - $this->locked = true; - } - else - { - // another cron process altered the table between script start and - // UPDATE query so return false - $this->locked = false; - } + // try to update the config value, if it was already modified by another + // process we failed to acquire the lock. + $this->locked = $this->config->set_atomic($this->config_name, $lock_value, $this->unique_id, false); return $this->locked; } /** - * Releases the cron lock. + * Releases the lock. * * The lock must have been previously obtained, that is, lock() call * was issued and returned true. * - * Note: Attempting to release a cron lock that is already released, + * Note: Attempting to release a lock that is already released, * that is, calling unlock() multiple times, is harmless. * * @return void @@ -146,12 +131,7 @@ class phpbb_lock_db { if ($this->locked) { - $sql = 'UPDATE ' . CONFIG_TABLE . " - SET config_value = '0' - WHERE config_name = '" . $this->db->sql_escape($this->config_name) . "' - AND config_value = '" . $this->db->sql_escape($this->unique_id) . "'"; - $this->db->sql_query($sql); - + $this->config->set_atomic($this->config_name, $this->unique_id, '0', false); $this->locked = false; } } diff --git a/tests/lock/db_test.php b/tests/lock/db_test.php index 0702a2c21e..f60d7e3ee8 100644 --- a/tests/lock/db_test.php +++ b/tests/lock/db_test.php @@ -25,22 +25,21 @@ class phpbb_lock_db_test extends phpbb_database_test_case global $db, $config; $db = $this->db = $this->new_dbal(); - $config = $this->config = array('rand_seed' => '', 'rand_seed_last_update' => '0'); + $config = $this->config = new phpbb_config(array('rand_seed' => '', 'rand_seed_last_update' => '0')); + set_config(null, null, null, $this->config); $this->lock = new phpbb_lock_db('test_lock', $this->config, $this->db); } public function test_new_lock() { - global $config; - $this->assertTrue($this->lock->lock()); - $this->assertTrue(isset($config['test_lock']), 'Lock was created'); + $this->assertTrue(isset($this->config['test_lock']), 'Lock was created'); - $lock2 = new phpbb_lock_db('test_lock', $config, $this->db); + $lock2 = new phpbb_lock_db('test_lock', $this->config, $this->db); $this->assertFalse($lock2->lock()); $this->lock->unlock(); - $this->assertEquals('0', $config['test_lock'], 'Lock was released'); + $this->assertEquals('0', $this->config['test_lock'], 'Lock was released'); } public function test_expire_lock() @@ -51,17 +50,34 @@ class phpbb_lock_db_test extends phpbb_database_test_case public function test_double_lock() { - global $config; + $this->assertTrue($this->lock->lock()); + $this->assertTrue(isset($this->config['test_lock']), 'Lock was created'); + + $value = $this->config['test_lock']; $this->assertTrue($this->lock->lock()); - $this->assertTrue(isset($config['test_lock']), 'Lock was created'); - - $value = $config['test_lock']; - - $this->assertTrue($this->lock->lock()); - $this->assertEquals($value, $config['test_lock'], 'Second lock was ignored'); + $this->assertEquals($value, $this->config['test_lock'], 'Second lock was ignored'); $this->lock->unlock(); - $this->assertEquals('0', $config['test_lock'], 'Lock was released'); + $this->assertEquals('0', $this->config['test_lock'], 'Lock was released'); + } + + public function test_double_unlock() + { + $this->assertTrue($this->lock->lock()); + $this->assertFalse(empty($this->config['test_lock']), 'First lock is acquired'); + + $this->lock->unlock(); + $this->assertEquals('0', $this->config['test_lock'], 'First lock is released'); + + $lock2 = new phpbb_lock_db('test_lock', $this->config, $this->db); + $this->assertTrue($lock2->lock()); + $this->assertFalse(empty($this->config['test_lock']), 'Second lock is acquired'); + + $this->lock->unlock(); + $this->assertFalse(empty($this->config['test_lock']), 'Double release of first lock is ignored'); + + $lock2->unlock(); + $this->assertEquals('0', $this->config['test_lock'], 'Second lock is released'); } }