[Process] Enhance compatiblity with --enable-sigchild #16915

nicolas-grekas · 2015-12-08T17:17:53Z

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#16888
License	MIT
Doc PR	-

This is complete rewrite of the fallback --enable-sigchild handling in the Process class.
It removes most of the differences between this and a non-sigchild-enabled php.
Which means the test suite doesn't need anymore to be replayed 3 times (which is how I started this PR, looking for a way to test this component in less time).

I validated this with a locally compiled php, sigchild-enabled. Green.
Changes affect only this special-mode php.

Ping @romainneutron and @Seldaek (original writer of the sigchild support)

Submitted on 2.3 as bugfix, which it is to me.

stof · 2015-12-08T17:26:44Z

src/Symfony/Component/Process/Tests/PhpProcessTest.php


        $process->wait();
-        $this->assertSame($commandLine, $process->getCommandLine(), '::getCommandLine() returns the command line of PHP after wait');
+        $this->assertContains($commandLine, $process->getCommandLine(), '::getCommandLine() returns the command line of PHP after wait');


why changing these conditions ?

because there are too much tightened to implementation details, and in fact do not work when testing with a sighchild enabled php

stof · 2015-12-08T18:26:43Z

src/Symfony/Component/Process/Process.php

     */
    public function hasBeenSignaled()
    {
        $this->requireProcessIsTerminated(__FUNCTION__);

-        if ($this->isSigchildEnabled()) {
-            throw new RuntimeException('This PHP has been compiled with --enable-sigchild. Term signal can not be retrieved.');


we are now able to retrieve it when enhancing compat, but can we also without enhancing it ?

knowing "if" it has been signaled works with or without enhanced mode thanks to https://github.com/symfony/symfony/pull/16915/files#diff-f9f2411040cda7b73402481facf3e4ddR1133
knowing which signal is more tricky and requires enhanced mode

in fact, I was wrong, knowing "if" it has been signaled doesn't work without the enhanced mode, fixed

nicolas-grekas · 2015-12-08T18:47:43Z

Comments addressed, ready for a second round of review :)

Seldaek · 2015-12-08T19:34:41Z

Sorry not able to review in depth right now but I sure hope it won't blow up anything. Then again I think sigchild usage is probably less and less of a problem the more time passes.

nicolas-grekas · 2015-12-09T10:06:02Z

I added a special build of php with sigchild enabled on travis so that we can test, verify, prove and enforce that this really works :)
Enabled on the first deps=no line only.

nicolas-grekas · 2015-12-09T10:31:50Z

src/Symfony/Component/Process/Process.php

            throw new RuntimeException('This PHP has been compiled with --enable-sigchild. Term signal can not be retrieved.');
        }

-        $this->updateStatus(false);


updateStatus is already called by requireProcessIsTerminated

stof · 2015-12-09T10:58:44Z

.travis.yml

@@ -32,6 +33,7 @@ env:

 before_install:
    - if [[ "$deps" = "no" ]] && [[ "$TRAVIS_PHP_VERSION" =~ 5.[45] ]] && [[ "$TRAVIS_PULL_REQUEST" != "false" ]]; then export deps=skip; fi;
+    - if [[ $deps = no && $TRAVIS_PHP_VERSION = 5.3 && ! -d php-5.3.9/sapi ]]; then wget http://museum.php.net/php5/php-5.3.9.tar.bz2; tar -xjf php-5.3.9.tar.bz2; (cd php-5.3.9; ./configure --enable-sigchild --enable-pcntl; make -j2); fi;


I assume that the version will have to change for the 3.0 branch ?

yes, I propose 5.5.9 there

stof · 2015-12-09T11:00:30Z

@nicolas-grekas have you tried running tests when forcing the sighchild compat layer to be disabled ?

nicolas-grekas · 2015-12-09T11:24:10Z

@stof yes. Nothing interesting here: tests either fail because of one of those exceptions about --enable-sigchild, or pass.

stof · 2015-12-09T11:50:34Z

@nicolas-grekas but it needs to be done to ensure we don't miss some of the sigchild checks

nicolas-grekas · 2015-12-09T18:13:18Z

Sigchild is now tested with and without the enhanced mode.

romainneutron · 2015-12-10T09:51:22Z

Looks good to me

stof · 2015-12-10T15:17:37Z

src/Symfony/Component/Process/Tests/ProcessTest.php

+    {
+        $process = new Process($commandline, $cwd, $env, $stdin, $timeout, $options);
+
+        if (false !== $enhance = getenv('ENHANCE_SIGCHLD')) {


aren't you missing a I in the name ?

SIGCHLD is the name of the said signal, so it was intentional...

stof · 2015-12-10T15:19:20Z

except for the comment I made, 👍

@romainneutron

…olas-grekas) This PR was merged into the 2.3 branch. Discussion ---------- [Process] Enhance compatiblity with --enable-sigchild | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #16888 | License | MIT | Doc PR | - This is complete rewrite of the fallback `--enable-sigchild` handling in the Process class. It removes most of the differences between this and a non-sigchild-enabled php. Which means the test suite doesn't need anymore to be replayed 3 times (which is how I started this PR, looking for a way to test this component in less time). I validated this with a locally compiled php, sigchild-enabled. Green. Changes affect only this special-mode php. Ping @romainneutron and @Seldaek (original writer of the sigchild support) Submitted on 2.3 as bugfix, which it is to me. Commits ------- e7cc4aa [Process] Enhance compatiblity with --enable-sigchild

romainneutron · 2015-12-11T08:12:46Z

src/Symfony/Component/Process/Process.php


-            $this->commandline = '('.$this->commandline.') 3>/dev/null; code=$?; echo $code >&3; exit $code';
+            $this->commandline = $trap.'{ ('.$this->commandline.') <&3 3<&- 3>/dev/null & } 3<&0;';


A comment explaining what's done in this commandline, with a link to the stackoverflow issue would be more than welcome.
I doubt anybody could read this without error in two years (thinking about the first implementation was years ago)

carsonbot added the Status: Needs Review label Dec 8, 2015

stof reviewed Dec 8, 2015
View reviewed changes

nicolas-grekas force-pushed the fixsigchild branch from b9395dc to deab96b Compare December 8, 2015 18:24

stof reviewed Dec 8, 2015
View reviewed changes

nicolas-grekas force-pushed the fixsigchild branch 2 times, most recently from 14d13ee to 7b11f1f Compare December 8, 2015 18:44

nicolas-grekas force-pushed the fixsigchild branch 7 times, most recently from 8cadae4 to e36b1b4 Compare December 9, 2015 10:02

nicolas-grekas force-pushed the fixsigchild branch from e36b1b4 to 36fb047 Compare December 9, 2015 10:29

nicolas-grekas reviewed Dec 9, 2015
View reviewed changes

nicolas-grekas force-pushed the fixsigchild branch 2 times, most recently from b2d9c18 to bd38abd Compare December 9, 2015 10:37

stof reviewed Dec 9, 2015
View reviewed changes

nicolas-grekas force-pushed the fixsigchild branch from bd38abd to 24bd3a2 Compare December 9, 2015 18:10

nicolas-grekas force-pushed the fixsigchild branch from 24bd3a2 to db6239d Compare December 9, 2015 18:26

nicolas-grekas force-pushed the fixsigchild branch from 070bf65 to db6239d Compare December 10, 2015 13:39

[Process] Enhance compatiblity with --enable-sigchild

e7cc4aa

nicolas-grekas force-pushed the fixsigchild branch from db6239d to e7cc4aa Compare December 10, 2015 15:01

stof reviewed Dec 10, 2015
View reviewed changes

nicolas-grekas merged commit e7cc4aa into symfony:2.3 Dec 10, 2015

nicolas-grekas deleted the fixsigchild branch December 10, 2015 17:36

romainneutron reviewed Dec 11, 2015
View reviewed changes

This was referenced Dec 26, 2015

Release v2.3.36 #17141

Merged

Release v2.7.8 #17142

Merged

Release v2.8.1 #17143

Merged

Release v3.0.1 #17144

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Process] Enhance compatiblity with --enable-sigchild #16915

[Process] Enhance compatiblity with --enable-sigchild #16915

nicolas-grekas commented Dec 8, 2015

stof Dec 8, 2015

nicolas-grekas Dec 8, 2015

stof Dec 8, 2015

nicolas-grekas Dec 8, 2015

nicolas-grekas Dec 9, 2015

nicolas-grekas commented Dec 8, 2015

Seldaek commented Dec 8, 2015

nicolas-grekas commented Dec 9, 2015

nicolas-grekas Dec 9, 2015

stof Dec 9, 2015

nicolas-grekas Dec 9, 2015

stof commented Dec 9, 2015

nicolas-grekas commented Dec 9, 2015

stof commented Dec 9, 2015

nicolas-grekas commented Dec 9, 2015 via email

romainneutron commented Dec 10, 2015

stof Dec 10, 2015

nicolas-grekas Dec 10, 2015

stof commented Dec 10, 2015

romainneutron Dec 11, 2015


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

[Process] Enhance compatiblity with --enable-sigchild #16915

[Process] Enhance compatiblity with --enable-sigchild #16915

Conversation

nicolas-grekas commented Dec 8, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Dec 8, 2015

Seldaek commented Dec 8, 2015

nicolas-grekas commented Dec 9, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stof commented Dec 9, 2015

nicolas-grekas commented Dec 9, 2015

stof commented Dec 9, 2015

nicolas-grekas commented Dec 9, 2015 via email

romainneutron commented Dec 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stof commented Dec 10, 2015

Choose a reason for hiding this comment