Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[Process] Fix Inconsistent Exit Status in proc_get_status for PHP Versions Below 8.3#53821
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
carsonbot commentedFeb 7, 2024
Hey! Thanks for your PR. You are targeting branch "5.4" but it seems your PR description refers to branch "5.4, 6.4, 7.0". Cheers! Carsonbot |
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.
Can you please add a test case to cover the behavior?
Uh oh!
There was an error while loading.Please reload this page.
Reading now your full description... I'd still be great to figure out a test case :) |
A first testcase could be one running your reproducer (even if it does not fail 100% of the time due to relying on a race condition, it would still be likely to failsometimes if we reintroduce the issue in the future) |
Uh oh!
There was an error while loading.Please reload this page.
I've added a test that reproduces the bug, it fails on |
If I remove the fix, you can see the racing condition factor in the tests causing the tests to fail erratically on the longRunning process test.
|
Luc45 commentedFeb 7, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
My tests uses |
I don't know why |
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 thanks.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| @@ -1556,7 +1602,6 @@ public function testNotTerminableInputPipe() | |||
| /** | |||
| * @param string|array $commandline | |||
| * @param mixed $input | |||
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.
I didn't want to touch this but Fabbot code style tests asks to remove it. Bringing it up just for clarity.
7.2 tests fail because of the short closure syntax, want me to keep it@nicolas-grekas? |
nope, indeed |
Tests in Windows is failing because it doesn't have
Since it runs a bunch of tests in parallel, we have to ignore 80s and 59s because that's CPU time, and look at "actual time" which is the last one: 20s and 12s respectively, which sounds about right. Re-running. |
The remaining Windows failures seems unrelated |
Thank you@Luc45. |
…-daubois)This PR was merged into the 5.4 branch.Discussion----------[Process] Fix failing tests causing segfaults| Q | A| ------------- | ---| Branch? | 5.4| Bug fix? | yes| New feature? | no| Deprecations? | no| Issues | Fix tests| License | MIT[This PR](#53821) introduces some code that [triggers segfaults](https://github.com/symfony/symfony/actions/runs/7870905388/job/21473073396) in newer branches (7.x especially). We can achieve the same goal by using some bash script which is lighter and avoid the segfaults. I first tried this fix on 7.x and solved the issue (that does not happen on older branches).Commits-------041e178 [Process] Fix failing tests causing segfaults
[](https://renovatebot.com)This PR contains the following updates:| Package | Change | Age | Adoption | Passing | Confidence ||---|---|---|---|---|---|| [symfony/console](https://symfony.com)([source](https://togithub.com/symfony/console)) | `6.4.4` -> `7.0.4` |[](https://docs.renovatebot.com/merge-confidence/)|[](https://docs.renovatebot.com/merge-confidence/)|[](https://docs.renovatebot.com/merge-confidence/)|[](https://docs.renovatebot.com/merge-confidence/)|| [symfony/process](https://symfony.com)([source](https://togithub.com/symfony/process)) | `6.4.4` -> `7.0.4` |[](https://docs.renovatebot.com/merge-confidence/)|[](https://docs.renovatebot.com/merge-confidence/)|[](https://docs.renovatebot.com/merge-confidence/)|[](https://docs.renovatebot.com/merge-confidence/)|---### Release Notes<details><summary>symfony/console (symfony/console)</summary>### [`v7.0.4`](https://togithub.com/symfony/console/releases/tag/v7.0.4)[CompareSource](https://togithub.com/symfony/console/compare/v7.0.3...v7.0.4)**Changelog**(symfony/console@v7.0.3...v7.0.4)- bug[symfony/symfony#54009](https://togithub.com/symfony/symfony/issues/54009)\[Console] Fix display of vertical Table on Windows OS([@​VincentLanglet](https://togithub.com/VincentLanglet))- bug[symfony/symfony#54001](https://togithub.com/symfony/symfony/issues/54001)\[Console] Fix display of Table on Windows OS([@​VincentLanglet](https://togithub.com/VincentLanglet))- bug[symfony/symfony#53707](https://togithub.com/symfony/symfony/issues/53707)\[Console] Fix color support for TTY output([@​theofidry](https://togithub.com/theofidry))- bug[symfony/symfony#53711](https://togithub.com/symfony/symfony/issues/53711)\[Console] Allow false as a $shortcut in InputOption([@​jayminsilicon](https://togithub.com/jayminsilicon))### [`v7.0.3`](https://togithub.com/symfony/console/releases/tag/v7.0.3)[CompareSource](https://togithub.com/symfony/console/compare/v7.0.2...v7.0.3)**Changelog**(symfony/console@v7.0.2...v7.0.3)- bug[symfony/symfony#53516](https://togithub.com/symfony/symfony/issues/53516)\[Console] Allow '0' as a $shortcut in InputOption.php([@​lawsonjl-ornl](https://togithub.com/lawsonjl-ornl))- bug[symfony/symfony#53576](https://togithub.com/symfony/symfony/issues/53576)\[Console] Only execute additional checks for color support if theoutput ([@​theofidry](https://togithub.com/theofidry))### [`v7.0.2`](https://togithub.com/symfony/console/releases/tag/v7.0.2)[CompareSource](https://togithub.com/symfony/console/compare/v7.0.1...v7.0.2)**Changelog**(symfony/console@v7.0.1...v7.0.2)- bug[symfony/symfony#52940](https://togithub.com/symfony/symfony/issues/52940)\[Console] Fix color support check on non-Windows platforms([@​theofidry](https://togithub.com/theofidry))- bug[symfony/symfony#52941](https://togithub.com/symfony/symfony/issues/52941)\[Console] Fix xterm detection([@​theofidry](https://togithub.com/theofidry))### [`v7.0.1`](https://togithub.com/symfony/console/releases/tag/v7.0.1)[CompareSource](https://togithub.com/symfony/console/compare/v7.0.0...v7.0.1)**Changelog**(symfony/console@v7.0.0...v7.0.1)- no significant changes### [`v7.0.0`](https://togithub.com/symfony/console/releases/tag/v7.0.0)[CompareSource](https://togithub.com/symfony/console/compare/v6.4.4...v7.0.0)**Changelog**(symfony/console@v7.0.0-RC2...v7.0.0)- no significant changes</details><details><summary>symfony/process (symfony/process)</summary>### [`v7.0.4`](https://togithub.com/symfony/process/releases/tag/v7.0.4)[CompareSource](https://togithub.com/symfony/process/compare/v7.0.3...v7.0.4)**Changelog**(symfony/process@v7.0.3...v7.0.4)- bug[symfony/symfony#54006](https://togithub.com/symfony/symfony/issues/54006)\[Process] Fix the `command -v` exception (@​kayw-geek)- bug[symfony/symfony#53821](https://togithub.com/symfony/symfony/issues/53821)\[Process] Fix Inconsistent Exit Status in proc_get_status for PHPVersions Below 8.3 ([@​Luc45](https://togithub.com/Luc45))### [`v7.0.3`](https://togithub.com/symfony/process/releases/tag/v7.0.3)[CompareSource](https://togithub.com/symfony/process/compare/v7.0.2...v7.0.3)**Changelog**(symfony/process@v7.0.2...v7.0.3)- bug[symfony/symfony#53481](https://togithub.com/symfony/symfony/issues/53481)\[Process] Fix executable finder when the command starts with a dash([@​kayw-geek](https://togithub.com/kayw-geek))### [`v7.0.2`](https://togithub.com/symfony/process/releases/tag/v7.0.2)[CompareSource](https://togithub.com/symfony/process/compare/v7.0.0...v7.0.2)**Changelog**(symfony/process@v7.0.1...v7.0.2)- bug[symfony/symfony#52864](https://togithub.com/symfony/symfony/issues/52864)\[HttpClient]\[Mailer]\[Process] always pass microseconds to usleep asintegers ([@​xabbuh](https://togithub.com/xabbuh))### [`v7.0.0`](https://togithub.com/symfony/process/releases/tag/v7.0.0)[CompareSource](https://togithub.com/symfony/process/compare/v6.4.4...v7.0.0)**Changelog**(symfony/process@v7.0.0-RC2...v7.0.0)- no significant changes</details>---### Configuration📅 **Schedule**: Branch creation - At any time (no schedule defined),Automerge - At any time (no schedule defined).🚦 **Automerge**: Enabled.♻ **Rebasing**: Whenever PR becomes conflicted, or you tick therebase/retry checkbox.🔕 **Ignore**: Close this PR and you won't be reminded about theseupdates again.---- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, checkthis box---This PR has been generated by [MendRenovate](https://www.mend.io/free-developer-tools/renovate/). Viewrepository job log[here](https://developer.mend.io/github/Lendable/composer-license-checker).<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4yMjAuMiIsInVwZGF0ZWRJblZlciI6IjM3LjIyMC4yIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
%5d(https%3a%2f%2frenovatebot.com){kind=link}
Uh oh!
There was an error while loading.Please reload this page.
This PR addresses a bug in Symfony's Process component affecting PHP versions prior to 8.3. In these versions, calling
proc_get_statusmultiple 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.3changelog notes/fix PR:
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 asisRunning,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 perthis Stack Overflow question, 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:
proc_get_statuscalls 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.
Impact:
Testing:
I haven't added tests to this PR because:
I'm really not the type of developer that does not write tests, but I beg my pardon for the reasons above.
Considerations