merged branch romainneutron/timeout-restart (PR #9050)

fabpot · fabpot · commit 8efd5cadc8f1 · 2013-09-16T13:54:08.000+02:00
This PR was merged into the 2.3 branch. Discussion ---------- [Process][2.3] Properly close pipes after a Process::stop call | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | License | MIT When calling `Process::stop`, if the process does not stop before the end of the method, pipes are not close, `proc_close` is not called. I added a test that randomly fails without the patch : ``` phpunit --filter=testStartAfterATimeout --repeat 100 Tests/SimpleProcessTest.php PHPUnit 3.8-g55a6dd0 by Sebastian Bergmann. Configuration read from /Users/romain/Documents/workspace/symfony/src/Symfony/Component/Process/phpunit.xml.dist ....E.E......E.....E..........EE..............EEE..EE.......E.. 63 / 100 ( 63%) E.E................E....EE.......E... Time: 29.55 seconds, Memory: 6.75Mb There were 18 errors: 1) Symfony\Component\Process\Tests\SimpleProcessTest::testStartAfterATimeout fclose(): 89 is not a valid stream resource /Users/romain/Documents/workspace/symfony/src/Symfony/Component/Process/ProcessPipes.php:95 /Users/romain/Documents/workspace/symfony/src/Symfony/Component/Process/ProcessPipes.php:80 /Users/romain/Documents/workspace/symfony/src/Symfony/Component/Process/ProcessPipes.php:62 /Users/romain/Documents/workspace/symfony/src/Symfony/Component/Process/Process.php:938 /Users/romain/Documents/workspace/symfony/src/Symfony/Component/Process/Process.php:229 /Users/romain/Documents/workspace/symfony/src/Symfony/Component/Process/Tests/AbstractProcessTest.php:490 ``` And of course, I solved the issue, tests are now OK. Commits ------- d84df4c [Process] Properly close pipes after a Process::stop call
diff --git a/src/Symfony/Component/Process/Process.php b/src/Symfony/Component/Process/Process.php
@@ -626,9 +626,13 @@ public function stop($timeout = 10, $signal = null)
                     $this->signal($signal ?: SIGKILL);
                 }
             }
+        }
 
-            $this->updateStatus(false);
+        $this->updateStatus(false);
+        if ($this->processInformation['running']) {
+            $this->close();
         }
+
         $this->status = self::STATUS_TERMINATED;
 
         return $this->exitcode;
diff --git a/src/Symfony/Component/Process/Tests/AbstractProcessTest.php b/src/Symfony/Component/Process/Tests/AbstractProcessTest.php
@@ -477,6 +477,21 @@ public function testCheckTimeoutOnStartedProcess()
         $this->assertFalse($process->isSuccessful());
     }
 
+    public function testStartAfterATimeout()
+    {
+        $process = $this->getProcess('php -r "while(true) {echo \'\'; usleep(1000); }"');
+        $process->setTimeout(0.1);
+        try {
+            $process->run();
+            $this->fail('An exception should have been raised.');
+        } catch (\Exception $e) {
+
+        }
+        $process->start();
+        usleep(10000);
+        $process->stop();
+    }
+
     public function testGetPid()
     {
         $process = $this->getProcess('php -r "sleep(1);"');

Original file line number	Diff line number	Diff line change
`@@ -626,9 +626,13 @@ public function stop($timeout = 10, $signal = null)`
`626`	`626`	`$this->signal($signal ?: SIGKILL);`
`627`	`627`	`}`
`628`	`628`	`}`
	`629`	`+ }`
`629`	`630`
`630`		`- $this->updateStatus(false);`
	`631`	`+ $this->updateStatus(false);`
	`632`	`+ if ($this->processInformation['running']) {`
	`633`	`+ $this->close();`
`631`	`634`	`}`
	`635`	`+`
`632`	`636`	`$this->status = self::STATUS_TERMINATED;`
`633`	`637`
`634`	`638`	`return $this->exitcode;`