Merge remote-tracking branch 'p/ticket/10103' into develop

* p/ticket/10103:
  [ticket/10103] New and improved wording.
  [ticket/10103] Assert with messages.
  [ticket/10103] assertLessThan/assertGreaterThan.
  [ticket/10103] Inline assignment is bad?
  [ticket/10103] $rv had too few characters.
  [ticket/10103] Correct flock class documentation.
  [ticket/10103] Try a longer sleep for travis.
  [ticket/10103] Convert the rest of the tree to flock class.
  [ticket/10103] Test for flock lock class, with concurrency no less.
  [ticket/10103] Use flock lock class in messenger.
  [ticket/10103] Factor out flock lock class.

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;
+			$return_value = true;
+		}
+		else
+		{
+			$return_value = false;
 		}
 
-		return false;
+		$lock->release();
+
+		return $return_value;
 	}
 
 	/**

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
@@ -716,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;
 		}
 
@@ -789,7 +732,7 @@ class queue
 				break;
 
 				default:
-					$this->unlock($lock_fp);
+					$lock->release();
 					return;
 			}
 
@@ -865,7 +808,7 @@ class queue
 			}
 		}
 
-		$this->unlock($lock_fp);
+		$lock->release();
 	}
 
 	/**
@@ -878,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))
 		{
@@ -905,7 +849,7 @@ class queue
 			phpbb_chmod($this->cache_file, CHMOD_READ | CHMOD_WRITE);
 		}
 
-		$this->unlock($lock_fp);
+		$lock->release();
 	}
 }
 

phpBB/includes/lock/flock.php

@@ -0,0 +1,133 @@
+<?php
+/**
+*
+* @package phpBB3
+* @copyright (c) 2012 phpBB Group
+* @license http://opensource.org/licenses/gpl-2.0.php GNU General Public License v2
+*
+*/
+
+/**
+* @ignore
+*/
+if (!defined('IN_PHPBB'))
+{
+	exit;
+}
+
+/**
+* File locking class
+* @package phpBB3
+*/
+class phpbb_lock_flock
+{
+	/**
+	* Path to the file to which access is controlled
+	*
+	* @var string
+	*/
+	private $path;
+
+	/**
+	* File pointer for the lock file
+	* @var string
+	*/
+	private $lock_fp;
+
+	/**
+	* Constructor.
+	*
+	* You have to call acquire() to actually acquire the lock.
+	*
+	* @param	string	$path	Path to the file to which access is controlled
+	*/
+	public function __construct($path)
+	{
+		$this->path = $path;
+		$this->lock_fp = null;
+	}
+
+	/**
+	* Tries to acquire 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
+	*/
+	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;
+		}
+	}
+}

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;

tests/lock/flock_test.php

@@ -0,0 +1,109 @@
+<?php
+/**
+*
+* @package testing
+* @copyright (c) 2012 phpBB Group
+* @license http://opensource.org/licenses/gpl-2.0.php GNU General Public License v2
+*
+*/
+
+class phpbb_lock_flock_test extends phpbb_test_case
+{
+	public function test_lock()
+	{
+		$path = __DIR__ . '/../tmp/precious';
+
+		$lock = new phpbb_lock_flock($path);
+		$ok = $lock->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';
+
+		$pid = pcntl_fork();
+		if ($pid)
+		{
+			// parent
+			// wait 0.5 s, acquire the lock, note how long it took
+			sleep(1);
+
+			$lock = new phpbb_lock_flock($path);
+			$start = time();
+			$ok = $lock->acquire();
+			$delta = time() - $start;
+			$this->assertTrue($ok);
+			$this->assertGreaterThan(0.5, $delta, 'First lock acquired too soon');
+
+			$lock->release();
+
+			// acquire again, this should be instantaneous
+			$start = time();
+			$ok = $lock->acquire();
+			$delta = time() - $start;
+			$this->assertTrue($ok);
+			$this->assertLessThan(0.1, $delta, 'Second lock not acquired instantaneously');
+
+			// 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'));
+		}
+	}
+}