Skip to content

[Process] Enhance compatiblity with --enable-sigchild #16915

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 10, 2015
Merged

[Process] Enhance compatiblity with --enable-sigchild #16915

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ addons:
cache:
directories:
- .phpunit
- php-5.3.9

matrix:
include:
Expand All @@ -32,6 +33,7 @@ env:

before_install:
- if [[ "$deps" = "no" ]] && [[ "$TRAVIS_PHP_VERSION" =~ 5.[45] ]] && [[ "$TRAVIS_PULL_REQUEST" != "false" ]]; then export deps=skip; fi;
- if [[ $deps = no && $TRAVIS_PHP_VERSION = 5.3 && ! -d php-5.3.9/sapi ]]; then wget http://museum.php.net/php5/php-5.3.9.tar.bz2; tar -xjf php-5.3.9.tar.bz2; (cd php-5.3.9; ./configure --enable-sigchild --enable-pcntl; make -j2); fi;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume that the version will have to change for the 3.0 branch ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I propose 5.5.9 there

- if [[ "$TRAVIS_PHP_VERSION" != "hhvm" ]]; then INI_FILE=~/.phpenv/versions/$(phpenv version-name)/etc/conf.d/travis.ini; else INI_FILE=/etc/hhvm/php.ini; fi;
- echo "memory_limit = -1" >> $INI_FILE
- echo "session.gc_probability = 0" >> $INI_FILE
Expand All @@ -54,6 +56,7 @@ install:
script:
- if [ "$deps" = "no" ]; then echo "$COMPONENTS" | parallel --gnu '$PHPUNIT --exclude-group tty,benchmark,intl-data {}'; fi;
- if [ "$deps" = "no" ]; then echo -e "\\nRunning tests requiring tty"; $PHPUNIT --group tty; fi;
- if [[ $deps = no && $TRAVIS_PHP_VERSION = 5.3 ]]; then echo -e "1\\n0" | parallel --gnu 'echo -e "\\nPHP --enable-sigchild enhanced={}" && ENHANCE_SIGCHLD={} php-5.3.9/sapi/cli/php .phpunit/phpunit-4.8/phpunit --colors=always src/Symfony/Component/Process/'; fi;
- if [ "$deps" = "high" ]; then echo "$COMPONENTS" | parallel --gnu -j10% 'cd {}; composer --prefer-source update; $PHPUNIT --exclude-group tty,benchmark,intl-data'; fi;
- if [ "$deps" = "low" ]; then echo "$COMPONENTS" | parallel --gnu -j10% 'cd {}; composer --prefer-source --prefer-lowest --prefer-stable update; $PHPUNIT --exclude-group tty,benchmark,intl-data'; fi;
- if [ "$deps" = "skip" ]; then echo 'This matrix line is skipped for pull requests.'; fi;
118 changes: 63 additions & 55 deletions src/Symfony/Component/Process/Process.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ class Process
private $timeout;
private $options;
private $exitcode;
private $fallbackExitcode;
private $fallbackStatus = array();
private $processInformation;
private $stdout;
private $stderr;
Expand All @@ -65,6 +65,14 @@ class Process
private $latestSignal;

private static $sigchild;
private static $posixSignals = array(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for the reader: this is the list of signals that are standardized per posix (except 9, SIGKILL, which can't be caught, thus forwarded)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest adding comment near each of them with their name, to make it more maintainable (similar to comments you added in tests when using such value)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea, added

1 => 1, // SIGHUP
2 => 2, // SIGINT
3 => 3, // SIGQUIT
6 => 6, // SIGABRT
14 => 14, // SIGALRM
15 => 15, // SIGTERM
);

/**
* Exit codes translation table.
Expand Down Expand Up @@ -339,17 +347,9 @@ public function wait($callback = null)
* Returns the Pid (process identifier), if applicable.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if the sigchild compat enhancements are not applied ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then it works :) Because the issue was the shell wrapper hack, and the new one is now able to report the correct pid. The exception should not have been thrown when in non-enhanced-sigchild mode.

*
* @return int|null The process id if running, null otherwise
*
* @throws RuntimeException In case --enable-sigchild is activated
*/
public function getPid()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are you sure this should be removed ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes: isRunning() calls it also

{
if ($this->isSigchildEnabled()) {
throw new RuntimeException('This PHP has been compiled with --enable-sigchild. The process identifier can not be retrieved.');
}

$this->updateStatus(false);

return $this->isRunning() ? $this->processInformation['pid'] : null;
}

Expand All @@ -361,7 +361,7 @@ public function getPid()
* @return Process
*
* @throws LogicException In case the process is not running
* @throws RuntimeException In case --enable-sigchild is activated
* @throws RuntimeException In case --enable-sigchild is activated and the process can't be killed
* @throws RuntimeException In case of failure
*/
public function signal($signal)
Expand Down Expand Up @@ -467,7 +467,9 @@ public function getIncrementalErrorOutput()
*/
public function getExitCode()
{
if ($this->isSigchildEnabled() && !$this->enhanceSigchildCompatibility) {
if (!$this->enhanceSigchildCompatibility && $this->isSigchildEnabled()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if it is not enhanced ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nicolas-grekas you haven't answered here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it throws again now

$this->stop(0);

throw new RuntimeException('This PHP has been compiled with --enable-sigchild. You must use setEnhanceSigchildCompatibility() to use this method.');
}

Expand All @@ -484,8 +486,6 @@ public function getExitCode()
*
* @return null|string A string representation for the exit status code, null if the Process is not terminated.
*
* @throws RuntimeException In case --enable-sigchild is activated and the sigchild compatibility mode is disabled
*
* @see http://tldp.org/LDP/abs/html/exitcodes.html
* @see http://en.wikipedia.org/wiki/Unix_signal
*/
Expand Down Expand Up @@ -522,12 +522,12 @@ public function hasBeenSignaled()
{
$this->requireProcessIsTerminated(__FUNCTION__);

if ($this->isSigchildEnabled()) {
if (!$this->enhanceSigchildCompatibility && $this->isSigchildEnabled()) {
$this->stop(0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are now able to retrieve it when enhancing compat, but can we also without enhancing it ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

knowing "if" it has been signaled works with or without enhanced mode thanks to https://github.com/symfony/symfony/pull/16915/files#diff-f9f2411040cda7b73402481facf3e4ddR1133
knowing which signal is more tricky and requires enhanced mode

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in fact, I was wrong, knowing "if" it has been signaled doesn't work without the enhanced mode, fixed


throw new RuntimeException('This PHP has been compiled with --enable-sigchild. Term signal can not be retrieved.');
}

$this->updateStatus(false);

return $this->processInformation['signaled'];
}

Expand All @@ -545,12 +545,12 @@ public function getTermSignal()
{
$this->requireProcessIsTerminated(__FUNCTION__);

if ($this->isSigchildEnabled()) {
if ($this->isSigchildEnabled() && (!$this->enhanceSigchildCompatibility || -1 === $this->processInformation['termsig'])) {
$this->stop(0);

throw new RuntimeException('This PHP has been compiled with --enable-sigchild. Term signal can not be retrieved.');
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updateStatus is already called by requireProcessIsTerminated


$this->updateStatus(false);

return $this->processInformation['termsig'];
}

Expand All @@ -567,8 +567,6 @@ public function hasBeenStopped()
{
$this->requireProcessIsTerminated(__FUNCTION__);

$this->updateStatus(false);

return $this->processInformation['stopped'];
}

Expand All @@ -585,8 +583,6 @@ public function getStopSignal()
{
$this->requireProcessIsTerminated(__FUNCTION__);

$this->updateStatus(false);

return $this->processInformation['stopsig'];
}

Expand Down Expand Up @@ -660,7 +656,7 @@ public function stop($timeout = 10, $signal = null)
usleep(1000);
} while ($this->isRunning() && microtime(true) < $timeoutMicro);

if ($this->isRunning() && !$this->isSigchildEnabled()) {
if ($this->isRunning()) {
// Avoid exception here: process is supposed to be running, but it might have stopped just
// after this line. In any case, let's silently discard the error, we cannot do anything.
$this->doSignal($signal ?: 9, false);
Expand Down Expand Up @@ -998,9 +994,15 @@ private function getDescriptors()

if (!$this->useFileHandles && $this->enhanceSigchildCompatibility && $this->isSigchildEnabled()) {
// last exit code is output on the fourth pipe and caught to work around --enable-sigchild
$descriptors = array_merge($descriptors, array(array('pipe', 'w')));
$descriptors[3] = array('pipe', 'w');

$trap = '';
foreach (self::$posixSignals as $s) {
$trap .= "trap 'echo s$s >&3' $s;";
}

$this->commandline = '('.$this->commandline.') 3>/dev/null; code=$?; echo $code >&3; exit $code';
$this->commandline = $trap.'{ ('.$this->commandline.') <&3 3<&- 3>/dev/null & } 3<&0;';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A comment explaining what's done in this commandline, with a link to the stackoverflow issue would be more than welcome.
I doubt anybody could read this without error in two years (thinking about the first implementation was years ago)

$this->commandline .= 'pid=$!; echo p$pid >&3; wait $pid; code=$?; echo x$code >&3; exit $code';
}

return $descriptors;
Expand Down Expand Up @@ -1047,10 +1049,13 @@ protected function updateStatus($blocking)
}

$this->processInformation = proc_get_status($this->process);
$this->captureExitCode();

$this->readPipes($blocking, '\\' === DIRECTORY_SEPARATOR ? !$this->processInformation['running'] : true);

if ($this->fallbackStatus && $this->enhanceSigchildCompatibility && $this->isSigchildEnabled()) {
$this->processInformation = $this->fallbackStatus + $this->processInformation;
}

if (!$this->processInformation['running']) {
$this->close();
}
Expand All @@ -1067,7 +1072,7 @@ protected function isSigchildEnabled()
return self::$sigchild;
}

if (!function_exists('phpinfo')) {
if (!function_exists('phpinfo') || defined('HHVM_VERSION')) {
return self::$sigchild = false;
}

Expand All @@ -1093,24 +1098,24 @@ private function readPipes($blocking, $close)

$callback = $this->callback;
foreach ($result as $type => $data) {
if (3 == $type) {
$this->fallbackExitcode = (int) $data;
if (3 === $type) {
foreach (explode("\n", substr($data, 0, -1)) as $data) {
if ('p' === $data[0]) {
$this->fallbackStatus['pid'] = (int) substr($data, 1);
} elseif ('s' === $data[0]) {
$this->fallbackStatus['signaled'] = true;
$this->fallbackStatus['exitcode'] = -1;
$this->fallbackStatus['termsig'] = (int) substr($data, 1);
} elseif ('x' === $data[0] && !isset($this->fallbackStatus['signaled'])) {
$this->fallbackStatus['exitcode'] = (int) substr($data, 1);
}
}
} else {
$callback($type === self::STDOUT ? self::OUT : self::ERR, $data);
}
}
}

/**
* Captures the exitcode if mentioned in the process information.
*/
private function captureExitCode()
{
if (isset($this->processInformation['exitcode']) && -1 != $this->processInformation['exitcode']) {
$this->exitcode = $this->processInformation['exitcode'];
}
}

/**
* Closes process resource, closes file handles, sets the exitcode.
*
Expand All @@ -1120,19 +1125,19 @@ private function close()
{
$this->processPipes->close();
if (is_resource($this->process)) {
$exitcode = proc_close($this->process);
} else {
$exitcode = -1;
proc_close($this->process);
}

$this->exitcode = -1 !== $exitcode ? $exitcode : (null !== $this->exitcode ? $this->exitcode : -1);
$this->exitcode = $this->processInformation['exitcode'];
$this->status = self::STATUS_TERMINATED;

if (-1 === $this->exitcode && null !== $this->fallbackExitcode) {
$this->exitcode = $this->fallbackExitcode;
} elseif (-1 === $this->exitcode && $this->processInformation['signaled'] && 0 < $this->processInformation['termsig']) {
// if process has been signaled, no exitcode but a valid termsig, apply Unix convention
$this->exitcode = 128 + $this->processInformation['termsig'];
if (-1 === $this->exitcode) {
if ($this->processInformation['signaled'] && 0 < $this->processInformation['termsig']) {
// if process has been signaled, no exitcode but a valid termsig, apply Unix convention
$this->exitcode = 128 + $this->processInformation['termsig'];
} elseif ($this->enhanceSigchildCompatibility && $this->isSigchildEnabled()) {
$this->processInformation['signaled'] = true;
$this->processInformation['termsig'] = -1;
}
}

// Free memory from self-reference callback created by buildCallback
Expand All @@ -1151,7 +1156,7 @@ private function resetProcessData()
$this->starttime = null;
$this->callback = null;
$this->exitcode = null;
$this->fallbackExitcode = null;
$this->fallbackStatus = array();
$this->processInformation = null;
$this->stdout = null;
$this->stderr = null;
Expand All @@ -1171,7 +1176,7 @@ private function resetProcessData()
* @return bool True if the signal was sent successfully, false otherwise
*
* @throws LogicException In case the process is not running
* @throws RuntimeException In case --enable-sigchild is activated
* @throws RuntimeException In case --enable-sigchild is activated and the process can't be killed
* @throws RuntimeException In case of failure
*/
private function doSignal($signal, $throwException)
Expand All @@ -1184,9 +1189,9 @@ private function doSignal($signal, $throwException)
return false;
}

if ($this->isSigchildEnabled()) {
if ($this->enhanceSigchildCompatibility && $this->isSigchildEnabled() && !isset(self::$posixSignals[$signal]) && !(function_exists('posix_kill') && @posix_kill($this->getPid(), $signal))) {
if ($throwException) {
throw new RuntimeException('This PHP has been compiled with --enable-sigchild. The process can not be signaled.');
throw new RuntimeException('This PHP has been compiled with --enable-sigchild and posix_kill() is not available.');
}

return false;
Expand All @@ -1211,7 +1216,10 @@ private function doSignal($signal, $throwException)
return false;
}

$this->latestSignal = $signal;
$this->latestSignal = (int) $signal;
$this->fallbackStatus['signaled'] = true;
$this->fallbackStatus['exitcode'] = -1;
$this->fallbackStatus['termsig'] = $this->latestSignal;

return true;
}
Expand Down
14 changes: 5 additions & 9 deletions src/Symfony/Component/Process/Tests/PhpProcessTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,24 +30,20 @@ public function testNonBlockingWorks()

public function testCommandLine()
{
if ('phpdbg' === PHP_SAPI) {
$this->markTestSkipped('phpdbg SAPI is not supported by this test.');
}

$process = new PhpProcess(<<<PHP
<?php echo 'foobar';
PHP
);

$f = new PhpExecutableFinder();
$commandLine = $f->find();
$commandLine = $process->getCommandLine();

$this->assertSame($commandLine, $process->getCommandLine(), '::getCommandLine() returns the command line of PHP before start');
$f = new PhpExecutableFinder();
$this->assertContains($f->find(), $commandLine, '::getCommandLine() returns the command line of PHP before start');

$process->start();
$this->assertSame($commandLine, $process->getCommandLine(), '::getCommandLine() returns the command line of PHP after start');
$this->assertContains($commandLine, $process->getCommandLine(), '::getCommandLine() returns the command line of PHP after start');

$process->wait();
$this->assertSame($commandLine, $process->getCommandLine(), '::getCommandLine() returns the command line of PHP after wait');
$this->assertContains($commandLine, $process->getCommandLine(), '::getCommandLine() returns the command line of PHP after wait');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why changing these conditions ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because there are too much tightened to implementation details, and in fact do not work when testing with a sighchild enabled php

}
}
22 changes: 0 additions & 22 deletions src/Symfony/Component/Process/Tests/ProcessInSigchildEnvironment.php
View file
Open in desktop

This file was deleted.

Loading