[Process][2.2] Fix #8739 #8741
Merged
[Process][2.2] Fix #8739 #8741
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Closed
|
some internal calls of |
|
Tests are failing because of another test suite than Process : |
fabpot
added a commit
that referenced
this pull request
Aug 13, 2013
This PR was merged into the 2.2 branch. Discussion ---------- [Process][2.2] Fix #8739 | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #8739 | License | MIT This adds a fix to #8739. Whenever a call is done to to any non-blocking methods (`Process::isRunning`, `Process::isStopped`, `Process::isTerminated`, `Process::getStatus`, `Process::getPid`...), buffers are read, and callback executed. Such code will now work : ``` $process->start(function ($type, $data) { echo $data; }); while ($process->isRunning()) { // some stuff // callback is executed } ``` Commits ------- fa769a2 [Process] Add more precision to Process::stop timeout 57d4159 [Process] Avoid zombie process in case of unit tests failure 3ef517b [Process] Fix #8739 7716fb2 [Process] Add failing test for #8739 bff6f3c [Process] Fix CS
|
Merged now. Can you check the additional changes needed in 2.3 and send a PR? Thanks. hmm, on my computer, this PR slows down the Process unit tests a lot and produces some output: |
|
Thanks for the feedback, going to propose a PR for 2.3 right now. Unfortunately, I can not dig later tonight for the slowness / output, will check it tomorrow morning. thanks for the feedback |
|
2.3 seems ok for me, no need for aditional changes. |
| @@ -343,7 +360,7 @@ public function testPhpDeadlock() | |||
|
|
|||
| // Sleep doesn't work as it will allow the process to handle signals and close | |||
| // file handles from the other end. | |||
| $process = $this->getProcess('php -r "while (true) {}"'); | |||
| $process = $this->getProcess('php -r "sleep 4"'); | |||
There was a problem hiding this comment.
@romainneutron Are you sure about this change?
There was a problem hiding this comment.
As I explained in #8740 :
On my computer, zombie processes might have remain in case of unit test failure.
It happend that some infinite loops were running and slowing down my system. A ps call alerted me that many (~100) php -r "while (true) {}" were running after half an hour debugging things.
In this case, we trigger the Process::stop call in the __destruct method. I don't see any problem here, do you ?
There was a problem hiding this comment.
- Notice the comment above the code (it says sleep wouldn't work)
- shouldn't it be
sleep(4);rathern thansleep 4?
There was a problem hiding this comment.
you are indeed right, I'll make some more tests tomorrow
romainneutron
added a commit
to romainneutron/symfony
that referenced
this pull request
Aug 14, 2013
fabpot
added a commit
that referenced
this pull request
Aug 14, 2013
This PR was merged into the 2.2 branch. Discussion ---------- [Process] Fix #8746 : slowness added in unit tests since #8741 | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #8746 | License | MIT Commits ------- 8c4bae3 [Process] Revert change 8d9c7c6 [Process] Fix #8746 : slowness added in unit tests since #8741
fabpot
added a commit
that referenced
this pull request
Aug 14, 2013
This PR was merged into the 2.3 branch. Discussion ---------- [Process] Fix #8742 : Signal-terminated processes are not successful | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | require #8741 to be merged to pass tests | Fixed tickets | #8742 | License | MIT Commits ------- 909fab6 [Process] Fix #8742 : Signal-terminated processes are not successful
fabpot
added a commit
that referenced
this pull request
Aug 14, 2013
* 2.3: [Process] Revert change [Process] Fix #8746 : slowness added in unit tests since #8741 [Process] Fix #8742 : Signal-terminated processes are not successful corrected English grammar (s/does not exists/does not exist) [Process] Add more precision to Process::stop timeout [Process] Avoid zombie process in case of unit tests failure [Process] Fix #8739 [Process] Add failing test for #8739 [Process] Fix CS [TwigBridge] removed superflous ; when rendering form_enctype() (closes #8660) Fixed documentation grammar for AuthenticationManagerInterface::authenticate() [Validator] fixed the wrong isAbstract() check against the class (fixed #8589) [TwigBridge] Prevent code extension to display warning Fix internal sub-request creation [FrameworkBundle] made code more generic [Form] Moved auto_initialize option to the BaseType Use strstr instead of strpos Make sure ContextErrorException is loaded during compile time errors Fix empty process argument escaping on Windows Ignore null value in comparison validators Conflicts: src/Symfony/Component/Debug/Tests/ErrorHandlerTest.php src/Symfony/Component/HttpKernel/Tests/Fragment/InlineFragmentRendererTest.php src/Symfony/Component/Process/Process.php
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This adds a fix to #8739. Whenever a call is done to to any non-blocking methods (
Process::isRunning,Process::isStopped,Process::isTerminated,Process::getStatus,Process::getPid...), buffers are read, and callback executed.Such code will now work :