From 01ef46a510c067d7699a3495ca533aee5f112ae4 Mon Sep 17 00:00:00 2001 From: Oleg Pudeyev Date: Tue, 20 Apr 2010 06:53:52 -0400 Subject: [ticket/9061] Fixed a race condition in queue locking. Changed queue locking to cover all queue file operations, in particular the check for queue file existince and inclusion of queue file must be done under one lock. Also refactored queue locking and unlocking into separate methods. PHPBB3-9061 --- phpBB/includes/functions_messenger.php | 81 ++++++++++++++++++++++++++-------- 1 file changed, 62 insertions(+), 19 deletions(-) (limited to 'phpBB/includes/functions_messenger.php') diff --git a/phpBB/includes/functions_messenger.php b/phpBB/includes/functions_messenger.php index 99883cd9ca..f22a77cf3c 100644 --- a/phpBB/includes/functions_messenger.php +++ b/phpBB/includes/functions_messenger.php @@ -631,6 +631,58 @@ 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); + // 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 + if (!$lock_fp && $mode == 'wb') + { + // Assign to $mode for the check in chmod below + $mode = 'rb'; + $lock_fp = @fopen($this->cache_file . '.lock', $mode); + } + if ($lock_fp && $mode == 'wb') + { + // 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 @@ -639,24 +691,16 @@ class queue { global $db, $config, $phpEx, $phpbb_root_path, $user; - set_config('last_queue_run', time(), true); + $lock_fp = $this->lock(); - // Delete stale lock file - if (file_exists($this->cache_file . '.lock') && !file_exists($this->cache_file)) - { - @unlink($this->cache_file . '.lock'); - return; - } + set_config('last_queue_run', time(), true); - if (!file_exists($this->cache_file) || (file_exists($this->cache_file . '.lock') && filemtime($this->cache_file) > time() - $config['queue_interval'])) + if (!file_exists($this->cache_file) || filemtime($this->cache_file) > time() - $config['queue_interval']) { + $this->unlock($lock_fp); return; } - $fp = @fopen($this->cache_file . '.lock', 'wb'); - fclose($fp); - @chmod($this->cache_file . '.lock', 0777); - include($this->cache_file); foreach ($this->queue_data as $object => $data_ary) @@ -713,6 +757,7 @@ class queue break; default: + $this->unlock($lock_fp); return; } @@ -738,8 +783,6 @@ class queue if (!$result) { - @unlink($this->cache_file . '.lock'); - messenger::error('EMAIL', $err_msg); continue 2; } @@ -783,16 +826,14 @@ class queue { if ($fp = @fopen($this->cache_file, 'wb')) { - @flock($fp, LOCK_EX); fwrite($fp, "queue_data = unserialize(" . var_export(serialize($this->queue_data), true) . ");\n\n?>"); - @flock($fp, LOCK_UN); fclose($fp); phpbb_chmod($this->cache_file, CHMOD_READ | CHMOD_WRITE); } } - @unlink($this->cache_file . '.lock'); + $this->unlock($lock_fp); } /** @@ -805,6 +846,8 @@ class queue return; } + $lock_fp = $this->lock(); + if (file_exists($this->cache_file)) { include($this->cache_file); @@ -824,13 +867,13 @@ class queue if ($fp = @fopen($this->cache_file, 'w')) { - @flock($fp, LOCK_EX); fwrite($fp, "queue_data = unserialize(" . var_export(serialize($this->data), true) . ");\n\n?>"); - @flock($fp, LOCK_UN); fclose($fp); phpbb_chmod($this->cache_file, CHMOD_READ | CHMOD_WRITE); } + + $this->unlock($lock_fp); } } -- cgit v1.2.1 From 981970024701bd1e740056c595c959afd5a85ba0 Mon Sep 17 00:00:00 2001 From: Chris Smith Date: Thu, 9 Sep 2010 20:05:11 +0100 Subject: [ticket/9061] Simplify conditional statements by reworking the logic. PHPBB3-9061 --- phpBB/includes/functions_messenger.php | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-) (limited to 'phpBB/includes/functions_messenger.php') diff --git a/phpBB/includes/functions_messenger.php b/phpBB/includes/functions_messenger.php index f22a77cf3c..227df95ac5 100644 --- a/phpBB/includes/functions_messenger.php +++ b/phpBB/includes/functions_messenger.php @@ -647,25 +647,31 @@ class queue { $mode = 'wb'; } + $lock_fp = @fopen($this->cache_file . '.lock', $mode); - // 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 - if (!$lock_fp && $mode == 'wb') - { - // Assign to $mode for the check in chmod below - $mode = 'rb'; - $lock_fp = @fopen($this->cache_file . '.lock', $mode); - } - if ($lock_fp && $mode == 'wb') + + if ($mode == 'wb') { - // Only need to set mode when the lock file is written - @chmod($this->cache_file . '.lock', 0666); + 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; } -- cgit v1.2.1