From f08c28c77a585e35cc17a2248ba61428275ccdd7 Mon Sep 17 00:00:00 2001 From: Oleg Pudeyev Date: Tue, 4 Dec 2012 01:59:27 -0500 Subject: [PATCH 01/11] [ticket/10103] Factor out flock lock class. PHPBB3-10103 --- phpBB/includes/functions_messenger.php | 58 ------------ phpBB/includes/lock/flock.php | 124 +++++++++++++++++++++++++ 2 files changed, 124 insertions(+), 58 deletions(-) create mode 100644 phpBB/includes/lock/flock.php diff --git a/phpBB/includes/functions_messenger.php b/phpBB/includes/functions_messenger.php index cf03de08c4..503f419e5a 100644 --- a/phpBB/includes/functions_messenger.php +++ b/phpBB/includes/functions_messenger.php @@ -650,64 +650,6 @@ class queue $this->data[$object]['data'][] = $scope; } - /** - * Obtains exclusive lock on queue cache file. - * Returns resource representing the lock - */ - function lock() - { - // For systems that can't have two processes opening - // one file for writing simultaneously - if (file_exists($this->cache_file . '.lock')) - { - $mode = 'rb'; - } - else - { - $mode = 'wb'; - } - - $lock_fp = @fopen($this->cache_file . '.lock', $mode); - - if ($mode == 'wb') - { - if (!$lock_fp) - { - // Two processes may attempt to create lock file at the same time. - // Have the losing process try opening the lock file again for reading - // on the assumption that the winning process created it - $mode = 'rb'; - $lock_fp = @fopen($this->cache_file . '.lock', $mode); - } - else - { - // Only need to set mode when the lock file is written - @chmod($this->cache_file . '.lock', 0666); - } - } - - if ($lock_fp) - { - @flock($lock_fp, LOCK_EX); - } - - return $lock_fp; - } - - /** - * Releases lock on queue cache file, using resource obtained from lock() - */ - function unlock($lock_fp) - { - // lock() will return null if opening lock file, and thus locking, failed. - // Accept null values here so that client code does not need to check them - if ($lock_fp) - { - @flock($lock_fp, LOCK_UN); - fclose($lock_fp); - } - } - /** * Process queue * Using lock file diff --git a/phpBB/includes/lock/flock.php b/phpBB/includes/lock/flock.php new file mode 100644 index 0000000000..e24a5f3e1c --- /dev/null +++ b/phpBB/includes/lock/flock.php @@ -0,0 +1,124 @@ +path = $path; + $this->lock_fp = null; + } + + /** + * Tries to acquire the lock. + * + * As a lock may only be held by one process at a time, lock + * acquisition may fail if another process is holding the lock. + * + * @return bool true if lock was acquired + * false otherwise + */ + public function acquire() + { + if ($this->locked) + { + return false; + } + + // For systems that can't have two processes opening + // one file for writing simultaneously + if (file_exists($this->path . '.lock')) + { + $mode = 'rb'; + } + else + { + $mode = 'wb'; + } + + $this->lock_fp = @fopen($this->path . '.lock', $mode); + + if ($mode == 'wb') + { + if (!$this->lock_fp) + { + // Two processes may attempt to create lock file at the same time. + // Have the losing process try opening the lock file again for reading + // on the assumption that the winning process created it + $mode = 'rb'; + $this->lock_fp = @fopen($this->path . '.lock', $mode); + } + else + { + // Only need to set mode when the lock file is written + @chmod($this->path . '.lock', 0666); + } + } + + if ($this->lock_fp) + { + @flock($this->lock_fp, LOCK_EX); + } + + return (bool) $this->lock_fp; + } + + /** + * Releases the lock. + * + * The lock must have been previously obtained, that is, acquire() call + * was issued and returned true. + * + * Note: Attempting to release a lock that is already released, + * that is, calling release() multiple times, is harmless. + * + * @return null + */ + public function release() + { + if ($this->lock_fp) + { + @flock($this->lock_fp, LOCK_UN); + fclose($this->lock_fp); + $this->lock_fp = null; + } + } +} From 4010f4085aa56f20fd95274ecfc2fe7404f98269 Mon Sep 17 00:00:00 2001 From: Oleg Pudeyev Date: Tue, 4 Dec 2012 02:00:27 -0500 Subject: [PATCH 02/11] [ticket/10103] Use flock lock class in messenger. PHPBB3-10103 --- phpBB/includes/functions_messenger.php | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/phpBB/includes/functions_messenger.php b/phpBB/includes/functions_messenger.php index 503f419e5a..56fc7e628f 100644 --- a/phpBB/includes/functions_messenger.php +++ b/phpBB/includes/functions_messenger.php @@ -658,13 +658,14 @@ class queue { global $db, $config, $phpEx, $phpbb_root_path, $user; - $lock_fp = $this->lock(); + $lock = new phpbb_lock_flock($this->cache_file); + $lock->acquire(); set_config('last_queue_run', time(), true); if (!file_exists($this->cache_file) || filemtime($this->cache_file) > time() - $config['queue_interval']) { - $this->unlock($lock_fp); + $lock->release(); return; } @@ -731,7 +732,7 @@ class queue break; default: - $this->unlock($lock_fp); + $lock->release(); return; } @@ -807,7 +808,7 @@ class queue } } - $this->unlock($lock_fp); + $lock->release(); } /** @@ -820,7 +821,8 @@ class queue return; } - $lock_fp = $this->lock(); + $lock = new phpbb_lock_flock($this->cache_file); + $lock->acquire(); if (file_exists($this->cache_file)) { @@ -847,7 +849,7 @@ class queue phpbb_chmod($this->cache_file, CHMOD_READ | CHMOD_WRITE); } - $this->unlock($lock_fp); + $lock->release(); } } From f72e435759e8fafe3b06af35072c1907ba016546 Mon Sep 17 00:00:00 2001 From: Oleg Pudeyev Date: Tue, 4 Dec 2012 02:21:53 -0500 Subject: [PATCH 03/11] [ticket/10103] Test for flock lock class, with concurrency no less. PHPBB3-10103 --- tests/lock/flock_test.php | 108 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 108 insertions(+) create mode 100644 tests/lock/flock_test.php diff --git a/tests/lock/flock_test.php b/tests/lock/flock_test.php new file mode 100644 index 0000000000..5c645de27c --- /dev/null +++ b/tests/lock/flock_test.php @@ -0,0 +1,108 @@ +acquire(); + $this->assertTrue($ok); + $lock->release(); + } + + public function test_consecutive_locking() + { + $path = __DIR__ . '/../tmp/precious'; + + $lock = new phpbb_lock_flock($path); + $ok = $lock->acquire(); + $this->assertTrue($ok); + $lock->release(); + + $ok = $lock->acquire(); + $this->assertTrue($ok); + $lock->release(); + + $ok = $lock->acquire(); + $this->assertTrue($ok); + $lock->release(); + } + + /* This hangs the process. + public function test_concurrent_locking_fail() + { + $path = __DIR__ . '/../tmp/precious'; + + $lock1 = new phpbb_lock_flock($path); + $ok = $lock1->acquire(); + $this->assertTrue($ok); + + $lock2 = new phpbb_lock_flock($path); + $ok = $lock2->acquire(); + $this->assertFalse($ok); + + $lock->release(); + $ok = $lock2->acquire(); + $this->assertTrue($ok); + } + */ + + public function test_concurrent_locking() + { + if (!function_exists('pcntl_fork')) + { + $this->markTestSkipped('pcntl extension and pcntl_fork are required for this test'); + } + + $path = __DIR__ . '/../tmp/precious'; + + if ($pid = pcntl_fork()) + { + // parent + // wait 0.5 s, acquire the lock, note how long it took + sleep(0.5); + + $lock = new phpbb_lock_flock($path); + $start = time(); + $ok = $lock->acquire(); + $delta = time() - $start; + $this->assertTrue($ok); + $this->assertTrue($delta > 0.5); + + $lock->release(); + + // acquire again, this should be instantaneous + $start = time(); + $ok = $lock->acquire(); + $delta = time() - $start; + $this->assertTrue($ok); + $this->assertTrue($delta < 0.1); + + // reap the child + $status = null; + pcntl_waitpid($pid, $status); + } + else + { + // child + // immediately acquire the lock and sleep for 2 s + $lock = new phpbb_lock_flock($path); + $ok = $lock->acquire(); + $this->assertTrue($ok); + sleep(2); + $lock->release(); + + // and go away silently + pcntl_exec('/usr/bin/env', array('true')); + } + } +} From 318140b4d6257dd49ae86ed1185568f4d523e53e Mon Sep 17 00:00:00 2001 From: Oleg Pudeyev Date: Tue, 4 Dec 2012 02:26:55 -0500 Subject: [PATCH 04/11] [ticket/10103] Convert the rest of the tree to flock class. PHPBB3-10103 --- phpBB/includes/cache/driver/file.php | 16 +++++++++++----- phpBB/includes/template/compile.php | 8 +++++--- 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/phpBB/includes/cache/driver/file.php b/phpBB/includes/cache/driver/file.php index a0f06dde4b..ee1b430451 100644 --- a/phpBB/includes/cache/driver/file.php +++ b/phpBB/includes/cache/driver/file.php @@ -653,10 +653,11 @@ class phpbb_cache_driver_file extends phpbb_cache_driver_base $file = "{$this->cache_dir}$filename.$phpEx"; + $lock = new phpbb_lock_flock($file); + $lock->acquire(); + if ($handle = @fopen($file, 'wb')) { - @flock($handle, LOCK_EX); - // File header fwrite($handle, '<' . '?php exit; ?' . '>'); @@ -697,7 +698,6 @@ class phpbb_cache_driver_file extends phpbb_cache_driver_base fwrite($handle, $data); } - @flock($handle, LOCK_UN); fclose($handle); if (!function_exists('phpbb_chmod')) @@ -708,10 +708,16 @@ class phpbb_cache_driver_file extends phpbb_cache_driver_base phpbb_chmod($file, CHMOD_READ | CHMOD_WRITE); - return true; + $rv = true; + } + else + { + $rv = false; } - return false; + $lock->release(); + + return $rv; } /** diff --git a/phpBB/includes/template/compile.php b/phpBB/includes/template/compile.php index d0b3d0f115..22da21820e 100644 --- a/phpBB/includes/template/compile.php +++ b/phpBB/includes/template/compile.php @@ -58,6 +58,9 @@ class phpbb_template_compile */ public function compile_file_to_file($source_file, $compiled_file) { + $lock = new phpbb_lock_flock($compiled_file); + $lock->acquire(); + $source_handle = @fopen($source_file, 'rb'); $destination_handle = @fopen($compiled_file, 'wb'); @@ -66,16 +69,15 @@ class phpbb_template_compile return false; } - @flock($destination_handle, LOCK_EX); - $this->compile_stream_to_stream($source_handle, $destination_handle); @fclose($source_handle); - @flock($destination_handle, LOCK_UN); @fclose($destination_handle); phpbb_chmod($compiled_file, CHMOD_READ | CHMOD_WRITE); + $lock->release(); + clearstatcache(); return true; From fc410e1cd0071a2de3dc90ce62a5abbef0266f15 Mon Sep 17 00:00:00 2001 From: Oleg Pudeyev Date: Tue, 4 Dec 2012 02:38:57 -0500 Subject: [PATCH 05/11] [ticket/10103] Try a longer sleep for travis. Apparently travis takes longer than half a second to fork php. PHPBB3-10103 --- tests/lock/flock_test.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/lock/flock_test.php b/tests/lock/flock_test.php index 5c645de27c..bc682f9410 100644 --- a/tests/lock/flock_test.php +++ b/tests/lock/flock_test.php @@ -69,7 +69,7 @@ class phpbb_lock_flock_test extends phpbb_test_case { // parent // wait 0.5 s, acquire the lock, note how long it took - sleep(0.5); + sleep(1); $lock = new phpbb_lock_flock($path); $start = time(); From 4cc81f1ffa62cdbcbb656300c7dd9fbd44cc21fc Mon Sep 17 00:00:00 2001 From: Oleg Pudeyev Date: Tue, 4 Dec 2012 13:44:22 -0500 Subject: [PATCH 06/11] [ticket/10103] Correct flock class documentation. PHPBB3-10103 --- phpBB/includes/lock/flock.php | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/phpBB/includes/lock/flock.php b/phpBB/includes/lock/flock.php index e24a5f3e1c..09450644bc 100644 --- a/phpBB/includes/lock/flock.php +++ b/phpBB/includes/lock/flock.php @@ -35,9 +35,9 @@ class phpbb_lock_flock private $lock_fp; /** - * Creates an instance of the lock. + * Constructor. * - * You have to call acquire() to actually create the lock. + * You have to call acquire() to actually acquire the lock. * * @param string $path Path to the file access to which is controlled */ @@ -50,8 +50,17 @@ class phpbb_lock_flock /** * Tries to acquire the lock. * - * As a lock may only be held by one process at a time, lock - * acquisition may fail if another process is holding the lock. + * If the lock is already held by another process, this call will block + * until the other process releases the lock. If a lock is acquired and + * is not released before script finishes but the process continues to + * live (apache/fastcgi) then subsequent processes trying to acquire + * the same lock will be blocked forever. + * + * If the lock is already held by the same process via another instance + * of this class, this call will block forever. + * + * If flock function is disabled in php or fails to work, lock + * acquisition will fail and false will be returned. * * @return bool true if lock was acquired * false otherwise From 3924676f2bd1a30ff56ab1db985cf622f1fac286 Mon Sep 17 00:00:00 2001 From: Oleg Pudeyev Date: Tue, 4 Dec 2012 13:45:02 -0500 Subject: [PATCH 07/11] [ticket/10103] $rv had too few characters. PHPBB3-10103 --- phpBB/includes/cache/driver/file.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/phpBB/includes/cache/driver/file.php b/phpBB/includes/cache/driver/file.php index ee1b430451..691abe0438 100644 --- a/phpBB/includes/cache/driver/file.php +++ b/phpBB/includes/cache/driver/file.php @@ -708,16 +708,16 @@ class phpbb_cache_driver_file extends phpbb_cache_driver_base phpbb_chmod($file, CHMOD_READ | CHMOD_WRITE); - $rv = true; + $return_value = true; } else { - $rv = false; + $return_value = false; } $lock->release(); - return $rv; + return $return_value; } /** From a553cfbc30472f136f9904d97bf4d09a7187518f Mon Sep 17 00:00:00 2001 From: Oleg Pudeyev Date: Tue, 4 Dec 2012 13:46:01 -0500 Subject: [PATCH 08/11] [ticket/10103] Inline assignment is bad? PHPBB3-10103 --- tests/lock/flock_test.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/lock/flock_test.php b/tests/lock/flock_test.php index bc682f9410..abcb4e79c2 100644 --- a/tests/lock/flock_test.php +++ b/tests/lock/flock_test.php @@ -65,7 +65,8 @@ class phpbb_lock_flock_test extends phpbb_test_case $path = __DIR__ . '/../tmp/precious'; - if ($pid = pcntl_fork()) + $pid = pcntl_fork(); + if ($pid) { // parent // wait 0.5 s, acquire the lock, note how long it took From 285feb49f82084b5b489aa1fc6765b9b02ea1b29 Mon Sep 17 00:00:00 2001 From: Oleg Pudeyev Date: Tue, 4 Dec 2012 13:47:57 -0500 Subject: [PATCH 09/11] [ticket/10103] assertLessThan/assertGreaterThan. PHPBB3-10103 --- tests/lock/flock_test.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/lock/flock_test.php b/tests/lock/flock_test.php index abcb4e79c2..8bb7d0a041 100644 --- a/tests/lock/flock_test.php +++ b/tests/lock/flock_test.php @@ -77,7 +77,7 @@ class phpbb_lock_flock_test extends phpbb_test_case $ok = $lock->acquire(); $delta = time() - $start; $this->assertTrue($ok); - $this->assertTrue($delta > 0.5); + $this->assertGreaterThan(0.5, $delta); $lock->release(); @@ -86,7 +86,7 @@ class phpbb_lock_flock_test extends phpbb_test_case $ok = $lock->acquire(); $delta = time() - $start; $this->assertTrue($ok); - $this->assertTrue($delta < 0.1); + $this->assertLessThan(0.1, $delta); // reap the child $status = null; From e22dd7dfadedd951d1fd17b61fa700c572ca4d79 Mon Sep 17 00:00:00 2001 From: Oleg Pudeyev Date: Tue, 4 Dec 2012 13:50:35 -0500 Subject: [PATCH 10/11] [ticket/10103] Assert with messages. PHPBB3-10103 --- tests/lock/flock_test.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/lock/flock_test.php b/tests/lock/flock_test.php index 8bb7d0a041..1edc96b3a4 100644 --- a/tests/lock/flock_test.php +++ b/tests/lock/flock_test.php @@ -77,7 +77,7 @@ class phpbb_lock_flock_test extends phpbb_test_case $ok = $lock->acquire(); $delta = time() - $start; $this->assertTrue($ok); - $this->assertGreaterThan(0.5, $delta); + $this->assertGreaterThan(0.5, $delta, 'First lock acquired too soon'); $lock->release(); @@ -86,7 +86,7 @@ class phpbb_lock_flock_test extends phpbb_test_case $ok = $lock->acquire(); $delta = time() - $start; $this->assertTrue($ok); - $this->assertLessThan(0.1, $delta); + $this->assertLessThan(0.1, $delta, 'Second lock not acquired instantaneously'); // reap the child $status = null; From 3e093c282a63df4d16b212859fd8137fd2bbca81 Mon Sep 17 00:00:00 2001 From: Oleg Pudeyev Date: Tue, 4 Dec 2012 14:05:49 -0500 Subject: [PATCH 11/11] [ticket/10103] New and improved wording. PHPBB3-10103 --- phpBB/includes/lock/flock.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/phpBB/includes/lock/flock.php b/phpBB/includes/lock/flock.php index 09450644bc..5c2288ce1b 100644 --- a/phpBB/includes/lock/flock.php +++ b/phpBB/includes/lock/flock.php @@ -22,7 +22,7 @@ if (!defined('IN_PHPBB')) class phpbb_lock_flock { /** - * Path to the file access to which is controlled + * Path to the file to which access is controlled * * @var string */ @@ -39,7 +39,7 @@ class phpbb_lock_flock * * You have to call acquire() to actually acquire the lock. * - * @param string $path Path to the file access to which is controlled + * @param string $path Path to the file to which access is controlled */ public function __construct($path) {