Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix race condition in test harness #1173

Merged
merged 1 commit into from
Dec 4, 2019
Merged

Fix race condition in test harness #1173

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 15 additions & 18 deletions classes/ETL/DataEndpoint/Filter/ExternalProcess.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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]);
Copy link
Contributor

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 the proc_close, but I've never tried that.

Copy link
Member Author

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.


// 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) {
Copy link
Contributor

Choose a reason for hiding this comment

The 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:

Returns the termination status of the process that was run. In case of an error then -1 is returned.

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.

Copy link
Member Author

@jpwhite4 jpwhite4 Dec 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

The 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

2019-12-02 20:27:56 [error] Error 2 executing external filter process: sh: 1: Syntax error: "(" unexpected
2019-12-02 20:27:56 [error] Error 127 executing external filter process: sh: 1: gobbledygook: not found

$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);

}

/**
Expand Down