[Process] Add feature "wait until callback" to process class #27742

Nek- · 2018-06-27T13:11:51Z

I often see code like the following:

$process->start();
// wait for the process to be ready
sleep(3);

Many times in tests, sometimes in other places. There is a problem with this kind of code because if for any reason the process starts slower than the usual... Your code may fail after that. Also if it's faster, you're losing time for waiting.

So here is my proposal:

$process->start();
$process->waitUntil(function($type, $output) {
    // check the output and return true to stop waiting when you got what you wait
});

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	none
License	MIT
Doc PR	Waiting for feedbacks

mcsky · 2018-06-27T14:02:24Z

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

+        $p->stop();
+
+        $this->assertEquals("First iteration output\nSecond iteration output\nOne more iteration output\n", $completeOutput);
+        $this->assertLessThan(15, microtime(true) - $start);


15 is too much, the process do 3 iterations in KillableProcessWithOutput.php so it should take less than 2 seconds

linaori · 2018-06-27T14:50:48Z

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

+
+foreach ($outputs as $output) {
+    usleep($iterationTime);
+    $iterationTime *= 10;


Won't this create an extremely slow test?

The test should end after 1s and 110ms of execution of this code. If a failure occurs, the timeout will be reached 60s after the start of the test.

The process is long to end (but ends, for "security" purpose) to simulate the running of a daemon. It is stop by the test.

Note: for some reason under windows it looks like this test takes 4s. No idea why and how. Any though about?

Windows PHPUnit output on test process:

1) Symfony\Component\Process\Tests\ProcessTest::testWaitUntilSpecificOutput Failed asserting that 4.3028318881989 is less than 2. C:\projects\symfony\src\Symfony\Component\Process\Tests\ProcessTest.php:155

Here is an ugly fix suggestion.

Nek- · 2018-07-16T10:29:44Z

Hello @iltar what's the state of this PR? :)

linaori · 2018-07-16T10:46:18Z

@Nek- it's your PR, so I'm not sure what you mean 😅

Nek- · 2018-07-16T11:53:25Z

@iltar sorry you're so active on PRs that I thought you were in the core team 😅!

Nek- · 2018-07-16T11:55:54Z

ping @romainneutron @nicolas-grekas then :)

Nek- · 2018-10-05T07:42:25Z

Annnnnd... Rebased again. ping @romainneutron @nicolas-grekas

Nek- · 2018-10-05T13:05:07Z

Error on CI not related.

fabpot · 2018-10-10T10:42:27Z

src/Symfony/Component/Process/Process.php

@@ -429,6 +429,45 @@ public function wait(callable $callback = null)
        return $this->exitcode;
    }

+    /**
+     * Waits until the callback return true.


typo: returns

fabpot · 2018-10-10T10:43:40Z

src/Symfony/Component/Process/Process.php

+
+        if (!$this->processPipes->haveReadSupport()) {
+            $this->stop(0);
+            throw new \LogicException('Pass the callback to the Process::start method or enableOutput to use a callback with Process::waitUntil');


Can you add a dot at the end of the exception message? Also, you need to quote PHP code:

Pass the callback to the "Process::start" method or "enableOutput" to use a callback with "Process::waitUntil".

Perhaps add call before enableOutput: ... method or call "enableOutput" ...

fabpot · 2018-10-10T10:45:33Z

Can you also add some notes in the CHANGELOG file of the component?

stof · 2018-10-10T10:52:31Z

src/Symfony/Component/Process/Process.php

@@ -1264,7 +1303,7 @@ protected function buildCallback(callable $callback = null)
        if ($this->outputDisabled) {
            return function ($type, $data) use ($callback) {
                if (null !== $callback) {
-                    \call_user_func($callback, $type, $data);
+                    return $callback($type, $data);


does this finally work for all kind of callables in PHP 7.1+ ? In PHP 5.6, I know there was still 1 kind of callable failing this.

I was not aware of that fact. No idea. Can you give more insights to verify that fact?

Let's revert this change, no big deal (same below)

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

fabpot · 2018-10-11T11:01:28Z

src/Symfony/Component/Process/Process.php

@@ -1264,7 +1303,7 @@ protected function buildCallback(callable $callback = null)
        if ($this->outputDisabled) {
            return function ($type, $data) use ($callback) {
                if (null !== $callback) {
-                    \call_user_func($callback, $type, $data);
+                    return $callback($type, $data);


Let's revert this change, no big deal (same below)

Nek- · 2018-10-11T16:03:23Z

@fabpot changes done.

fabpot · 2018-10-11T17:22:52Z

Thank you @Nek-.

… class (Nek-) This PR was squashed before being merged into the 4.2-dev branch (closes #27742). Discussion ---------- [Process] Add feature "wait until callback" to process class I often see code like the following: ```php $process->start(); // wait for the process to be ready sleep(3); ``` Many times in tests, sometimes in other places. There is a problem with this kind of code because if for any reason the process starts slower than the usual... Your code may fail after that. Also if it's faster, you're losing time for waiting. So here is my proposal: ```php $process->start(); $process->waitUntil(function($type, $output) { // check the output and return true to stop waiting when you got what you wait }); ``` | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | none | License | MIT | Doc PR | Waiting for feedbacks Commits ------- 27eaf83 [Process] Add feature \"wait until callback\" to process class

This PR was merged into the master branch. Discussion ---------- Add some lines of doc for waitUntil() (Process) This adds some documentation for symfony/symfony#27742 Commits ------- 2814fea Add some lines of doc for waitUntil() (Process)

miniyarov · 2018-10-21T09:13:54Z

do {
            $this->checkTimeout();
            $running = '\\' === \DIRECTORY_SEPARATOR ? $this->isRunning() : $this->processPipes->areOpen();
            $output = $this->processPipes->readAndWrite($running, '\\' !== \DIRECTORY_SEPARATOR || !$running);
             foreach ($output as $type => $data) {
                if (3 !== $type) {
                    $wait = !$callback(self::STDOUT === $type ? self::OUT : self::ERR, $data);
                } elseif (!isset($this->fallbackStatus['signaled'])) {
                    $this->fallbackStatus['exitcode'] = (int) $data;
                }
            }
        } while ($wait);

This loop uses CPU %100 while waiting for the output. Because getting process information comes with a cost.
For this reason it's a good idea to put at least a usleep(1000) in loop, to minimize cpu usage.
As done in wait function https://github.com/symfony/process/blob/master/Process.php#L421

nicolas-grekas · 2018-10-21T09:24:10Z

Would you mind sending a PR doing so?

miniyarov · 2018-10-21T10:32:39Z

Yes, forgot to mention it here #28940

miniyarov · 2018-10-22T18:59:46Z

@nicolas-grekas fyi

greg0ire · 2019-01-01T22:15:59Z

src/Symfony/Component/Process/Process.php

+            $output = $this->processPipes->readAndWrite($running, '\\' !== \DIRECTORY_SEPARATOR || !$running);
+
+            foreach ($output as $type => $data) {
+                if (3 !== $type) {


Hey @Nek- ! Happy New Year! I'm a bit late to the party but… could you please explain what this magic number is about? I thought possible values would be 1 or 2, but I didn't expect 3… any pointers? I'm trying to investigate this failure: https://ci.appveyor.com/project/fabpot/symfony/builds/21320382#L1210

This issue is not related to this condition (or doesn't look related to me). Basically, if you extend the timeout for windows only, it may fix the issue. It looks like windows is slooooooow (not that slower on my computer though) to execute a process.

But I can't remember why I used $type !== 3, sorry.

Not sure it has to do with Windows being slow, because if you look, there is the beginning of the output, and then the end of it, but the middle is missing 😅 :

First iteration output\n One more iteration output\n

The code of the test is pretty straightforward, that's why I'm starting to suspect the code might be at fault, maybe here or in WindowsPipes

3 seems to be the index of a pipe that will receive the last exit code, and indeed, does not seem to have much to do with the problem at hand because it is does not seem to be used on Windows.

carsonbot added Status: Needs Review Feature labels Jun 27, 2018

Nek- force-pushed the feature/process-wait-until branch from ac00c28 to 29b946c Compare June 27, 2018 13:12

nicolas-grekas added this to the next milestone Jun 27, 2018

Nek- changed the title ~~Add feature "wait until callback" to process class~~ [Process] Add feature "wait until callback" to process class Jun 27, 2018

mcsky reviewed Jun 27, 2018

View reviewed changes

Nek- force-pushed the feature/process-wait-until branch 2 times, most recently from 9b4a44f to bb56dc2 Compare June 27, 2018 14:19

linaori reviewed Jun 27, 2018

View reviewed changes

Nek- force-pushed the feature/process-wait-until branch from 5339b83 to 0f428eb Compare June 27, 2018 16:13

Nek- force-pushed the feature/process-wait-until branch 2 times, most recently from 4df8f2f to f6b12e0 Compare October 5, 2018 07:41

fabpot requested changes Oct 10, 2018

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Oct 10, 2018

stof reviewed Oct 10, 2018

View reviewed changes

Nek- force-pushed the feature/process-wait-until branch from f6b12e0 to 0617230 Compare October 11, 2018 06:58

fabpot requested changes Oct 11, 2018

View reviewed changes

Nek- force-pushed the feature/process-wait-until branch from 84d938d to 01eb0d5 Compare October 11, 2018 13:41

fabpot approved these changes Oct 11, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Work labels Oct 11, 2018

[Process] Add feature \"wait until callback\" to process class

27eaf83

fabpot force-pushed the feature/process-wait-until branch from 01eb0d5 to 27eaf83 Compare October 11, 2018 17:22

fabpot merged commit 27eaf83 into symfony:master Oct 11, 2018

Nek- deleted the feature/process-wait-until branch October 12, 2018 07:48

xabbuh added the Process label Oct 12, 2018

Nek- mentioned this pull request Oct 15, 2018

Add some lines of doc for waitUntil() (Process) symfony/symfony-docs#10492

Merged

nicolas-grekas modified the milestones: next, 4.2 Nov 1, 2018

This was referenced Nov 3, 2018

Release v4.2.0-BETA1 #29072

Closed

Release v4.2.0-BETA1 #29073

Merged

Nek- mentioned this pull request Nov 28, 2018

[Router] Remove goto in RedirectableUrlMatcher #29362

Closed

greg0ire reviewed Jan 1, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Process] Add feature "wait until callback" to process class #27742

[Process] Add feature "wait until callback" to process class #27742

Nek- commented Jun 27, 2018 •

edited

Loading

mcsky Jun 27, 2018

linaori Jun 27, 2018

Nek- Jun 27, 2018 •

edited

Loading

Nek- commented Jul 16, 2018

linaori commented Jul 16, 2018

Nek- commented Jul 16, 2018

Nek- commented Jul 16, 2018

Nek- commented Oct 5, 2018

Nek- commented Oct 5, 2018

fabpot Oct 10, 2018

fabpot Oct 10, 2018

fabpot commented Oct 10, 2018

stof Oct 10, 2018

Nek- Oct 11, 2018 •

edited

Loading

fabpot Oct 11, 2018

fabpot Oct 11, 2018

Nek- commented Oct 11, 2018

fabpot commented Oct 11, 2018

miniyarov commented Oct 21, 2018

nicolas-grekas commented Oct 21, 2018

miniyarov commented Oct 21, 2018

miniyarov commented Oct 22, 2018

greg0ire Jan 1, 2019

Nek- Jan 1, 2019

greg0ire Jan 2, 2019

greg0ire Jan 2, 2019

[Process] Add feature "wait until callback" to process class #27742

[Process] Add feature "wait until callback" to process class #27742

Conversation

Nek- commented Jun 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nek- Jun 27, 2018 • edited Loading

Choose a reason for hiding this comment

Nek- commented Jul 16, 2018

linaori commented Jul 16, 2018

Nek- commented Jul 16, 2018

Nek- commented Jul 16, 2018

Nek- commented Oct 5, 2018

Nek- commented Oct 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Oct 10, 2018

Choose a reason for hiding this comment

Nek- Oct 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nek- commented Oct 11, 2018

fabpot commented Oct 11, 2018

miniyarov commented Oct 21, 2018

nicolas-grekas commented Oct 21, 2018

miniyarov commented Oct 21, 2018

miniyarov commented Oct 22, 2018

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

Nek- commented Jun 27, 2018 •

edited

Loading

Nek- Jun 27, 2018 •

edited

Loading

Nek- Oct 11, 2018 •

edited

Loading