Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
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
- Loading branch information