From 32912b8c8d82b02c71a067de346e5990acc2f6dc Mon Sep 17 00:00:00 2001 From: Max Semenik Date: Fri, 6 Oct 2017 19:26:52 -0700 Subject: [PATCH] Introduce Shell\CommandFactory Bug: T177038 Change-Id: Id875e68ea1fa72b44a463f977ab52270fe1e7088 --- autoload.php | 1 + includes/MediaWikiServices.php | 9 +++ includes/ServiceWiring.php | 18 +++++ includes/shell/Command.php | 11 ++-- includes/shell/CommandFactory.php | 65 +++++++++++++++++++ includes/shell/Shell.php | 16 +---- .../includes/MediaWikiServicesTest.php | 4 +- .../includes/shell/CommandFactoryTest.php | 33 ++++++++++ 8 files changed, 138 insertions(+), 19 deletions(-) create mode 100644 includes/shell/CommandFactory.php create mode 100644 tests/phpunit/includes/shell/CommandFactoryTest.php diff --git a/autoload.php b/autoload.php index 83f25193ed..cf4a115a9b 100644 --- a/autoload.php +++ b/autoload.php @@ -930,6 +930,7 @@ $wgAutoloadLocalClasses = [ 'MediaWiki\\Session\\UserInfo' => __DIR__ . '/includes/session/UserInfo.php', 'MediaWiki\\ShellDisabledError' => __DIR__ . '/includes/exception/ShellDisabledError.php', 'MediaWiki\\Shell\\Command' => __DIR__ . '/includes/shell/Command.php', + 'MediaWiki\\Shell\\CommandFactory' => __DIR__ . '/includes/shell/CommandFactory.php', 'MediaWiki\\Shell\\Result' => __DIR__ . '/includes/shell/Result.php', 'MediaWiki\\Shell\\Shell' => __DIR__ . '/includes/shell/Shell.php', 'MediaWiki\\Site\\MediaWikiPageNameNormalizer' => __DIR__ . '/includes/site/MediaWikiPageNameNormalizer.php', diff --git a/includes/MediaWikiServices.php b/includes/MediaWikiServices.php index 84fc959fa7..0d010b49d1 100644 --- a/includes/MediaWikiServices.php +++ b/includes/MediaWikiServices.php @@ -10,6 +10,7 @@ use GenderCache; use GlobalVarConfig; use Hooks; use IBufferingStatsdDataFactory; +use MediaWiki\Shell\CommandFactory; use Wikimedia\Rdbms\LBFactory; use LinkCache; use Wikimedia\Rdbms\LoadBalancer; @@ -681,6 +682,14 @@ class MediaWikiServices extends ServiceContainer { return $this->getService( 'ReadOnlyMode' ); } + /** + * @since 1.30 + * @return CommandFactory + */ + public function getShellCommandFactory() { + return $this->getService( 'ShellCommandFactory' ); + } + /////////////////////////////////////////////////////////////////////////// // NOTE: When adding a service getter here, don't forget to add a test // case for it in MediaWikiServicesTest::provideGetters() and in diff --git a/includes/ServiceWiring.php b/includes/ServiceWiring.php index d048007c3a..75ce8eca94 100644 --- a/includes/ServiceWiring.php +++ b/includes/ServiceWiring.php @@ -41,6 +41,7 @@ use MediaWiki\Interwiki\ClassicInterwikiLookup; use MediaWiki\Linker\LinkRendererFactory; use MediaWiki\Logger\LoggerFactory; use MediaWiki\MediaWikiServices; +use MediaWiki\Shell\CommandFactory; return [ 'DBLoadBalancerFactory' => function ( MediaWikiServices $services ) { @@ -428,6 +429,23 @@ return [ ); }, + 'ShellCommandFactory' => function ( MediaWikiServices $services ) { + $config = $services->getMainConfig(); + + $limits = [ + 'time' => $config->get( 'MaxShellTime' ), + 'walltime' => $config->get( 'MaxShellWallClockTime' ), + 'memory' => $config->get( 'MaxShellMemory' ), + 'filesize' => $config->get( 'MaxShellFileSize' ), + ]; + $cgroup = $config->get( 'ShellCgroup' ); + + $factory = new CommandFactory( $limits, $cgroup ); + $factory->setLogger( LoggerFactory::getInstance( 'exec' ) ); + + return $factory; + }, + /////////////////////////////////////////////////////////////////////////// // NOTE: When adding a service here, don't forget to add a getter function // in the MediaWikiServices class. The convenience getter should just call diff --git a/includes/shell/Command.php b/includes/shell/Command.php index a16f4af32f..bd44ef8937 100644 --- a/includes/shell/Command.php +++ b/includes/shell/Command.php @@ -63,7 +63,7 @@ class Command { private $everExecuted = false; /** @var string|false */ - private $cGroup = false; + private $cgroup = false; /** * Constructor. Don't call directly, instead use Shell::command() @@ -133,7 +133,8 @@ class Command { /** * Sets execution limits * - * @param array $limits Optional array with limits(filesize, memory, time, walltime). + * @param array $limits Associative array of limits. Keys (all optional): + * filesize (for ulimit -f), memory, time, walltime. * @return $this */ public function limits( array $limits ) { @@ -187,11 +188,11 @@ class Command { /** * Sets cgroup for this command * - * @param string|false $cgroup + * @param string|false $cgroup Absolute file path to the cgroup, or false to not use a cgroup * @return $this */ public function cgroup( $cgroup ) { - $this->cGroup = $cgroup; + $this->cgroup = $cgroup; return $this; } @@ -243,7 +244,7 @@ class Command { escapeshellarg( "MW_INCLUDE_STDERR=" . ( $this->useStderr ? '1' : '' ) . ';' . "MW_CPU_LIMIT=$time; " . - 'MW_CGROUP=' . escapeshellarg( $this->cGroup ) . '; ' . + 'MW_CGROUP=' . escapeshellarg( $this->cgroup ) . '; ' . "MW_MEM_LIMIT=$mem; " . "MW_FILE_SIZE_LIMIT=$filesize; " . "MW_WALL_CLOCK_LIMIT=$wallTime; " . diff --git a/includes/shell/CommandFactory.php b/includes/shell/CommandFactory.php new file mode 100644 index 0000000000..c0b8f899ed --- /dev/null +++ b/includes/shell/CommandFactory.php @@ -0,0 +1,65 @@ +limits = $limits; + $this->cgroup = $cgroup; + $this->setLogger( new NullLogger() ); + } + + /** + * Instantiates a new Command + * + * @return Command + */ + public function create() { + $command = new Command(); + $command->setLogger( $this->logger ); + + return $command + ->limits( $this->limits ) + ->cgroup( $this->cgroup ); + } +} diff --git a/includes/shell/Shell.php b/includes/shell/Shell.php index e21d762d20..a660a22314 100644 --- a/includes/shell/Shell.php +++ b/includes/shell/Shell.php @@ -22,7 +22,6 @@ namespace MediaWiki\Shell; -use MediaWiki\Logger\LoggerFactory; use MediaWiki\MediaWikiServices; /** @@ -57,18 +56,9 @@ class Shell { // treat it as a list of arguments $args = reset( $args ); } - $command = new Command(); - $config = MediaWikiServices::getInstance()->getMainConfig(); - - $limits = [ - 'time' => $config->get( 'MaxShellTime' ), - 'walltime' => $config->get( 'MaxShellWallClockTime' ), - 'memory' => $config->get( 'MaxShellMemory' ), - 'filesize' => $config->get( 'MaxShellFileSize' ), - ]; - $command->limits( $limits ); - $command->cgroup( $config->get( 'ShellCgroup' ) ); - $command->setLogger( LoggerFactory::getInstance( 'exec' ) ); + $command = MediaWikiServices::getInstance() + ->getShellCommandFactory() + ->create(); return $command->params( $args ); } diff --git a/tests/phpunit/includes/MediaWikiServicesTest.php b/tests/phpunit/includes/MediaWikiServicesTest.php index f6bc2093c9..a5c468806f 100644 --- a/tests/phpunit/includes/MediaWikiServicesTest.php +++ b/tests/phpunit/includes/MediaWikiServicesTest.php @@ -6,6 +6,7 @@ use MediaWiki\MediaWikiServices; use MediaWiki\Services\DestructibleService; use MediaWiki\Services\SalvageableService; use MediaWiki\Services\ServiceDisabledException; +use MediaWiki\Shell\CommandFactory; /** * @covers MediaWiki\MediaWikiServices @@ -328,7 +329,8 @@ class MediaWikiServicesTest extends MediaWikiTestCase { 'MainObjectStash' => [ 'MainObjectStash', BagOStuff::class ], 'MainWANObjectCache' => [ 'MainWANObjectCache', WANObjectCache::class ], 'LocalServerObjectCache' => [ 'LocalServerObjectCache', BagOStuff::class ], - 'VirtualRESTServiceClient' => [ 'VirtualRESTServiceClient', VirtualRESTServiceClient::class ] + 'VirtualRESTServiceClient' => [ 'VirtualRESTServiceClient', VirtualRESTServiceClient::class ], + 'ShellCommandFactory' => [ 'ShellCommandFactory', CommandFactory::class ], ]; } diff --git a/tests/phpunit/includes/shell/CommandFactoryTest.php b/tests/phpunit/includes/shell/CommandFactoryTest.php new file mode 100644 index 0000000000..aacfd43c16 --- /dev/null +++ b/tests/phpunit/includes/shell/CommandFactoryTest.php @@ -0,0 +1,33 @@ + 1000, + 'memory' => 1000, + 'time' => 30, + 'walltime' => 40, + ]; + + $factory = new CommandFactory( $limits, $cgroup ); + $factory->setLogger( $logger ); + $command = $factory->create(); + + $wrapper = TestingAccessWrapper::newFromObject( $command ); + $this->assertSame( $logger, $wrapper->logger ); + $this->assertSame( $cgroup, $wrapper->cgroup ); + $this->assertSame( $limits, $wrapper->limits ); + } +} -- 2.20.1