[Process][2.2] Fix Process component on windows #8924
Closed
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
| $this->fileHandles[Process::STDOUT], | ||
| $this->fileHandles[Process::STDERR], | ||
| ); | ||
| } else { |
There was a problem hiding this comment.
no need to use else as the if returns
|
Does it also workaround for https://bugs.php.net/bug.php?id=60120 ? |
|
@Koc yes, but as I previously mentioned : this workaround may produce corrupted output/error output in some race conditions. |
|
For the record, I did not add a workaround in this PR, it already existed. I just fixed the tests and decouples the file handling in a sub class. |
|
@fabpot is anything missing / wrong in this one ? |
fabpot
added a commit
that referenced
this pull request
Sep 7, 2013
This PR was squashed before being merged into the 2.2 branch (closes #8924). Discussion ---------- [Process][2.2] Fix Process component on windows | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #8836, #8799, #7078 | License | MIT This PR fixes Process on windows (almost, see note below). - Some unit tests were not Windows compatible - Use a file handle for STDERR as well as STDOUT to avoid blocking - Decouple pipes and descriptors from Process As this move some a part of Process in a sub class, I hope merging this in 2.3 and master would not be a PITA. I'm here to make some adjustments after theses merge if needed. **Important note** : We are using file handles instead of streams for `proc_open` pipes as described in the code (see [PHP bug #51800](https://bugs.php.net/bug.php?id=51800)). Unfortunately, this workaround may produce corrupted output/error output in some race conditions. That's why `AbstractProcessTest::testProcessPipes` randomly fails when using file handles (on unix and windows). Commits ------- 4a76c76 [Process][2.2] Fix Process component on windows
|
Indeed, merging this in 2.3 is a PITA. Can you do it yourself? Merge 2.2 in 2.3, resolve the conflicts, and send a PR on 2.3. Thanks. |
|
sure, I'll do that as soon as I'm on the keyboard |
This was referenced Sep 7, 2013
fabpot
added a commit
that referenced
this pull request
Sep 10, 2013
This PR was merged into the 2.2 branch. Discussion ---------- [Process][2.2] Fix #8970 : read output once the process is finished, enable pipe tests on Windows | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #8970 | License | MIT This fix the read of file handles after `proc_close` as described in the issue. I enable some stdin=>stdout pipes tests on windows with some dedicated buffer size. Solving the issue, I finally reproduced this [PHP bug](https://bugs.php.net/bug.php?id=65650) that I first described in my PR note in #8924 Most of Windows usage should be okay, but in case of a program throws lots of output and fills the buffer, some data might be lost/corrupted. Should it be documented ? This is a Windows only known issue. Commits ------- 1e75cf9 [Process] Fix #8970 : read output once the process is finished, enable pipe tests on Windows
fabpot
added a commit
that referenced
this pull request
Oct 26, 2013
…ows platform (romainneutron) This PR was merged into the 2.2 branch. Discussion ---------- [2.2][Process] Fix #9343 : revert file handle usage on Windows platform | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #9343 | License | MIT Hello, I propose to revert the use of file handles only for `STDOUT` on Windows platform (see implementation in 2.2.6 [here](https://github.com/symfony/symfony/blob/4059720232e8c4acce6ca31627b87b904cf1aa77/src/Symfony/Component/Process/Process.php#L231-L242)). When I decoupled pipes management from `Process` in #8924, I used file handles for both `STDOUT` and `STDERR`. This was an error as it introduced random failure in reading the handles (reported as [PHP#65650](https://bugs.php.net/bug.php?id=65650)). Reverting to the previous implementation solves the issue. My apologies for the issues it introduced. Versions that have been affected by the bug are 2.2.7, 2.2.8, 2.2.9, 2.3.4, 2.3.5 and 2.3.6. Side note : I thought about testing the file handles implementation on *nix, but it fails most of the time where as Windows is okay. Unit testing on windows is okay (AbstractProcessTest::testProcessPipes tests it), but I don't provide a travis compatible test. Commits ------- e9dd408 [Process] Fix #9343 : revert file handle usage on Windows platform
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 PR fixes Process on windows (almost, see note below).
As this move some a part of Process in a sub class, I hope merging this in 2.3 and master would not be a PITA. I'm here to make some adjustments after theses merge if needed.
Important note :
We are using file handles instead of streams for
proc_openpipes as described in the code (see PHP bug #51800). Unfortunately, this workaround may produce corrupted output/error output in some race conditions. That's whyAbstractProcessTest::testProcessPipesrandomly fails when using file handles (on unix and windows).