8000 bug #53821 [Process] Fix Inconsistent Exit Status in proc_get_status … · symfony/symfony@3870d1e · GitHub
[go: up one dir, main page]
Skip to content

Commit 3870d1e

Browse files
nicolas-grekasnicolas-grekas
committed
bug #53821 [Process] Fix Inconsistent Exit Status in proc_get_status for PHP Versions Below 8.3 (Luc45)
This PR was squashed before being merged into the 5.4 branch. Discussion ---------- [Process] Fix Inconsistent Exit Status in proc_get_status for PHP Versions Below 8.3 | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | N/A <!-- No existing issue, direct PR --> | License | MIT This PR addresses a bug in Symfony's Process component affecting PHP versions prior to 8.3. In these versions, calling `proc_get_status` multiple times on the same process resource only returns the correct exit status on the first call, with subsequent calls returning -1 due to the process being discarded. This behavior can lead to race conditions and incorrect process status reporting in Symfony applications. PHP 8.3 [changelog notes](https://www.php.net/manual/en/migration83.incompatible.php)/[fix PR](php/php-src#10250): > Executing proc_get_status() multiple times > > Executing proc_get_status() multiple times will now always return the right value on POSIX systems. > Previously, only the first call of the function returned the right value. Executing proc_close() > after proc_get_status() will now also return the right exit code. Previously this would return -1. Symfony Process is very good at avoiding this, but it's possible to trigger this behavior if you pass the Process as a reference to the output callback itself, and inside of it you invoke a method that eventually triggers `proc_get_status`, such as `isRunning`, `isSuccessful`, etc. This PR tries to mimick PHP 8.3 behavior in older versions, caching the first reported valid exit status code once the process exits, and reusing the cached value if the new exit status code is now invalid. I believe this bug has been pestering Symfony Process for a long time, as per [this Stack Overflow question](https://stackoverflow.com/questions/31828700/symfony-process-component-returns-exit-code-1/77951961#77951961), but it was very hard to track it down, and only happens in certain conditions, and it also have a racing condition factor depending on the time between the last output of the process and it's termination. ### Changes: - Added caching mechanism for the exit status in the Process component. - Ensured the cached status is used for subsequent `proc_get_status` calls if PHP version is below 8.3. ### Proof of Concept: To demonstrate the issue and the effectiveness of the fix, a proof of concept script is included below. This script uses Symfony's Process component to start a subprocess that outputs a message and exits with a specific status code. The script then attempts to retrieve the exit status of the process using getExitCode(). In PHP versions prior to 8.3, without the proposed fix, this script will often incorrectly report an exit code of -1 due to the race condition described earlier. ```php <?php if ( getenv( 'OUTPUT' ) ) { #echo 'This should fail when "wait" gets large'; sleep( 2 ); echo 'This should fail even when "wait" is small'; die( 123 ); } use Symfony\Component\Process\Process; require_once __DIR__ . '/vendor/autoload.php'; $wait = 0; do { // Increasingly bigger waits. $wait += 100000; echo sprintf( 'Wait: %s', $wait ) . PHP_EOL; $p = new Process( [ 'php', __FILE__ ], __DIR__, [ 'OUTPUT' => 1, ] ); $p->start( function ( string $type, string $out ) use ( $p ) { echo $out . PHP_EOL; /** * Calling most methods in Symfony Process that triggers * updateStatus() can potentially trigger the -1 bug. * * `@see` Process::updateStatus() */ echo sprintf( 'Is Running: %s', $p->isRunning() ? 'Yes' : 'No' ) . PHP_EOL; echo sprintf( 'Exit Code: %s', $p->getExitCode() ) . PHP_EOL; } ); while ( $p->isRunning() ) { usleep( $wait ); } if ( ! $p->isSuccessful() ) { break; } } while ( true ); $is_started = $p->isStarted(); $is_running = $p->isRunning(); $exit_code = $p->getExitCode(); echo sprintf( 'Started: %s, Running: %s, Exit code: %s', $is_started, $is_running, $exit_code ) . PHP_EOL; ``` ### Impact: - This change only affects Symfony applications running on PHP versions below 8.3. - It improves the reliability of process status reporting in these environments. - No breaking changes or backward compatibility issues are introduced. ### Testing: I haven't added tests to this PR because: - This bug involves racing conditions, which is hard to replicate. - It's past 1 AM local time right now, and I've been debugging this for the past 6 hours. - The provided "Proof of Concept" can serve as an initial check to confirm the bug. I'm really not the type of developer that does not write tests, but I beg my pardon for the reasons above. ### Considerations - Based on the [proc_open](https://github.com/php/php-src/blob/php-8.3.2/ext/standard/proc_open.c) implementation, this fix might not be needed on Windows. - You can find additional context for this bug in this [Stack Overflow answer](https://stackoverflow.com/a/77951961/2056484). Commits ------- 500caa3 [Process] Fix Inconsistent Exit Status in proc_get_status for PHP Versions Below 8.3
2 parents e804c27 + 500caa3 commit 3870d1e

File tree

2 files changed

+68
-1
lines changed

2 files changed

+68
-1
lines changed

‎src/Symfony/Component/Process/Process.php

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,7 @@ class Process implements \IteratorAggregate
8080
private $processPipes;
8181

8282
private $latestSignal;
83+
private $cachedExitCode;
8384

8485
private static $sigchild;
8586

@@ -1345,6 +1346,19 @@ protected function updateStatus(bool $blocking)
13451346
$this->processInformation = proc_get_status($this->process);
13461347
$running = $this->processInformation['running'];
13471348

1349+
// In PHP < 8.3, "proc_get_status" only returns the correct exit status on the first call.
1350+
// Subsequent calls return -1 as the process is discarded. This workaround caches the first
1351+
// retrieved exit status for consistent results in later calls, mimicking PHP 8.3 behavior.
1352+
if (\PHP_VERSION_ID < 80300) {
1353+
if (!isset($this->cachedExitCode) && !$running && -1 !== $this->processInformation['exitcode']) {
1354+
$this->cachedExitCode = $this->processInformation['exitcode'];
1355+
}
1356+
1357+
if (isset($this->cachedExitCode) && !$running && -1 === $this->processInformation['exitcode']) {
1358+
$this->processInformation['exitcode'] = $this->cachedExitCode;
1359+
}
1360+
}
1361+
13481362
$this->readPipes($running && $blocking, '\\' !== \DIRECTORY_SEPARATOR || !$running);
13491363

13501364
if ($this->fallbackStatus && $this->isSigchildEnabled()) {

‎src/Symfony/Component/Process/Tests/ProcessTest.php

Lines changed: 54 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1541,6 +1541,60 @@ public function testEnvCaseInsensitiveOnWindows()
15411541
}
15421542
}
15431543

1544+
public function testMultipleCallsToProcGetStatus()
1545+
{
1546+
$process = $this->getProcess('echo foo');
1547+
$process->start(static function () use ($process) {
1548+
return $process->isRunning();
1549+
});
1550+
while ($process->isRunning()) {
1551+
usleep(1000);
1552+
}
1553+
$this->assertSame(0, $process->getExitCode());
1554+
}
1555+
1556+
public function testFailingProcessWithMultipleCallsToProcGetStatus()
1557+
{
1558+
$process = $this->getProcess('exit 123');
1559+
$process->start(static function () use ($process) {
1560+
return $process->isRunning();
1561+
});
1562+
while ($process->isRunning()) {
1563+
usleep(1000);
1564+
}
1565+
$this->assertSame(123, $process->getExitCode());
1566+
}
1567+
1568+
/**
1569+
* @group slow
1570+
*/
1571+
public function testLongRunningProcessWithMultipleCallsToProcGetStatus()
1572+
{
1573+
$process = $this->getProcess('php -r "sleep(1); echo \'done\';"');
1574+
$process->start(static function () use ($process) {
1575+
return $process->isRunning();
1576+
});
1577+
while ($process->isRunning()) {
1578+
usleep(1000);
1579+
}
1580+
$this->assertSame(0, $process->getExitCode());
1581+
}
1582+
1583+
/**
1584+
* @group slow
1585+
*/
1586+
public function testLongRunningProcessWithMultipleCallsToProcGetStatusError()
1587+
{
1588+
$process = $this->getProcess('php -r "sleep(1); echo \'failure\'; exit(123);"');
1589+
$process->start(static function () use ($process) {
1590+
return $process->isRunning();
1591+
});
1592+
while ($process->isRunning()) {
1593+
usleep(1000);
1594+
}
1595+
$this->assertSame(123, $process->getExitCode());
1596+
}
1597+
15441598
/**
15451599
* @group transient-on-windows
15461600
*/
@@ -1556,7 +1610,6 @@ public function testNotTerminableInputPipe()
15561610

15571611
/**
15581612
* @param string|array $commandline
1559-
* @param mixed $input
15601613
*/
15611614
private function getProcess($commandline, ?string $cwd = null, ?array $env = null, $input = null, ?int $timeout = 60): Process
15621615
{

0 commit comments

Comments
 (0)
0