jenkins-bot has submitted this change and it was merged. Change subject: Move pid locking code from Utils into a dedicated class ......................................................................
Move pid locking code from Utils into a dedicated class And add tests. Pid-lock file naming unchanged. Sill not super nice, but way better. Change-Id: Ifde86f49a3f536004c20417b5f2eceb1c9a70506 --- A lib/includes/PidLock.php M lib/includes/Utils.php A lib/tests/phpunit/PidLockTest.php M repo/maintenance/pruneChanges.php 4 files changed, 195 insertions(+), 120 deletions(-) Approvals: Thiemo Mättig (WMDE): Looks good to me, approved jenkins-bot: Verified diff --git a/lib/includes/PidLock.php b/lib/includes/PidLock.php new file mode 100644 index 0000000..2e7e686 --- /dev/null +++ b/lib/includes/PidLock.php @@ -0,0 +1,147 @@ +<?php + +namespace Wikibase\Lib; + +/** + * Utility class for pid-locking. + * + * @since 0.5 + * + * @license GNU GPL v2+ + * @author Jeroen De Dauw < [email protected] > + * @author Tobias Gritschacher + * @author Jens Ohlig < [email protected] > + * @author John Erling Blad < [email protected] > + * @author Marius Hoch < [email protected] > + */ +class PidLock { + + /** + * @var string + */ + private $module; + + /** + * @var string|null + */ + private $wikiId; + + /** + * @param string $module used as a basis for the file name. + * @param string $wikiId the wiki's id, used for per-wiki file names. Defaults to wfWikiID(). + */ + public function __construct( $module, $wikiId ) { + $this->module = $module; + $this->wikiId = $wikiId; + } + + /** + * Check the given PID to see if it is alive + * + * @param int $pid the process identifier to check + * + * @return boolean true if the process exist + */ + private function isPidAlive( $pid ) { + // Are we anything but Windows, i.e. some kind of Unix? + if ( strtoupper( substr( PHP_OS, 0, 3 ) ) !== 'WIN' ) { + return !!posix_getsid( $pid ); + } + // Welcome to Redmond + else { + $processes = explode( "\n", shell_exec( "tasklist.exe" ) ); + if ( $processes !== false && count( $processes ) > 0 ) { + foreach( $processes as $process ) { + if( strlen( $process ) > 0 + && ( strpos( "Image Name", $process ) === 0 + || strpos( "===", $process ) === 0 ) ) { + continue; + } + $matches = false; + preg_match( "/^(\D*)(\d+).*$/", $process, $matches ); + $processid = 0; + if ( $matches !== false && count ($matches) > 1 ) { + $processid = $matches[ 2 ]; + } + if ( $processid === $pid ) { + return true; + } + } + } + } + return false; + } + + /** + * Tries to allocate a PID based lock for the process, to avoid running more than one + * instance. + * + * Note that this method creates the file $pidfile if necessary. + * + * @since 0.5 + * + * @param boolean $force make the function skip the test and always grab the lock + * + * @return boolean true if we got the lock, i.e. if no instance is already running, + * or $force was set. + */ + public function getPidLock( $force = false ) { + $pidfile = $this->getStateFile(); + + if ( $force !== true ) { + // check if the process still exist and is alive + // XXX: there's a race condition here. + if ( file_exists( $pidfile ) ) { + $pid = file_get_contents( $pidfile ); + if ( self::isPidAlive( $pid ) === true ) { + return false; + } + } + } + + file_put_contents( $pidfile, getmypid() ); + + return true; + } + + /** + * Remove the pid lock. Assumes that we hold it. + * + * @return bool Success + */ + public function removePidLock() { + return unlink( $this->getStateFile() ); + } + + /** + * Generate a name and path for a file to store some kind of state in. + * + * @return string File path + */ + private function getStateFile() { + $wikiId = $this->wikiId; + if ( $wikiId === null ) { + $wikiId = wfWikiID(); + } + + // Build the filename + $pidfileName = preg_replace('/[^a-z0-9]/i', '', $this->module ) . '_' + . preg_replace('/[^a-z0-9]/i', '', $wikiId ) . '.pid'; + + $pidfile = '/var/run/' . $pidfileName; + + // Let's see if we can write to the file in /var/run + if ( is_writable( $pidfile ) + || ( is_dir( dirname( $pidfile ) ) + && ( is_writable( dirname( $pidfile ) ) ) ) ) { + $pidfile = '/var/run/' . $pidfileName; + } else { + // else use the temporary directory + $temp = str_replace( '\\', '/', sys_get_temp_dir() ); + $pidfile = $temp . '/' . $pidfileName; + } + + return $pidfile; + } + +} diff --git a/lib/includes/Utils.php b/lib/includes/Utils.php index 87d5c2c..978d72d 100644 --- a/lib/includes/Utils.php +++ b/lib/includes/Utils.php @@ -69,121 +69,4 @@ return $languageName; } - /** - * Check the given PID to see if it is alive - * - * @param int $pid the process identifier to check - * - * @return boolean true if the process exist - */ - private static function isPidAlive( $pid ) { - // Are we anything but Windows, i.e. some kind of Unix? - if ( strtoupper( substr( PHP_OS, 0, 3 ) ) !== 'WIN' ) { - return !!posix_getsid( $pid ); - } - // Welcome to Redmond - else { - $processes = explode( "\n", shell_exec( "tasklist.exe" ) ); - if ( $processes !== false && count( $processes ) > 0 ) { - foreach( $processes as $process ) { - if( strlen( $process ) > 0 - && ( strpos( "Image Name", $process ) === 0 - || strpos( "===", $process ) === 0 ) ) { - continue; - } - $matches = false; - preg_match( "/^(\D*)(\d+).*$/", $process, $matches ); - $processid = 0; - if ( $matches !== false && count ($matches) > 1 ) { - $processid = $matches[ 2 ]; - } - if ( $processid === $pid ) { - return true; - } - } - } - } - return false; - } - - /** - * Tries to allocate a PID based lock for the process, to avoid running more than one - * instance. - * - * Note that this method creates the file $pidfile if necessary. - * - * @since 0.3 - * - * @param string $pidfile the place where the pid is stored - * @param boolean $force make the function skip the test and always grab the lock - * - * @return boolean true if we got the lock, i.e. if no instance is already running, - * or $force was set. - */ - public static function getPidLock( $pidfile, $force = false ) { - if ( $force === true ) { - file_put_contents( $pidfile, getmypid() ); - return true; - } else { - // check if the process still exist and is alive - // XXX: there's a race condition here. - if ( file_exists( $pidfile ) ) { - $pid = file_get_contents( $pidfile ); - if ( self::isPidAlive( $pid ) === true ) { - return false; - } - } - file_put_contents( $pidfile, getmypid() ); - return true; - } - } - - /** - * Create a pid file name - * - * @since 0.3 - * - * @param string $module used as a basis for the file name. - * @param string $wikiId the wiki's id, used for per-wiki file names. Defaults to wfWikiID(). - * - * @return boolean true if the process exist - */ - public static function makePidFilename( $module, $wikiId = null ) { - return self::makeStateFilename( $module, '.pid', $wikiId ); - } - - /** - * Generate a name and path for a file to store some kind of state in. - * - * @param string $module used as a basis for the file name. - * @param string $suffix suffix, including file extension, appended without a separator. - * @param string $wikiId the wiki's id, used for per-wiki file names. Defaults to wfWikiID(). - * - * @return boolean true if the process exist - */ - private static function makeStateFilename( $module, $suffix, $wikiId = null ) { - if ( $wikiId === null ) { - $wikiId = wfWikiID(); - } - - // Build the filename - $pidfileName = preg_replace('/[^a-z0-9]/i', '', $module ) . '_' - . preg_replace('/[^a-z0-9]/i', '', $wikiId ) . $suffix; - - $pidfile = '/var/run/' . $pidfileName; - - // Let's see if we can write to the file in /var/run - if ( is_writable( $pidfile ) - || ( is_dir( dirname( $pidfile ) ) - && ( is_writable( dirname( $pidfile ) ) ) ) ) { - $pidfile = '/var/run/' . $pidfileName; - } else { - // else use the temporary directory - $temp = str_replace( '\\', '/', sys_get_temp_dir() ); - $pidfile = $temp . '/' . $pidfileName; - } - - return $pidfile; - } - } diff --git a/lib/tests/phpunit/PidLockTest.php b/lib/tests/phpunit/PidLockTest.php new file mode 100644 index 0000000..95d8988 --- /dev/null +++ b/lib/tests/phpunit/PidLockTest.php @@ -0,0 +1,44 @@ +<?php + +namespace Wikibase\Lib\Test; + +use PHPUnit_Framework_TestCase; +use Wikibase\Lib\PidLock; + +/** + * @covers Wikibase\Lib\PidLock + * + * @group WikibaseLib + * @group Wikibase + * + * @license GNU GPL v2+ + * @author Marius Hoch < [email protected] > + */ +class PidLockTest extends PHPUnit_Framework_TestCase { + + /** + * @dataProvider wikiIdProvider + */ + public function testPidLock( $wikiId ) { + $pidLock = new PidLock( 'PidLockTest', $wikiId ); + + $this->assertTrue( $pidLock->getPidLock(), 'Get an initial log' ); + $this->assertFalse( $pidLock->getPidLock(), 'Try to get the lock, although some already has it' ); + $this->assertTrue( $pidLock->getPidLock( true ), 'Force getting the lock' ); + + $this->assertTrue( $pidLock->removePidLock() ); + + // Make sure that the given file has actually been removed. + // unlink gives a warning if you use it a file that doesn't exist, suppress that + wfSuppressWarnings(); + $this->assertFalse( $pidLock->removePidLock() ); + wfRestoreWarnings(); + } + + public function wikiIdProvider() { + return array( + array( wfWikiID() ), + array( null ) + ); + } +} diff --git a/repo/maintenance/pruneChanges.php b/repo/maintenance/pruneChanges.php index e3facee..c973fb5 100644 --- a/repo/maintenance/pruneChanges.php +++ b/repo/maintenance/pruneChanges.php @@ -2,6 +2,7 @@ namespace Wikibase; use Maintenance; +use Wikibase\Lib\PidLock; /** * Prune the Wikibase changes table to a maximum number of entries. @@ -62,9 +63,9 @@ } $force = $this->getOption( 'force', false ); - $pidfile = Utils::makePidFilename( 'WBpruneChanges', wfWikiID() ); + $pidLock = new PidLock( 'WBpruneChanges', wfWikiID() ); - if ( !Utils::getPidLock( $pidfile, $force ) ) { + if ( !$pidLock->getPidLock( $force ) ) { $this->output( date( 'H:i:s' ) . " already running, exiting\n" ); exit( 5 ); } @@ -99,7 +100,7 @@ $deleted = $this->pruneChanges( $until ); $this->output( date( 'H:i:s' ) . " $deleted rows pruned.\n" ); - unlink( $pidfile ); // delete lockfile on normal exit + $pidLock->removePidLock(); // delete lockfile on normal exit } /** -- To view, visit https://gerrit.wikimedia.org/r/190463 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: merged Gerrit-Change-Id: Ifde86f49a3f536004c20417b5f2eceb1c9a70506 Gerrit-PatchSet: 2 Gerrit-Project: mediawiki/extensions/Wikibase Gerrit-Branch: master Gerrit-Owner: Hoo man <[email protected]> Gerrit-Reviewer: Adrian Lang <[email protected]> Gerrit-Reviewer: Aude <[email protected]> Gerrit-Reviewer: Daniel Kinzler <[email protected]> Gerrit-Reviewer: Hoo man <[email protected]> Gerrit-Reviewer: Jeroen De Dauw <[email protected]> Gerrit-Reviewer: Thiemo Mättig (WMDE) <[email protected]> Gerrit-Reviewer: jenkins-bot <> _______________________________________________ MediaWiki-commits mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
