[feature/system-cron] Abstract the database locking mechanism out of cron.

Added a number of tests for the locking mechanism which can now lock arbitrary config variables. PHPBB3-9596
author: Nils Adermann <naderman@naderman.de> 2011-01-07 20:58:28 +0100
committer: Oleg Pudeyev <oleg@bsdpower.com> 2011-02-12 22:05:53 -0500
commit: 3a3a8bb96d0cb7be2529ab095f305fd3b042783c (patch)
tree: e8bc13cacb8ab2f9803c2d13d55025e4903e4d7f
parent: 6e5e4721d83c8bde9780b02bd011bdbf5d188dea (diff)
download: forums-3a3a8bb96d0cb7be2529ab095f305fd3b042783c.tar
forums-3a3a8bb96d0cb7be2529ab095f305fd3b042783c.tar.gz
forums-3a3a8bb96d0cb7be2529ab095f305fd3b042783c.tar.bz2
forums-3a3a8bb96d0cb7be2529ab095f305fd3b042783c.tar.xz
forums-3a3a8bb96d0cb7be2529ab095f305fd3b042783c.zip
5 files changed, 243 insertions, 109 deletions
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 @@
-<?php
-/**
-*
-* @package phpBB3
-* @copyright (c) 2010 phpBB Group
-* @license http://opensource.org/licenses/gpl-license.php GNU Public License
-*
-*/
-
-/**
-* @ignore
-*/
-if (!defined('IN_PHPBB'))
-{
-	exit;
-}
-
-/**
-* Cron lock class
-* @package phpBB3
-*/
-class phpbb_cron_lock
-{
-	/**
-	* Unique identifier for this lock.
-	*
-	* @var string
-	*/
-	private $cron_id;
-
-	/**
-	* 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()
-	{
-		global $config, $db;
-
-		if (!isset($config['cron_lock']))
-		{
-			set_config('cron_lock', '0', true);
-		}
-
-		// make sure cron doesn't run multiple times in parallel
-		if ($config['cron_lock'])
-		{
-			// if the other process is running more than an hour already we have to assume it
-			// aborted without cleaning the lock
-			$time = explode(' ', $config['cron_lock']);
-			$time = $time[0];
-
-			if ($time + 3600 >= 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 @@
+<?php
+/**
+*
+* @package phpBB3
+* @copyright (c) 2010 phpBB Group
+* @license http://opensource.org/licenses/gpl-license.php GNU Public License
+*
+*/
+
+/**
+* @ignore
+*/
+if (!defined('IN_PHPBB'))
+{
+	exit;
+}
+
+/**
+* Database locking class
+* @package phpBB3
+*/
+class phpbb_lock_db
+{
+	/**
+	* Name of the config variable this lock uses
+	* @var string
+	*/
+	private $config_name;
+
+	/**
+	* Unique identifier for this lock.
+	*
+	* @var string
+	*/
+	private $unique_id;
+
+	/**
+	* Stores the state of this lock
+	* @var bool
+	*/
+	private $locked;
+
+	/**
+	* The phpBB configuration
+	* @var array
+	*/
+	private $config;
+
+	/**
+	* A database connection
+	* @var dbal
+	*/
+	private $db;
+
+	/**
+	* Creates a named released instance of the lock.
+	*
+	* You have to call lock to actually create the lock.
+	*
+	* @param	string	$config_name	A config variable to be used for locking
+	* @param	array	$config			The phpBB configuration
+	* @param	dbal	$db				A database connection
+	*/
+	public function __construct($config_name, $config, dbal $db)
+	{
+		$this->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 @@
+<?php
+/**
+*
+* @package testing
+* @copyright (c) 2010 phpBB Group
+* @license http://opensource.org/licenses/gpl-license.php GNU Public License
+*
+*/
+
+require_once __DIR__ . '/../../phpBB/includes/functions.php';
+
+class phpbb_lock_db_test extends phpbb_database_test_case
+{
+	private $db;
+	private $config;
+	private $lock;
+
+	public function getDataSet()
+	{
+		return $this->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 @@
+<?xml version="1.0" encoding="UTF-8" ?>
+<dataset>
+	<table name="phpbb_config">
+		<column>config_name</column>
+		<column>config_value</column>
+		<column>is_dynamic</column>
+		<row>
+			<value>foo_lock</value>
+			<value>1 abcd</value>
+			<value>1</value>
+		</row>
+	</table>
+</dataset>
author	Nils Adermann <naderman@naderman.de>	2011-01-07 20:58:28 +0100
committer	Oleg Pudeyev <oleg@bsdpower.com>	2011-02-12 22:05:53 -0500
commit	3a3a8bb96d0cb7be2529ab095f305fd3b042783c (patch)
tree	e8bc13cacb8ab2f9803c2d13d55025e4903e4d7f
parent	6e5e4721d83c8bde9780b02bd011bdbf5d188dea (diff)
download	forums-3a3a8bb96d0cb7be2529ab095f305fd3b042783c.tar forums-3a3a8bb96d0cb7be2529ab095f305fd3b042783c.tar.gz forums-3a3a8bb96d0cb7be2529ab095f305fd3b042783c.tar.bz2 forums-3a3a8bb96d0cb7be2529ab095f305fd3b042783c.tar.xz forums-3a3a8bb96d0cb7be2529ab095f305fd3b042783c.zip