-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[2.1][Process] Fix stop in non-sigchild environments #5543
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
This will also fix the error reported in Behat/MinkZombieDriver#10 |
|
||
$this->commandline = '('.$this->commandline.') 3>/dev/null; code=$?; echo $code >&3; exit $code'; | ||
} else { | ||
$this->commandline = 'exec ' . $this->commandline; |
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 change this to only prepend exec for non-Windows environments?
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.
Hello @schmittjoh , don't worry, look at https://github.com/romainneutron/symfony/blob/44dfdca1c35ee6b956e3814bb96d090504453b46/src/Symfony/Component/Process/Process.php#L212 , we're outside windows scope here :)
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.
Great, thanks :)
This mode is required to use a hack to determine if the process finished with success when PHP has been compiled with the --enable-sigchild option
Commits ------- 7bafc69 Add a Sigchild compatibility mode (set to false by default) Discussion ---------- [2.1][Process] Fix stop in non-sigchild environments Bug fix: yes Feature addition: yes Backwards compatibility break: no Symfony2 tests pass: yes License of the code: MIT Fix #5030 in half way. - `proc_terminate` now sends the `SIGTERM` to the real process, not the sh (add the exec prefix to remove the wrapper as suggested by @schmittjoh). So now, process will stop, except if you're working with a PHP compiled with `--enable-sigchild` - Add a Sigchild compatibility mode (activated only if PHP has been compiled with it) This mode is required to use a hack to determine if the process finished with success when PHP has been compiled with the --enable-sigchild option #5030 will be totally fixed in 2.2 with #5476 as it would allow to send a `SIGKILL` after timeout --------------------------------------------------------------------------- by stof at 2012-09-18T21:19:50Z This will also fix the error reported in Behat/MinkZombieDriver#10 The stop method was broken because of the sigchild workaround introduced in the latest 2.1-RC times
Commits ------- 27b2df9 [Process] Fixed bug introduced by 7bafc69. 7a955c0 [Process][Tests] Prove process fail (Add more test case) 598dcf3 [Process][Tests] Prove process fail Discussion ---------- [Process][Tests] Prove process fail with chained commands Bug fix: no Feature addition: no Backwards compatibility break: no Symfony2 tests pass: no Fixes the following tickets: - Todo: Fix that License of the code: MIT This PR is against 2.1 branch. Previous PR was #5575 This PR try to hiligh a regression in Process component. ``` php $process = new Process("echo -n 1 && echo -n 1"); // or $process = new Process("echo -n 1 ; echo -n 1"); $process->run(); var_dump('11' == $process->getOutput()); // false, var_dump($process->getOutput()); // '1', ``` This test failed because of PR #5543 ; see 7bafc69#L0R233 --------------------------------------------------------------------------- by romainneutron at 2012-09-25T13:05:45Z You've to revert the change that causes the fail (ie: remove https://github.com/symfony/symfony/blob/2.1/src/Symfony/Component/Process/Process.php#L233) --------------------------------------------------------------------------- by romainneutron at 2012-09-25T13:06:56Z BTW, removing this line re-open #5030 --------------------------------------------------------------------------- by stof at 2012-09-25T13:11:15Z @lyrixx please add a commit reverting the addition of ``exec`` in the case of sigchild not being used (only this addition, not the full commit you linked) as it should fix your test. --------------------------------------------------------------------------- by stof at 2012-09-25T13:12:21Z @fabpot btw, this regression is quite important. As I said in the previous PR, it impacts composer in a bunch of places. --------------------------------------------------------------------------- by romainneutron at 2012-09-25T13:30:07Z You reverted too much things, you just had to remove line 233 --------------------------------------------------------------------------- by stof at 2012-09-25T13:42:49Z @lyrixx I explicitly asked you to revert only the ``exec`` addition for the case without sigchild. --------------------------------------------------------------------------- by lyrixx at 2012-09-25T13:55:57Z @stof Sorry, i fixed that. --------------------------------------------------------------------------- by romainneutron at 2012-09-25T13:56:26Z @lyrixx just remove the two last commit, edit Process.php and remove line 233 --------------------------------------------------------------------------- by lyrixx at 2012-09-25T13:59:59Z @romainneutron I think it's ok now. --------------------------------------------------------------------------- by romainneutron at 2012-09-25T14:11:28Z yep it's good :)
Bug fix: yes
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
License of the code: MIT
Fix #5030 in half way.
proc_terminate
now sends theSIGTERM
to the real process, not the sh (add the exec prefix to remove the wrapper as suggested by @schmittjoh). So now, process will stop, except if you're working with a PHP compiled with--enable-sigchild
This mode is required to use a hack to determine if the process finished with success when PHP has been compiled with the --enable-sigchild option
#5030 will be totally fixed in 2.2 with #5476 as it would allow to send a
SIGKILL
after timeout