minor #16813 [Process] Fix stopping a process on Windows (nicolas-grekas)

This PR was merged into the 2.3 branch.

Discussion
----------

[Process] Fix stopping a process on Windows

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

Commits
-------

80fb51c [Process] Fix stopping a process on Windows

src/Symfony/Component/Process/Process.php

@@ -157,8 +157,16 @@ class Process
 
     public function __destruct()
     {
-        // stop() will check if we have a process running.
-        $this->stop();
+        if ($this->isRunning()) {
+            $this->doSignal(15, false);
+            usleep(10000);
+        }
+        if ($this->isRunning()) {
+            usleep(100000);
+            $this->doSignal(9, false);
+        }
+
+        // Don't call ->stop() nor ->close() since we don't want to wait for the subprocess here
     }
 
     public function __clone()
@@ -1195,7 +1203,7 @@ class Process
 
         if ('\\' === DIRECTORY_SEPARATOR) {
             exec(sprintf('taskkill /F /T /PID %d 2>&1', $this->getPid()), $output, $exitCode);
-            if ($exitCode) {
+            if ($exitCode && $this->isRunning()) {
                 if ($throwException) {
                     throw new RuntimeException(sprintf('Unable to kill the process (%s).', implode(' ', $output)));
                 }

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

@@ -167,6 +167,10 @@ abstract class AbstractProcessTest extends \PHPUnit_Framework_TestCase
         $this->assertEquals($expectedLength, strlen($p->getErrorOutput()));
     }
 
+    /**
+     * @expectedException Symfony\Component\Process\Exception\LogicException
+     * @expectedExceptionMessage STDIN can not be set while the process is running.
+     */
     public function testSetStdinWhileRunningThrowsAnException()
     {
         $process = $this->getProcess(self::$phpBin.' -r "usleep(500000);"');
@@ -176,9 +180,10 @@ abstract class AbstractProcessTest extends \PHPUnit_Framework_TestCase
             $process->stop();
             $this->fail('A LogicException should have been raised.');
         } catch (LogicException $e) {
-            $this->assertEquals('STDIN can not be set while the process is running.', $e->getMessage());
         }
         $process->stop();
+
+        throw $e;
     }
 
     /**
@@ -659,6 +664,10 @@ abstract class AbstractProcessTest extends \PHPUnit_Framework_TestCase
         $this->assertNotEquals($process1->getOutput(), $process2->getOutput());
     }
 
+    /**
+     * @expectedException Symfony\Component\Process\Exception\RuntimeException
+     * @expectedExceptionMessage The process timed-out.
+     */
     public function testRunProcessWithTimeout()
     {
         $timeout = 0.5;
@@ -672,14 +681,13 @@ abstract class AbstractProcessTest extends \PHPUnit_Framework_TestCase
         }
         $duration = microtime(true) - $start;
 
-        if ('\\' === DIRECTORY_SEPARATOR) {
-            // Windows is a bit slower as it read file handles, then allow twice the precision
-            $maxDuration = $timeout + 2 * Process::TIMEOUT_PRECISION;
-        } else {
+        if ('\\' !== DIRECTORY_SEPARATOR) {
+            // On Windows, timers are too transient
             $maxDuration = $timeout + Process::TIMEOUT_PRECISION;
+            $this->assertLessThan($maxDuration, $duration);
         }
 
-        $this->assertLessThan($maxDuration, $duration);
+        throw $e;
     }
 
     public function testCheckTimeoutOnNonStartedProcess()
@@ -695,6 +703,10 @@ abstract class AbstractProcessTest extends \PHPUnit_Framework_TestCase
         $process->checkTimeout();
     }
 
+    /**
+     * @expectedException Symfony\Component\Process\Exception\RuntimeException
+     * @expectedExceptionMessage The process timed-out.
+     */
     public function testCheckTimeoutOnStartedProcess()
     {
         $timeout = 0.5;
@@ -717,8 +729,14 @@ abstract class AbstractProcessTest extends \PHPUnit_Framework_TestCase
 
         $this->assertLessThan($timeout + $precision, $duration);
         $this->assertFalse($process->isSuccessful());
+
+        throw $e;
     }
 
+    /**
+     * @expectedException Symfony\Component\Process\Exception\RuntimeException
+     * @expectedExceptionMessage The process timed-out.
+     */
     public function testStartAfterATimeout()
     {
         $process = $this->getProcess(sprintf('%s -r %s', self::$phpBin, escapeshellarg('$n = 1000; while ($n--) {echo \'\'; usleep(1000); }')));
@@ -731,6 +749,8 @@ abstract class AbstractProcessTest extends \PHPUnit_Framework_TestCase
         $process->start();
         usleep(10000);
         $process->stop();
+
+        throw $e;
     }
 
     public function testGetPid()
@@ -760,14 +780,14 @@ abstract class AbstractProcessTest extends \PHPUnit_Framework_TestCase
             $this->markTestSkipped('Extension pcntl is required.');
         }
 
-        $process = $this->getProcess('exec php -f '.__DIR__.'/SignalListener.php');
+        $process = $this->getProcess('exec '.self::$phpBin.' '.__DIR__.'/SignalListener.php');
         $process->start();
-        usleep(500000);
-        $process->signal(SIGUSR1);
 
-        while ($process->isRunning() && false === strpos($process->getOutput(), 'Caught SIGUSR1')) {
-            usleep(10000);
+        while (false === strpos($process->getOutput(), 'Caught')) {
+            usleep(1000);
         }
+        $process->signal(SIGUSR1);
+        $process->wait();
 
         $this->assertEquals('Caught SIGUSR1', $process->getOutput());
     }
@@ -828,6 +848,8 @@ abstract class AbstractProcessTest extends \PHPUnit_Framework_TestCase
 
     /**
      * @dataProvider provideMethodsThatNeedATerminatedProcess
+     * @expectedException Symfony\Component\Process\Exception\LogicException
+     * @expectedExceptionMessage Process must be terminated before calling
      */
     public function testMethodsThatNeedATerminatedProcess($method)
     {
@@ -838,10 +860,10 @@ abstract class AbstractProcessTest extends \PHPUnit_Framework_TestCase
             $process->stop(0);
             $this->fail('A LogicException must have been thrown');
         } catch (\Exception $e) {
-            $this->assertInstanceOf('Symfony\Component\Process\Exception\LogicException', $e);
-            $this->assertEquals(sprintf('Process must be terminated before calling %s.', $method), $e->getMessage());
         }
         $process->stop(0);
+
+        throw $e;
     }
 
     public function provideMethodsThatNeedATerminatedProcess()

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

@@ -112,6 +112,10 @@ class SigchildEnabledProcessTest extends AbstractProcessTest
         $this->markTestSkipped('Signal is not supported in sigchild environment');
     }
 
+    /**
+     * @expectedException Symfony\Component\Process\Exception\RuntimeException
+     * @expectedExceptionMessage The process timed-out.
+     */
     public function testStartAfterATimeout()
     {
         if ('\\' === DIRECTORY_SEPARATOR) {

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

@@ -9,17 +9,13 @@
  * file that was distributed with this source code.
  */
 
-// required for signal handling
-declare (ticks = 1);
+pcntl_signal(SIGUSR1, function () {echo 'SIGUSR1'; exit;});
 
-pcntl_signal(SIGUSR1, function () {echo 'Caught SIGUSR1'; exit;});
+echo 'Caught ';
 
 $n = 0;
 
-// ticks require activity to work - sleep(4); does not work
-while ($n < 400) {
+while ($n++ < 400) {
     usleep(10000);
-    ++$n;
+    pcntl_signal_dispatch();
 }
-
-return;