From 3780fdb214ac8c192fb38bac00134a2c5f561453 Mon Sep 17 00:00:00 2001 From: Romain Neutron Date: Sat, 6 Apr 2013 20:51:55 +0200 Subject: [PATCH 1/7] Fix Process timeout --- src/Symfony/Component/Process/Process.php | 43 +++++++++++++++---- .../Component/Process/ProcessBuilder.php | 6 +-- .../Process/Tests/AbstractProcessTest.php | 42 ++++++++++++++++++ 3 files changed, 80 insertions(+), 11 deletions(-) diff --git a/src/Symfony/Component/Process/Process.php b/src/Symfony/Component/Process/Process.php index f8a3293b12..a4ee6a760a 100644 --- a/src/Symfony/Component/Process/Process.php +++ b/src/Symfony/Component/Process/Process.php @@ -34,10 +34,14 @@ class Process const STDOUT = 1; const STDERR = 2; + // Timeout Precision in seconds. + CONST TIMEOUT_PRECISION = 0.2; + private $commandline; private $cwd; private $env; private $stdin; + private $starttime; private $timeout; private $options; private $exitcode; @@ -211,6 +215,7 @@ class Process throw new \RuntimeException('Process is already running'); } + $this->starttime = microtime(true); $this->stdout = ''; $this->stderr = ''; $callback = $this->buildCallback($callback); @@ -285,7 +290,7 @@ class Process $w = $writePipes; $e = null; - $n = @stream_select($r, $w, $e, $this->timeout); + $n = @stream_select($r, $w, $e, 0, static::TIMEOUT_PRECISION * 1E6); if (false === $n) { break; @@ -318,6 +323,8 @@ class Process unset($this->pipes[$type]); } } + + $this->checkTimeout(); } $this->updateStatus(); @@ -345,13 +352,15 @@ class Process if (defined('PHP_WINDOWS_VERSION_BUILD') && $this->fileHandles) { $this->processFileHandles($callback, !$this->pipes); } + $this->checkTimeout(); if ($this->pipes) { $r = $this->pipes; $w = null; $e = null; - if (false === $n = @stream_select($r, $w, $e, $this->timeout)) { + // let's have a look if something changed in streams + if (false === $n = @stream_select($r, $w, $e, 0, static::TIMEOUT_PRECISION * 1E6)) { $lastError = error_get_last(); // stream_select returns false when the `select` system call is interrupted by an incoming signal @@ -361,10 +370,11 @@ class Process continue; } - if (0 === $n) { - proc_terminate($this->process); - throw new \RuntimeException('The process timed out.'); + + // nothing has changed + if (0 === $n) { + continue; } foreach ($r as $pipe) { @@ -675,7 +685,7 @@ class Process * * To disable the timeout, set this value to null. * - * @param integer|null $timeout The timeout in seconds + * @param float|null $timeout The timeout in seconds * * @throws \InvalidArgumentException if the timeout is negative */ @@ -687,10 +697,10 @@ class Process return; } - $timeout = (integer) $timeout; + $timeout = (float) $timeout; if ($timeout < 0) { - throw new \InvalidArgumentException('The timeout value must be a valid positive integer.'); + throw new \InvalidArgumentException('The timeout value must be a valid positive integer or float number.'); } $this->timeout = $timeout; @@ -829,6 +839,23 @@ class Process $this->enhanceSigchildCompatibility = (Boolean) $enhance; } + /** + * Performs a check between the timeout definition and the time the process + * started + * + * In case you run a background process (with the start method), you should + * trigger this method regularly to ensure the process timeout + * + * @throws RuntimeException In case the timeout was reached + */ + public function checkTimeout() + { + if (0 < $this->timeout && $this->timeout < (microtime(true) - $this->starttime)) { + $this->stop(0); + throw new RuntimeException('Process timed-out.'); + } + } + /** * Builds up the callback used by wait(). * diff --git a/src/Symfony/Component/Process/ProcessBuilder.php b/src/Symfony/Component/Process/ProcessBuilder.php index 2ffb3af5ff..3d8b687fec 100644 --- a/src/Symfony/Component/Process/ProcessBuilder.php +++ b/src/Symfony/Component/Process/ProcessBuilder.php @@ -86,7 +86,7 @@ class ProcessBuilder * * To disable the timeout, set this value to null. * - * @param integer|null + * @param float|null */ public function setTimeout($timeout) { @@ -96,10 +96,10 @@ class ProcessBuilder return $this; } - $timeout = (integer) $timeout; + $timeout = (float) $timeout; if ($timeout < 0) { - throw new \InvalidArgumentException('The timeout value must be a valid positive integer.'); + throw new \InvalidArgumentException('The timeout value must be a valid positive integer or float number.'); } $this->timeout = $timeout; diff --git a/src/Symfony/Component/Process/Tests/AbstractProcessTest.php b/src/Symfony/Component/Process/Tests/AbstractProcessTest.php index 51d51f40b8..10dea51098 100644 --- a/src/Symfony/Component/Process/Tests/AbstractProcessTest.php +++ b/src/Symfony/Component/Process/Tests/AbstractProcessTest.php @@ -12,6 +12,7 @@ namespace Symfony\Component\Process\Tests; use Symfony\Component\Process\Process; +use Symfony\Component\Process\Exception\RuntimeException; /** * @author Robert Schönthal @@ -255,6 +256,47 @@ abstract class AbstractProcessTest extends \PHPUnit_Framework_TestCase // PHP will deadlock when it tries to cleanup $process } + public function testRunProcessWithTimeout() + { + $timeout = 0.5; + $process = $this->getProcess('sleep 3'); + $process->setTimeout($timeout); + $start = microtime(true); + try { + $process->run(); + $this->fail('A RuntimeException should have been raised'); + } catch (RuntimeException $e) { + + } + $duration = microtime(true) - $start; + + $this->assertLessThan($timeout + Process::TIMEOUT_PRECISION, $duration); + } + + public function testCheckTimeoutOnStartedProcess() + { + $timeout = 0.5; + $precision = 100000; + $process = $this->getProcess('sleep 3'); + $process->setTimeout($timeout); + $start = microtime(true); + + $process->start(); + + try { + while ($process->isRunning()) { + $process->checkTimeout(); + usleep($precision); + } + $this->fail('A RuntimeException should have been raised'); + } catch (RuntimeException $e) { + + } + $duration = microtime(true) - $start; + + $this->assertLessThan($timeout + $precision, $duration); + } + public function responsesCodeProvider() { return array( From bf4a9b083c0df4d797a0fd5489b9cf8cb9901baf Mon Sep 17 00:00:00 2001 From: Romain Neutron Date: Sun, 7 Apr 2013 14:38:17 +0200 Subject: [PATCH 2/7] Round stream_select fifth argument up. - This argument must be an integer - An argument equal to 0 should be avoided as it consumes too much CPU time --- src/Symfony/Component/Process/Process.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Symfony/Component/Process/Process.php b/src/Symfony/Component/Process/Process.php index a4ee6a760a..56ff6d1ecc 100644 --- a/src/Symfony/Component/Process/Process.php +++ b/src/Symfony/Component/Process/Process.php @@ -290,7 +290,7 @@ class Process $w = $writePipes; $e = null; - $n = @stream_select($r, $w, $e, 0, static::TIMEOUT_PRECISION * 1E6); + $n = @stream_select($r, $w, $e, 0, ceil(static::TIMEOUT_PRECISION * 1E6)); if (false === $n) { break; @@ -360,7 +360,7 @@ class Process $e = null; // let's have a look if something changed in streams - if (false === $n = @stream_select($r, $w, $e, 0, static::TIMEOUT_PRECISION * 1E6)) { + if (false === $n = @stream_select($r, $w, $e, 0, ceil(static::TIMEOUT_PRECISION * 1E6))) { $lastError = error_get_last(); // stream_select returns false when the `select` system call is interrupted by an incoming signal From 783ae3adffea7d12963156c4d9befb5138886c16 Mon Sep 17 00:00:00 2001 From: Fabien Potencier Date: Sun, 7 Apr 2013 16:10:23 +0200 Subject: [PATCH 3/7] fixed CS --- src/Symfony/Component/Process/Process.php | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/Symfony/Component/Process/Process.php b/src/Symfony/Component/Process/Process.php index 56ff6d1ecc..7cbaf700b5 100644 --- a/src/Symfony/Component/Process/Process.php +++ b/src/Symfony/Component/Process/Process.php @@ -35,7 +35,7 @@ class Process const STDERR = 2; // Timeout Precision in seconds. - CONST TIMEOUT_PRECISION = 0.2; + const TIMEOUT_PRECISION = 0.2; private $commandline; private $cwd; @@ -850,9 +850,10 @@ class Process */ public function checkTimeout() { - if (0 < $this->timeout && $this->timeout < (microtime(true) - $this->starttime)) { + if (0 < $this->timeout && $this->timeout < microtime(true) - $this->starttime) { $this->stop(0); - throw new RuntimeException('Process timed-out.'); + + throw new RuntimeException('The process timed-out.'); } } From bec8ff16e8ec01e344811ec93d95ef0703b9ecfa Mon Sep 17 00:00:00 2001 From: Romain Neutron Date: Sat, 6 Apr 2013 19:59:02 +0200 Subject: [PATCH 4/7] Fix timeout in Process::stop method - The timeout is ten times more than set. - The timeout does not occurs, it is actually blocking until the process dies. --- src/Symfony/Component/Process/Process.php | 6 ++- .../Process/Tests/AbstractProcessTest.php | 25 +++++++++++++ .../Process/Tests/NonStopableProcess.php | 37 +++++++++++++++++++ 3 files changed, 67 insertions(+), 1 deletion(-) create mode 100644 src/Symfony/Component/Process/Tests/NonStopableProcess.php diff --git a/src/Symfony/Component/Process/Process.php b/src/Symfony/Component/Process/Process.php index f8a3293b12..7d2535095e 100644 --- a/src/Symfony/Component/Process/Process.php +++ b/src/Symfony/Component/Process/Process.php @@ -591,7 +591,7 @@ class Process */ public function stop($timeout=10) { - $timeoutMicro = (int) $timeout*10E6; + $timeoutMicro = (int) $timeout*1E6; if ($this->isRunning()) { proc_terminate($this->process); $time = 0; @@ -600,6 +600,10 @@ class Process usleep(1000); } + if (!defined('PHP_WINDOWS_VERSION_BUILD') && $this->isRunning()) { + proc_terminate($this->process, SIGKILL); + } + foreach ($this->pipes as $pipe) { fclose($pipe); } diff --git a/src/Symfony/Component/Process/Tests/AbstractProcessTest.php b/src/Symfony/Component/Process/Tests/AbstractProcessTest.php index 51d51f40b8..7f622d38ac 100644 --- a/src/Symfony/Component/Process/Tests/AbstractProcessTest.php +++ b/src/Symfony/Component/Process/Tests/AbstractProcessTest.php @@ -44,6 +44,31 @@ abstract class AbstractProcessTest extends \PHPUnit_Framework_TestCase $this->assertNull($p->getTimeout()); } + public function testStopWithTimeoutIsActuallyWorking() + { + if (defined('PHP_WINDOWS_VERSION_BUILD')) { + $this->markTestSkipped('Stop with timeout does not work on windows, it requires posix signals'); + } + if (!function_exists('pcntl_signal')) { + $this->markTestSkipped('This test require pcntl_signal function'); + } + + // exec is mandatory here since we send a signal to the process + // see https://github.com/symfony/symfony/issues/5030 about prepending + // command with exec + $p = $this->getProcess('exec php '.__DIR__.'/NonStopableProcess.php 3'); + $p->start(); + usleep(100000); + $start = microtime(true); + $p->stop(1.1); + while ($p->isRunning()) { + usleep(1000); + } + $duration = microtime(true) - $start; + + $this->assertLessThan(1.3, $duration); + } + /** * tests results from sub processes * diff --git a/src/Symfony/Component/Process/Tests/NonStopableProcess.php b/src/Symfony/Component/Process/Tests/NonStopableProcess.php new file mode 100644 index 0000000000..a4db838256 --- /dev/null +++ b/src/Symfony/Component/Process/Tests/NonStopableProcess.php @@ -0,0 +1,37 @@ + (microtime(true) - $start)) { + usleep(1000); +} From 9d71ebe8e2e809da93ea4555759846c7676d9d08 Mon Sep 17 00:00:00 2001 From: Jordi Boggiano Date: Sun, 7 Apr 2013 16:34:20 +0200 Subject: [PATCH 5/7] Fix autocompletion of command names when namespaces conflict --- src/Symfony/Component/Console/Application.php | 42 +++++++++---------- .../Console/Tests/ApplicationTest.php | 10 +++++ 2 files changed, 31 insertions(+), 21 deletions(-) diff --git a/src/Symfony/Component/Console/Application.php b/src/Symfony/Component/Console/Application.php index af80091613..7f7da75858 100644 --- a/src/Symfony/Component/Console/Application.php +++ b/src/Symfony/Component/Console/Application.php @@ -478,20 +478,24 @@ class Application */ public function findNamespace($namespace) { - $allNamespaces = array(); - foreach ($this->getNamespaces() as $n) { - $allNamespaces[$n] = explode(':', $n); - } - - $found = array(); + $allNamespaces = $this->getNamespaces(); + $found = ''; foreach (explode(':', $namespace) as $i => $part) { - $abbrevs = static::getAbbreviations(array_unique(array_values(array_filter(array_map(function ($p) use ($i) { return isset($p[$i]) ? $p[$i] : ''; }, $allNamespaces))))); + // select sub-namespaces matching the current namespace we found + $namespaces = array(); + foreach ($allNamespaces as $n) { + if ('' === $found || 0 === strpos($n, $found)) { + $namespaces[$n] = explode(':', $n); + } + } + + $abbrevs = static::getAbbreviations(array_unique(array_values(array_filter(array_map(function ($p) use ($i) { return isset($p[$i]) ? $p[$i] : ''; }, $namespaces))))); if (!isset($abbrevs[$part])) { $message = sprintf('There are no commands defined in the "%s" namespace.', $namespace); if (1 <= $i) { - $part = implode(':', $found).':'.$part; + $part = $found.':'.$part; } if ($alternatives = $this->findAlternativeNamespace($part, $abbrevs)) { @@ -507,14 +511,19 @@ class Application throw new \InvalidArgumentException($message); } + // there are multiple matches, but $part is an exact match of one of them so we select it + if (in_array($part, $abbrevs[$part])) { + $abbrevs[$part] = array($part); + } + if (count($abbrevs[$part]) > 1) { throw new \InvalidArgumentException(sprintf('The namespace "%s" is ambiguous (%s).', $namespace, $this->getAbbreviationSuggestions($abbrevs[$part]))); } - $found[] = $abbrevs[$part][0]; + $found .= $found ? ':' . $abbrevs[$part][0] : $abbrevs[$part][0]; } - return implode(':', $found); + return $found; } /** @@ -637,21 +646,12 @@ class Application { $abbrevs = array(); foreach ($names as $name) { - for ($len = strlen($name) - 1; $len > 0; --$len) { + for ($len = strlen($name); $len > 0; --$len) { $abbrev = substr($name, 0, $len); - if (!isset($abbrevs[$abbrev])) { - $abbrevs[$abbrev] = array($name); - } else { - $abbrevs[$abbrev][] = $name; - } + $abbrevs[$abbrev][] = $name; } } - // Non-abbreviations always get entered, even if they aren't unique - foreach ($names as $name) { - $abbrevs[$name] = array($name); - } - return $abbrevs; } diff --git a/src/Symfony/Component/Console/Tests/ApplicationTest.php b/src/Symfony/Component/Console/Tests/ApplicationTest.php index 7a0a4895cb..bb4e11e6a2 100644 --- a/src/Symfony/Component/Console/Tests/ApplicationTest.php +++ b/src/Symfony/Component/Console/Tests/ApplicationTest.php @@ -342,6 +342,16 @@ class ApplicationTest extends \PHPUnit_Framework_TestCase } } + public function testFindNamespaceDoesNotFailOnDeepSimilarNamespaces() + { + $application = $this->getMock('Symfony\Component\Console\Application', array('getNamespaces')); + $application->expects($this->once()) + ->method('getNamespaces') + ->will($this->returnValue(array('foo:sublong', 'bar:sub'))); + + $this->assertEquals('foo:sublong', $application->findNamespace('f:sub')); + } + public function testSetCatchExceptions() { $application = $this->getMock('Symfony\Component\Console\Application', array('getTerminalWidth')); From a3826ab4f4f5660c9e26c830f4bb35b5a0d067c7 Mon Sep 17 00:00:00 2001 From: Roland Franssen Date: Mon, 1 Apr 2013 13:19:43 +0200 Subject: [PATCH 6/7] #7531: [HttpKernel][Config] FileLocator adds NULL as global resource path --- .../HttpKernel/Config/FileLocator.php | 10 +-- .../Tests/Config/FileLocatorTest.php | 61 +++++++++++++++++++ 2 files changed, 67 insertions(+), 4 deletions(-) mode change 100644 => 100755 src/Symfony/Component/HttpKernel/Config/FileLocator.php create mode 100755 src/Symfony/Component/HttpKernel/Tests/Config/FileLocatorTest.php diff --git a/src/Symfony/Component/HttpKernel/Config/FileLocator.php b/src/Symfony/Component/HttpKernel/Config/FileLocator.php old mode 100644 new mode 100755 index d241b9da19..50c7d07a1c --- a/src/Symfony/Component/HttpKernel/Config/FileLocator.php +++ b/src/Symfony/Component/HttpKernel/Config/FileLocator.php @@ -28,14 +28,16 @@ class FileLocator extends BaseFileLocator * Constructor. * * @param KernelInterface $kernel A KernelInterface instance - * @param string $path The path the global resource directory - * @param string|array $paths A path or an array of paths where to look for resources + * @param null|string $path The path the global resource directory + * @param array $paths An array of paths where to look for resources */ public function __construct(KernelInterface $kernel, $path = null, array $paths = array()) { $this->kernel = $kernel; - $this->path = $path; - $paths[] = $path; + if(null !== $path) { + $this->path = $path; + $paths[] = $path; + } parent::__construct($paths); } diff --git a/src/Symfony/Component/HttpKernel/Tests/Config/FileLocatorTest.php b/src/Symfony/Component/HttpKernel/Tests/Config/FileLocatorTest.php new file mode 100755 index 0000000000..336a4bcff2 --- /dev/null +++ b/src/Symfony/Component/HttpKernel/Tests/Config/FileLocatorTest.php @@ -0,0 +1,61 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\HttpKernel\Tests\Config; + +use Symfony\Component\HttpKernel\Config\FileLocator; +use Symfony\Component\HttpKernel\KernelInterface; + +class FileLocatorTest extends \PHPUnit_Framework_TestCase +{ + + /** @var KernelInterface */ + private $kernel; + + public function setUp() + { + $this->kernel = $this->getMock('Symfony\Component\HttpKernel\KernelInterface'); + } + + public function tearDown() + { + $this->kernel = null; + } + + public function testLocate() + { + $this->kernel + ->expects($this->atLeastOnce()) + ->method('locateResource') + ->with('@BundleName/some/path', null, true) + ->will($this->returnValue('/bundle-name/some/path')); + $locator = new FileLocator($this->kernel); + $this->assertEquals('/bundle-name/some/path', $locator->locate('@BundleName/some/path')); + + $this->kernel + ->expects($this->never()) + ->method('locateResource'); + $this->setExpectedException('LogicException'); + $locator->locate('/some/path'); + } + + public function testLocateWithGlobalResourcePath() + { + $this->kernel + ->expects($this->atLeastOnce()) + ->method('locateResource') + ->with('@BundleName/some/path', '/global/resource/path', false); + + $locator = new FileLocator($this->kernel, '/global/resource/path'); + $locator->locate('@BundleName/some/path', null, false); + } + +} From 12b7073607cb413a1e4cf7ee9bc7180f09fe96f7 Mon Sep 17 00:00:00 2001 From: Fabien Potencier Date: Sun, 7 Apr 2013 17:51:26 +0200 Subject: [PATCH 7/7] [HttpKernel] tweaked previous merge --- .../HttpKernel/Config/FileLocator.php | 2 +- .../Tests/Config/FileLocatorTest.php | 28 +++++-------------- 2 files changed, 8 insertions(+), 22 deletions(-) mode change 100755 => 100644 src/Symfony/Component/HttpKernel/Config/FileLocator.php diff --git a/src/Symfony/Component/HttpKernel/Config/FileLocator.php b/src/Symfony/Component/HttpKernel/Config/FileLocator.php old mode 100755 new mode 100644 index 50c7d07a1c..47b543c15e --- a/src/Symfony/Component/HttpKernel/Config/FileLocator.php +++ b/src/Symfony/Component/HttpKernel/Config/FileLocator.php @@ -34,7 +34,7 @@ class FileLocator extends BaseFileLocator public function __construct(KernelInterface $kernel, $path = null, array $paths = array()) { $this->kernel = $kernel; - if(null !== $path) { + if (null !== $path) { $this->path = $path; $paths[] = $path; } diff --git a/src/Symfony/Component/HttpKernel/Tests/Config/FileLocatorTest.php b/src/Symfony/Component/HttpKernel/Tests/Config/FileLocatorTest.php index 336a4bcff2..be59486269 100755 --- a/src/Symfony/Component/HttpKernel/Tests/Config/FileLocatorTest.php +++ b/src/Symfony/Component/HttpKernel/Tests/Config/FileLocatorTest.php @@ -12,35 +12,21 @@ namespace Symfony\Component\HttpKernel\Tests\Config; use Symfony\Component\HttpKernel\Config\FileLocator; -use Symfony\Component\HttpKernel\KernelInterface; class FileLocatorTest extends \PHPUnit_Framework_TestCase { - - /** @var KernelInterface */ - private $kernel; - - public function setUp() - { - $this->kernel = $this->getMock('Symfony\Component\HttpKernel\KernelInterface'); - } - - public function tearDown() - { - $this->kernel = null; - } - public function testLocate() { - $this->kernel + $kernel = $this->getMock('Symfony\Component\HttpKernel\KernelInterface'); + $kernel ->expects($this->atLeastOnce()) ->method('locateResource') ->with('@BundleName/some/path', null, true) ->will($this->returnValue('/bundle-name/some/path')); - $locator = new FileLocator($this->kernel); + $locator = new FileLocator($kernel); $this->assertEquals('/bundle-name/some/path', $locator->locate('@BundleName/some/path')); - $this->kernel + $kernel ->expects($this->never()) ->method('locateResource'); $this->setExpectedException('LogicException'); @@ -49,13 +35,13 @@ class FileLocatorTest extends \PHPUnit_Framework_TestCase public function testLocateWithGlobalResourcePath() { - $this->kernel + $kernel = $this->getMock('Symfony\Component\HttpKernel\KernelInterface'); + $kernel ->expects($this->atLeastOnce()) ->method('locateResource') ->with('@BundleName/some/path', '/global/resource/path', false); - $locator = new FileLocator($this->kernel, '/global/resource/path'); + $locator = new FileLocator($kernel, '/global/resource/path'); $locator->locate('@BundleName/some/path', null, false); } - }