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] Fix pipes cleaning on Windows#19118
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
nicolas-grekas commentedJun 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 | - |
fabpot commentedJun 20, 2016
I'm sure you are working on a unit test to cover this, right? |
9b1b04c toee3d7d7Compare| foreach ($this->filesas$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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
it's just a method, but anyway: changed
nicolas-grekas commentedJun 21, 2016
Status: needs work |
aee7694 to4b2b5dfComparenicolas-grekas commentedJun 21, 2016
Status: needs review |
| } | ||
| for ($i =0;; ++$i) { | ||
| foreach ($pipesas$pipe =>$name) { | ||
| @unlink($file =sprintf('%s\\sf_proc_%02X.%s',$tmpDir,$i,$name)); |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ok, fixed as part of other changes to make tests green
2ccb00d to9ffb895Compare| if (false ===$file ||false ===$this->fileHandles[$offset] = @fopen($file,'rb')) { | ||
| thrownewRuntimeException('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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What if another process is started at the same time?
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.
wb allows it without any issue
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.
Alright then
romainneutron commentedJun 21, 2016
👍 |
romainneutron commentedJun 21, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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