-
Notifications
You must be signed in to change notification settings - Fork 68
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix race condition in test harness #1173
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -123,6 +123,7 @@ public function filter($in, $out, &$consumed, $closing) | |
// Close the process's input file so it can finish up | ||
|
||
@fclose($this->pipes[0]); | ||
$this->pipes[0] = null; | ||
|
||
// Process all data left on the pipe from the process | ||
|
||
|
@@ -220,30 +221,26 @@ public function onCreate() | |
|
||
public function onClose() | ||
{ | ||
// There is difficulty detecting if there is an error running the external | ||
// process. By the time the process executes and fails, it is entirely possible | ||
// that all data could be written to the process before we can discover that it | ||
// has a non-zero exit status. | ||
if ($this->pipes[0]) { | ||
fclose($this->pipes[0]); | ||
} | ||
fclose($this->pipes[1]); | ||
|
||
$status = proc_get_status($this->filterResource); | ||
// Read stderr stream to EOF before closing. | ||
$errorOutput = ''; | ||
while (($buffer = fgets($this->pipes[2])) !== false) { | ||
$errorOutput .= $buffer; | ||
} | ||
fclose($this->pipes[2]); | ||
|
||
// While the process is running the exit status is shown as -1 | ||
$exitStatus = proc_close($this->filterResource); | ||
fclose($this->tmpResource); | ||
|
||
if ( ! in_array($status['exitcode'], array(-1, 0)) ) { | ||
$errorMessage = 'Error executing external filter process: '; | ||
while ( ! feof($this->pipes[2]) ) { | ||
$errorMessage .= fgets($this->pipes[2]); | ||
} | ||
if ($exitStatus !== 0) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know how accurate this is, but the proc_close documentation states that:
I don't know if it's possible that a value other than 0 or -1 is returned, but elsewhere I've written code that explicitly compares against -1. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. exit status can be a range of values not just 0 or -1. The unit tests have a 127 and a 1 return values (this can be seen in the logs). [edit to add] .. of the unit tests
|
||
$errorMessage = 'Error ' . $exitStatus . ' executing external filter process: ' . $errorOutput; | ||
$this->logError($errorMessage); | ||
throw new \Exception($errorMessage); | ||
} | ||
|
||
fclose($this->pipes[0]); | ||
fclose($this->pipes[1]); | ||
fclose($this->pipes[2]); | ||
proc_close($this->filterResource); | ||
fclose($this->tmpResource); | ||
|
||
} | ||
|
||
/** | ||
|
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.
Is it possible that anything additional could be written to STDOUT and STDERR between here and the
proc_close
below? I'm curious if this can be put after theproc_close
, but I've never tried that.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 docs state that you should close all open file handles before the
proc_close
to avoid deadlocks. Since STDERR is read until EOF (or error) then you shouldn't get any more data out of stderr. Any data sitting in the stdout buffer will get thrown away, but this should be handled by the code starting on line 121.If you add a filter that writes an infinite amount of data to stderr after stdin is closed then the proc_close() call will never execute. Don't do this.