-
Notifications
You must be signed in to change notification settings - Fork 7.8k
Fix GH-10239: proc_close after proc_get_status always returns -1 #10250
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.
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
Conversation
Does this also change the behavior of proc_get_status? |
Previously, multiple |
Although I believe this is an improvement, it is technically a breaking change and needs to be documented in the upgrading notes. The behavior is well known and documented in the proc_get_status page (but not in the proc_close page, even though it was also known behavior) |
I at first didn't see the note in the docs. The note is even incomplete as not only the exit code, but also the other values will not be set correctly on the second call. |
Rebased on master, and added a NOTE to UPGRADING. |
I'm not really sure if we want to do this as currently |
I understand what you're saying, but I think this reasoning only holds for posix systems. These functions are also supported on Windows, and as far as my understanding goes, the child process handle on Windows is only closed upon calling The original bug report was about using |
Good point about Windows even though it's a minority OS for PHP. I'm thinking that it might be actually enough to add a field to the result specifying that the result is cached. Then it could still be used as |
The waitpid function only works once when a process is exited. Cache the result so subsequent status reads succeed.
Co-authored-by: pezcurrel <33592962+pezcurrel@users.noreply.github.com>
…atus() Users who previously would've relied on proc_get_status() failing after calling it more than one time can now check the "cached" key to see whether the result was cached. If it is cached, it would've previously returned a failure in the form of a -1 exit code and the status booleans all being false.
I've updated this with your suggestion. I've added a "cached" key to the array which indicates whether the result was cached. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thank you both, I fixed the terminology to always use cached instead of stored. |
…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
Fixes GH-10239
The waitpid function only works once when a process is exited.
Cache the result so subsequent status reads succeed.