diff --git a/phpBB/cron.php b/phpBB/cron.php index d1b96b12e1..80e744d358 100644 --- a/phpBB/cron.php +++ b/phpBB/cron.php @@ -33,9 +33,9 @@ function output_image() // flush(); } -function do_cron($run_tasks) +function do_cron($cron_lock, $run_tasks) { - global $cron_lock, $config; + global $config; foreach ($run_tasks as $task) { @@ -73,7 +73,7 @@ else output_image(); } -$cron_lock = new phpbb_cron_lock(); +$cron_lock = new phpbb_lock_db('cron_lock', $config, $db); if ($cron_lock->lock()) { if ($config['use_system_cron']) @@ -103,11 +103,11 @@ if ($cron_lock->lock()) } if ($use_shutdown_function) { - register_shutdown_function('do_cron', $run_tasks); + register_shutdown_function('do_cron', $cron_lock, $run_tasks); } else { - do_cron($run_tasks); + do_cron($cron_lock, $run_tasks); } } else diff --git a/phpBB/includes/cron/lock.php b/phpBB/includes/cron/lock.php deleted file mode 100644 index 27b8b425c1..0000000000 --- a/phpBB/includes/cron/lock.php +++ /dev/null @@ -1,104 +0,0 @@ -= time()) - { - return false; - } - } - - $this->cron_id = time() . ' ' . unique_id(); - - $sql = 'UPDATE ' . CONFIG_TABLE . " - SET config_value = '" . $db->sql_escape($this->cron_id) . "' - WHERE config_name = 'cron_lock' - AND config_value = '" . $db->sql_escape($config['cron_lock']) . "'"; - $db->sql_query($sql); - - // another cron process altered the table between script start and UPDATE query so exit - if ($db->sql_affectedrows() != 1) - { - return false; - } - - return true; - } - - /** - * Releases the cron 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, - * that is, calling unlock() multiple times, is harmless. - * - * @return void - */ - public function unlock() - { - global $db; - - $sql = 'UPDATE ' . CONFIG_TABLE . " - SET config_value = '0' - WHERE config_name = 'cron_lock' - AND config_value = '" . $db->sql_escape($this->cron_id) . "'"; - $db->sql_query($sql); - } -} diff --git a/phpBB/includes/lock/db.php b/phpBB/includes/lock/db.php new file mode 100644 index 0000000000..e9885285d9 --- /dev/null +++ b/phpBB/includes/lock/db.php @@ -0,0 +1,158 @@ +config_name = $config_name; + $this->config = $config; + $this->db = $db; + } + + /** + * Tries to acquire the cron lock by updating + * the 'cron_lock' 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 + * or if another process obtained the lock but never released it. + * Locks are forcibly released after a timeout of 1 hour. + * + * @return bool true if lock was acquired + * false otherwise + */ + public function lock() + { + if ($this->locked) + { + return true; + } + + if (!isset($this->config[$this->config_name])) + { + set_config($this->config_name, '0', true); + global $config; + $this->config = $config; + } + $lock_value = $this->config[$this->config_name]; + + // make sure cron doesn't run multiple times in parallel + if ($lock_value) + { + // if the other process is running more than an hour already we have to assume it + // aborted without cleaning the lock + $time = explode(' ', $lock_value); + $time = $time[0]; + + if ($time + 3600 >= time()) + { + return false; + } + } + + $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; + } + + return $this->locked; + } + + /** + * Releases the cron 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, + * that is, calling unlock() multiple times, is harmless. + * + * @return void + */ + public function unlock() + { + 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->locked = false; + } + } +} diff --git a/tests/lock/db_test.php b/tests/lock/db_test.php new file mode 100644 index 0000000000..0702a2c21e --- /dev/null +++ b/tests/lock/db_test.php @@ -0,0 +1,67 @@ +createXMLDataSet(dirname(__FILE__).'/fixtures/config.xml'); + } + + public function setUp() + { + global $db, $config; + + $db = $this->db = $this->new_dbal(); + $config = $this->config = array('rand_seed' => '', 'rand_seed_last_update' => '0'); + $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'); + + $lock2 = new phpbb_lock_db('test_lock', $config, $this->db); + $this->assertFalse($lock2->lock()); + + $this->lock->unlock(); + $this->assertEquals('0', $config['test_lock'], 'Lock was released'); + } + + public function test_expire_lock() + { + $lock = new phpbb_lock_db('foo_lock', $this->config, $this->db); + $this->assertTrue($lock->lock()); + } + + public function test_double_lock() + { + global $config; + + $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->lock->unlock(); + $this->assertEquals('0', $config['test_lock'], 'Lock was released'); + } +} diff --git a/tests/lock/fixtures/config.xml b/tests/lock/fixtures/config.xml new file mode 100644 index 0000000000..f36c8b929a --- /dev/null +++ b/tests/lock/fixtures/config.xml @@ -0,0 +1,13 @@ + + + + config_name + config_value + is_dynamic + + foo_lock + 1 abcd + 1 + +
+