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

[2.1][Process] Fix stop in non-sigchild environments #5543

Merged
merged 1 commit into from Sep 19, 2012

Conversation

Projects
None yet
5 participants
@romainneutron
Member

romainneutron commented Sep 18, 2012

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

@stof

This comment has been minimized.

Member

stof commented Sep 18, 2012

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

$this->commandline = '('.$this->commandline.') 3>/dev/null; code=$?; echo $code >&3; exit $code';
} else {
$this->commandline = 'exec ' . $this->commandline;

This comment has been minimized.

@schmittjoh

schmittjoh Sep 18, 2012

Contributor

Can you change this to only prepend exec for non-Windows environments?

This comment has been minimized.

This comment has been minimized.

@schmittjoh

schmittjoh Sep 18, 2012

Contributor

Great, thanks :)

@schmittjoh

View changes

src/Symfony/Component/Process/Process.php Outdated
);
$this->commandline = '('.$this->commandline.') 3>/dev/null; code=$?; echo $code >&3; exit $code';
if ($this->isSigchildEnabled() && $this->enhanceSigchildCompatibility) {

This comment has been minimized.

@schmittjoh

schmittjoh Sep 18, 2012

Contributor

I would reorder these conditions as the second is less expensive, but it's just a micro-optimization :)

This comment has been minimized.

@pborreli

pborreli Sep 18, 2012

Contributor

both are executed every time, no ?

This comment has been minimized.

@romainneutron

romainneutron Sep 18, 2012

Member

As sigchild compatibility is activated in constructor in case PHP is compiled with sigchild, isSigchildEnabled is called at constructor.
As the result is saved in a static var, there's no micro-optimization left here. sorry :/

This comment has been minimized.

@schmittjoh

schmittjoh Sep 18, 2012

Contributor

Then the first condition would not make any sense if it always evaluates to true, but I haven't looked that deeply.

This comment has been minimized.

@stof

stof Sep 18, 2012

Member

@pborreli no. && stops executing conditions as soon as one of them returns false. It is the same for || when the left condition returns true.

@schmittjoh this does not change much IMO as Linux systems will call the isSigchildEnabled in the constructor to set the default value of the enhanceSigchildCompatibility anyway and the logic is done only the first time the Process is used and then cached. So the only difference in cost is a method call reading a property vs reading a property.

This comment has been minimized.

@schmittjoh

schmittjoh Sep 18, 2012

Contributor

Well, property access is faster than a method call, but I won't argue much here. Calling a process is probably a magnitude slower that it is not significant.

This comment has been minimized.

@pborreli

pborreli Sep 18, 2012

Contributor

@stof ah yeah forgot about that, thanks, good point @schmittjoh

Add a Sigchild compatibility mode (set to false by default)
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

fabpot added a commit that referenced this pull request Sep 19, 2012

merged branch romainneutron/FixProcessStop (PR #5543)
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

@fabpot fabpot merged commit 7bafc69 into symfony:2.1 Sep 19, 2012

fabpot added a commit that referenced this pull request Sep 28, 2012

merged branch lyrixx/process (PR #5592)
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 :)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment