Skip to content

[Process] More robustness and deterministic tests #17094

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 23, 2015
Merged

[Process] More robustness and deterministic tests #17094

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
6 changes: 6 additions & 0 deletions src/Symfony/Component/Process/PhpProcess.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,12 @@ public function __construct($script, $cwd = null, array $env = null, $timeout =
$php .= ' '.ProcessUtils::escapeArgument($file);
$script = null;
}
if ('\\' !== DIRECTORY_SEPARATOR && null !== $php) {
// exec is mandatory to deal with sending a signal to the process
// see https://github.com/symfony/symfony/issues/5030 about prepending
// command with exec
$php = 'exec '.$php;
Copy link
Member Author

Choose a reason for hiding this comment

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

OK for this as bug fix?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, but please add a comment explaning what's this / link to existing issues

Copy link
Member Author

Choose a reason for hiding this comment

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

comment added

}

parent::__construct($php, $cwd, $env, $script, $timeout, $options);
}
Expand Down
89 changes: 39 additions & 50 deletions src/Symfony/Component/Process/Process.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,12 +66,12 @@ class Process

private static $sigchild;
private static $posixSignals = array(
1 => 1, // SIGHUP
2 => 2, // SIGINT
3 => 3, // SIGQUIT
6 => 6, // SIGABRT
14 => 14, // SIGALRM
15 => 15, // SIGTERM
1, // SIGHUP
2, // SIGINT
3, // SIGQUIT
6, // SIGABRT
14, // SIGALRM
15, // SIGTERM
);

/**
Expand Down Expand Up @@ -242,27 +242,34 @@ public function start($callback = null)
if (!isset($this->options['bypass_shell'])) {
$this->options['bypass_shell'] = true;
}
}
} elseif (!$this->useFileHandles && $this->enhanceSigchildCompatibility && $this->isSigchildEnabled()) {
// last exit code is output on the fourth pipe and caught to work around --enable-sigchild
$descriptors[3] = array('pipe', 'w');

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

$ptsWorkaround = null;
// See https://unix.stackexchange.com/questions/71205/background-process-pipe-input
$commandline .= '{ ('.$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.

could you add the link to the stackoverflow post explaining this command line?

Copy link
Member Author

Choose a reason for hiding this comment

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

link added

$commandline .= 'pid=$!; echo $pid >&3; wait $pid; code=$?; echo x$code >&3; exit $code';
Copy link
Member

Choose a reason for hiding this comment

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

the old code had a p prefix before the pid, which is not there anymore. Does it still work fine ?


if (!$this->useFileHandles && $this->enhanceSigchildCompatibility && $this->isSigchildEnabled()) {
// Workaround for the bug, when PTS functionality is enabled.
Copy link
Member Author

Choose a reason for hiding this comment

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

the file handle is auto-closed once the local $ptsWorkaround var is freed

// @see : https://bugs.php.net/69442
$ptsWorkaround = fopen(__FILE__, 'r');
}

$this->process = proc_open($commandline, $descriptors, $this->processPipes->pipes, $this->cwd, $this->env, $this->options);

if ($ptsWorkaround) {
fclose($ptsWorkaround);
}

if (!is_resource($this->process)) {
throw new RuntimeException('Unable to launch a new process.');
}
$this->status = self::STATUS_STARTED;

if (isset($descriptors[3])) {
$this->fallbackStatus['pid'] = (int) fgets($this->processPipes->pipes[3]);
}
$this->processPipes->unblock();

if ($this->tty) {
Expand Down Expand Up @@ -468,8 +475,6 @@ public function getIncrementalErrorOutput()
public function getExitCode()
{
if (!$this->enhanceSigchildCompatibility && $this->isSigchildEnabled()) {
$this->stop(0);

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

Expand Down Expand Up @@ -523,8 +528,6 @@ public function hasBeenSignaled()
$this->requireProcessIsTerminated(__FUNCTION__);

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

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

Expand All @@ -546,8 +549,6 @@ public function getTermSignal()
$this->requireProcessIsTerminated(__FUNCTION__);

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.');
}

Expand Down Expand Up @@ -990,22 +991,8 @@ public function checkTimeout()
private function getDescriptors()
{
$this->processPipes = new ProcessPipes($this->useFileHandles, $this->tty);
$descriptors = $this->processPipes->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[3] = array('pipe', 'w');

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

$this->commandline = $trap.'{ ('.$this->commandline.') <&3 3<&- 3>/dev/null & } 3<&0;';
$this->commandline .= 'pid=$!; echo p$pid >&3; wait $pid; code=$?; echo x$code >&3; exit $code';
}

return $descriptors;
return $this->processPipes->getDescriptors();
}

/**
Expand Down Expand Up @@ -1106,8 +1093,11 @@ private function readPipes($blocking, $close)
$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);
} elseif ('x' === $data[0]) {
$this->fallbackStatus['running'] = false;
if (!isset($this->fallbackStatus['signaled'])) {
$this->fallbackStatus['exitcode'] = (int) substr($data, 1);
}
}
}
} else {
Expand Down Expand Up @@ -1189,14 +1179,6 @@ private function doSignal($signal, $throwException)
return false;
}

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 and posix_kill() is not available.');
}

return false;
}

if ('\\' === DIRECTORY_SEPARATOR) {
exec(sprintf('taskkill /F /T /PID %d 2>&1', $this->getPid()), $output, $exitCode);
if ($exitCode && $this->isRunning()) {
Expand All @@ -1206,14 +1188,21 @@ private function doSignal($signal, $throwException)

return false;
}
}

if (true !== @proc_terminate($this->process, $signal) && '\\' !== DIRECTORY_SEPARATOR) {
if ($throwException) {
throw new RuntimeException(sprintf('Error while sending signal `%s`.', $signal));
} else {
if (!$this->enhanceSigchildCompatibility || !$this->isSigchildEnabled()) {
$ok = @proc_terminate($this->process, $signal);
} elseif (function_exists('posix_kill')) {
$ok = @posix_kill($this->getPid(), $signal);
} elseif ($ok = proc_open(sprintf('kill -%d %d', $signal, $this->getPid()), array(2 => array('pipe', 'w')), $pipes)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

proc_open is available since we already used it to start the process

Copy link
Contributor

Choose a reason for hiding this comment

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

what about using a synchronous call for the commandline like exec? When reading next line, kill might not have returned yet

Copy link
Member Author

Choose a reason for hiding this comment

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

fgets is blocking so there is no way that reading the next line happens before kill is done

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

$ok = false === fgets($pipes[2]);
}
if (!$ok) {
if ($throwException) {
throw new RuntimeException(sprintf('Error while sending signal `%s`.', $signal));
}

return false;
return false;
}
}

$this->latestSignal = (int) $signal;
Expand Down
8 changes: 5 additions & 3 deletions src/Symfony/Component/Process/Tests/NonStopableProcess.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,16 +30,18 @@ function handleSignal($signal)
break;
}

echo "received signal $name\n";
echo "signal $name\n";
}

declare (ticks = 1);
pcntl_signal(SIGTERM, 'handleSignal');
pcntl_signal(SIGINT, 'handleSignal');

echo 'received ';

$duration = isset($argv[1]) ? (int) $argv[1] : 3;
$start = microtime(true);

while ($duration > (microtime(true) - $start)) {
usleep(1000);
usleep(10000);
pcntl_signal_dispatch();
}
Loading