[Process] Fix pipes cleaning on Windows #19118
Conversation
nicolas-grekas
commented
Jun 20, 2016
| Q | A |
|---|---|
| Branch? | 2.7 |
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #19089 |
| License | MIT |
| Doc PR | - |
ad3663a to
5a39000
Compare
|
I'm sure you are working on a unit test to cover this, right? |
9b1b04c to
ee3d7d7
Compare
| @@ -52,9 +52,11 @@ public function __construct($disableOutput, $input) | ||
| Process::STDERR => tempnam(sys_get_temp_dir(), 'err_sf_proc'), | ||
| ); | ||
| foreach ($this->files as $offset => $file) { | ||
| if (false === $file || false === $this->fileHandles[$offset] = @fopen($file, 'rb')) { | ||
| if (false === $file || false === $h = @fopen($file, 'rb')) { | ||
| $this->__destruct(); |
There was a problem hiding this comment.
I know this should work but to me it looks wrong to do it this way. Can't we rather move the code from __destruct() in its own method and reuse that then?
There was a problem hiding this comment.
it's just a method, but anyway: changed
ee3d7d7 to
dcb2109
Compare
|
Status: needs work |
aee7694 to
4b2b5df
Compare
|
Status: needs review |
4b2b5df to
1bd1c15
Compare
| } | ||
| for ($i = 0;; ++$i) { | ||
| foreach ($pipes as $pipe => $name) { | ||
| @unlink($file = sprintf('%s\\sf_proc_%02X.%s', $tmpDir, $i, $name)); |
There was a problem hiding this comment.
shouldn't you check whether $this->files[$pipe] is already set, to avoid reopening the same pipe several times in case of a retry when the other one fails ? Or is it done on purpose ?
There was a problem hiding this comment.
it's on purpose: code is simpler, and $i is the name for err & out files for the same process
There was a problem hiding this comment.
then, you need to close and unlink files you opened for this iteration in case you move to a retry loop
There was a problem hiding this comment.
I don't think so, for 3 reasons: rare error condition, simpler code, and the file will be reused on the next run of the process
There was a problem hiding this comment.
ok, fixed as part of other changes to make tests green
2ccb00d to
9ffb895
Compare
| if (false === $file || false === $this->fileHandles[$offset] = @fopen($file, 'rb')) { | ||
| throw new RuntimeException('A temporary file could not be opened to write the process output to, verify that your TEMP environment variable is writable'); | ||
| $tmpDir = sys_get_temp_dir(); | ||
| if (!@fopen($file = $tmpDir.'\\sf_proc_00.check', 'wb')) { |
There was a problem hiding this comment.
What if another process is started at the same time?
There was a problem hiding this comment.
wb allows it without any issue
9ffb895 to
d54cd02
Compare
|
👍 |
|
Muchas gracias @nicolas-grekas! |
This PR was merged into the 2.7 branch. Discussion ---------- [Process] Fix pipes cleaning on Windows | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #19089 | License | MIT | Doc PR | - Commits ------- d54cd02 [Process] Fix pipes cleaning on Windows