Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Process] Add feature "wait until callback" to process class#27742
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ac00c28 to29b946cCompare| $p->stop(); | ||
| $this->assertEquals("First iteration output\nSecond iteration output\nOne more iteration output\n",$completeOutput); | ||
| $this->assertLessThan(15,microtime(true) -$start); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
15 is too much, the process do 3 iterations inKillableProcessWithOutput.php so it should take less than 2 seconds
9b4a44f tobb56dc2Compare| foreach ($outputsas$output) { | ||
| usleep($iterationTime); | ||
| $iterationTime *=10; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Won't this create an extremely slow test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The test should end after1s 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::testWaitUntilSpecificOutputFailed asserting that 4.3028318881989 is less than 2.C:\projects\symfony\src\Symfony\Component\Process\Tests\ProcessTest.php:155Here is anugly fix suggestion.
5339b83 to0f428ebCompareNek- commentedJul 16, 2018
Hello@iltar what's the state of this PR? :) |
linaori commentedJul 16, 2018
@Nek- it's your PR, so I'm not sure what you mean 😅 |
Nek- commentedJul 16, 2018
@iltar sorry you're so active on PRs that I thought you were in the core team 😅! |
Nek- commentedJul 16, 2018
ping@romainneutron@nicolas-grekas then :) |
4df8f2f tof6b12e0CompareNek- commentedOct 5, 2018
Annnnnd... Rebased again. ping@romainneutron@nicolas-grekas |
Nek- commentedOct 5, 2018
Error on CI not related. |
| } | ||
| /** | ||
| * Waits until the callback return true. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
typo: returns
| if (!$this->processPipes->haveReadSupport()) { | ||
| $this->stop(0); | ||
| thrownew \LogicException('Pass the callback to the Process::start method or enableOutput to use a callback with Process::waitUntil'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 addcall before enableOutput:... method or call "enableOutput" ...
fabpot commentedOct 10, 2018
Can you also add some notes in the CHANGELOG file of the component? |
| returnfunction ($type,$data)use ($callback) { | ||
| if (null !==$callback) { | ||
| \call_user_func($callback,$type,$data); | ||
| return$callback($type,$data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
does this finally work forall kind of callables in PHP 7.1+ ? In PHP 5.6, I know there was still 1 kind of callable failing this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I was not aware of that fact. No idea. Can you give more insights to verify that fact?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Let's revert this change, no big deal (same below)
Uh oh!
There was an error while loading.Please reload this page.
f6b12e0 to0617230Compare| returnfunction ($type,$data)use ($callback) { | ||
| if (null !==$callback) { | ||
| \call_user_func($callback,$type,$data); | ||
| return$callback($type,$data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Let's revert this change, no big deal (same below)
84d938d to01eb0d5CompareNek- commentedOct 11, 2018
@fabpot changes done. |
01eb0d5 to27eaf83Comparefabpot commentedOct 11, 2018
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 classI often see code like the following:```php$process->start();// wait for the process to be readysleep(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 feedbacksCommits-------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 forsymfony/symfony#27742Commits-------2814fea Add some lines of doc for waitUntil() (Process)
miniyarov commentedOct 21, 2018
This loop uses CPU %100 while waiting for the output. Because getting process information comes with a cost. |
nicolas-grekas commentedOct 21, 2018
Would you mind sending a PR doing so? |
miniyarov commentedOct 21, 2018
Yes, forgot to mention it here#28940 |
miniyarov commentedOct 22, 2018
@nicolas-grekas fyi |
| $output =$this->processPipes->readAndWrite($running,'\\' !== \DIRECTORY_SEPARATOR || !$running); | ||
| foreach ($outputas$type =>$data) { | ||
| if (3 !==$type) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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\nOne more iteration output\nThe code of the test is pretty straightforward, that's why I'm starting to suspect the code might be at fault, maybe here or inWindowsPipes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Uh oh!
There was an error while loading.Please reload this page.
I often see code like the following:
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: